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(a)redhat.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |tcallawa(a)redhat.com
--- Comment #3 from Tom "spot" Callaway <tcallawa(a)redhat.com> 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_sp...
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_Ow...
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.