Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=702143 Tom "spot" Callaway <tcallawa@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tcallawa@xxxxxxxxxx --- Comment #3 from Tom "spot" Callaway <tcallawa@xxxxxxxxxx> 2011-06-13 12:36:12 EDT --- Initial Review: * BuildRoot is not necessary, except on EPEL 5 and older. * rm -rf %{buildroot} at the beginning of %install is not necessary, except on EPEL 5 and older * %clean section is not necessary if it is just rm -rf %{buildroot}, except on EPEL 5 or older * %defattr(-,root,root,-) is not necessary in %files sections, except on EPEL 5 or older. All four of those items have sane defaults in current Fedora. :) * Given that this is a Fedora Hosted project, I would really like to see the release tarball have a canonical upstream home, e.g. https://fedorahosted.org/releases/g/r/grid/%{name}-%{version}.tar.gz Once that happens, the Source0 field in the spec file should reflect that. * Is there a need for Requires: ruby if you have Requires: ruby(abi) = 1.8 ? * Also, you have the BuildRequires split out (and duplicated) across subpackage sections, this is unnecessary. BuildRequires apply to the SRPM only, so just put them all at the top (once). * Descriptions should be proper sentences and end in periods. Also, try to flesh them out a bit. Two of your subpackages have identical descriptions. * Strongly consider making a systemd unit file instead. SysVinit files are deprecated. (https://fedoraproject.org/wiki/Packaging:Systemd) * You're missing the exit 0 at the end of %pre (see https://fedoraproject.org/wiki/Packaging:UsersAndGroups) * Your %postun scriptlet seems entirely inappropriate. Compare it to the Fedora standard here: https://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscripts_in_spec_file_scriptlets Of course, you're going to move to a systemd unit file, right? :) * Please read the guidelines concerning file and directory ownership: https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership I suspect you need to own some/all of the directories under %{ruby_sitelib} that you're using. You may also be able to simplify the %files listings as a result. * Don't bother conditionalizing for Fedora newer than 12. At this point, you can assume this package will never go into a Fedora older than 14. Just drop those conditionals, or reframe them for RHEL if you want to put this in EPEL. ==== Show me a new package, and I'll do the final review. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review