https://bugzilla.redhat.com/show_bug.cgi?id=1089770



--- Comment #5 from NIWA Hideyuki <[email protected]> ---
Hi, thank you for the comment. 
I corrected them as follows. 

-
> Not a full review, but one needs to start somewhere.
> 
> Consider running "fedora-review -b 1089770" to point that tool at this 
> ticket. It evaluates the "SRPM URL:" and "Spec URL:" lines and performs many 
> helpful checks.
> 
> 
> > Summary:               A LXC facility tool
> 
> Even in the %summary there is enough room to expand "LXC". Ommitting the 
> leading article improves readability.
> 
>   Summary: Linux Containers (LXC) facility tool
> 
> https://fedoraproject.org/wiki/Examples_of_good_package_summaries
> 

I thought that your description was good. 
Please let me use it for the summary as it is. 
Summary:               Linux Containers (LXC) facility tool


> 
> > Source:                
> > http://prdownloads.sourceforge.net/lxcfacility/source/lxcf-%{version}.tar.gz
> 
> https://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net
> 

I corrected it as follows.
Source0:              
http://downloads.sourceforge.net/lxcfacility/source/%{name}-%{version}.tar.gz


> 
> > Prefix:                %{_prefix}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Relocatable_packages
> 

Prefix was erased. 

> 
> > Requires:              python
> > Requires:              python-IPy
> > Requires:              virt-manager
> 
> Please add comments on explicit dependencies. What exactly is needed from 
> these packages? Files may move. And would these deps need to be arch-specific?
> 

These "Require" was erased. 

> 
> 
> > %build
> > make clean
> 
> Comment on unusual command invocations like this one. Why is it needed?
> 

I erased it by thinking it was unnecessary. 

> 
> > %files
> > %defattr(-,root,root)
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
> 

I erased this. 

> > %{_prefix}/sbin/lxcf
> > %{_libdir}/lxcf/lxcf-init
> 

I corrected it as follows.
%{_sbindir}/lxcf
%{_libdir}/lxcf/  

> Directory %_libdir/lxcf is not included yet.
> 
> > %{_libdir}/lxcf/lxcf-keygen
> > %{_libdir}/lxcf/lxcf-rc
> > %{_libdir}/lxcf/lxcf-config
> > %{_libdir}/lxcf/lxcf-maintenance
> > [...]
> 
> A comment in the spec file here would be beneficial, too. You list the 
> specific file names of over 50 files. Is that really necessary? For example, 
> do you want the build to fail if any of those files gets 
> added/dropped/renamed in a future update? If so, a brief comment could 
> explain that. Else, consider using '*' based wildcards to include files more 
> conveniently.
> 

It is as you say. 
I corrected it as follows.
%{_libdir}/lxcf/  

> 
> > %doc %attr(-,root,root)
> 
> What does that line do?

I erased it because I was unnecessary. 

Thanks.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
[email protected]
https://admin.fedoraproject.org/mailman/listinfo/package-review

Reply via email to