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 --- Comment #5 from Tom "spot" Callaway <tcallawa@xxxxxxxxxx> 2011-09-21 10:33:07 EDT --- Okay, here's the next round of feedback: * %define rel 8 There is no need to separate out that variable, and you really shouldn't be using it in the tarball naming schema. Release: is supposed to indicate the build revision of the spec file independent of the upstream source code, so that you can make spec file fixes or rebuild to resolve dependency changes without needing to make a new source tarball. * Instead of using %define, use %global unless something breaks. * Is there a reason you've opted not to provide a tarball at a URL via Fedora hosted? * Since you're conditionalizing EL items, conditionalize BuildRoot: and the rm -rf %{buildroot} at the beginning of %install in the same way. * Add BuildRequires: systemd-units (for %{_unitdir}). * The el5 conditionalization for %clean should be around the whole %clean section, not just the rm -rf %{buildroot}. * You are owning the %dir %{ruby_sitelib}/mrg/grid/ but none of the directories below it. I think you can replace the entire ruby-wallaby files section with this: %files -n ruby-wallaby %if %{building_for_el5} %defattr(-, root, root, -) %endif %{ruby_sitelib}/mrg/grid/ %if %{has_sinatra} %exclude %{ruby_sitelib}/mrg/grid/config/shell/cmd_http_server.rb %files -n wallaby-http-server %{ruby_sitelib}/mrg/grid/config/shell/cmd_http_server.rb %endif * My personal opinion is that all of the conditionalization for el5 makes the spec really nasty and difficult to parse, especially since if this goes into EPEL (as opposed to RHEL directly), there will be a separate git branch for EL5 & EL6, so you can just have EL5 specific spec releases there. Nevertheless, I do understand the logic behind "one spec everywhere", so I'll not hold back the review on this point, just wanted to make it. ========= I should be able to do a final pass of this review once I see a package with those changes made -- 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