Spec URL: http://packages.getwallaby.com/wallaby.spec SRPM URL: http://packages.getwallaby.com/wallaby-0.10.5-5.fc14.src.rpm Description: Wallaby is a configuration service for Condor pools that is accessible over the Qpid Management Framework. I am a new packager and would appreciate a sponsor.
If you use the "copy" command for the installation, add always the switch -p to keep timestamps. If you need a sponsor, you have to add FE-NEEDSPONSOR to "Blocks:". I did this for you.
Here's an updated package: http://packages.getwallaby.com/wallaby-0.10.5-6.fc14.src.rpm
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.
A new package is available at: http://packages.getwallaby.com/wallaby-0.10.5-8.fc14.src.rpm The spec file there is updated as well: http://packages.getwallaby.com/wallaby.spec I believe that this updated package addresses all of Tom's concerns (except for the canonical tarball location). It uses systemd on F15 or later and conditionalizes out all of the EL5-specific behaviors so that these only happen when building on EL5. (The delay was largely due to fixing some F15-specific bugs in the "rhubarb" library that Wallaby uses.)
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
Thanks, Tom. The updated package is here: http://packages.getwallaby.com/wallaby-0.10.6-1.fc14.src.rpm
rpmlint output: wallaby.src: W: file-size-mismatch wallaby-0.10.6.tar.gz = 532480, https://fedorahosted.org/releases/g/r/grid/wallaby-0.10.6.tar.gz = 347590 Sure enough, the sizes and sums do not match: 3f18b05e52dbb7af1f6e4c40cefea2d50d162af732897bc7057bb21bd2199841 wallaby-0.10.6.tar.gz 4175e82e705eafe07d41ccdc58a04fd27890b9dfaadc2e01420971264a17341c rpmbuild/SOURCES/wallaby-0.10.6.tar.gz Please resolve that. :) wallaby.noarch: W: non-standard-uid /var/lib/wallaby wallaby wallaby.noarch: W: non-standard-gid /var/lib/wallaby wallaby wallaby.noarch: W: non-standard-uid /var/log/wallaby wallaby wallaby.noarch: W: non-standard-gid /var/log/wallaby wallaby Safe to ignore. wallaby.noarch: E: script-without-shebang /lib/systemd/system/wallaby.service This is happening because of this line in the -wallaby files section: %defattr(0755,root,root,-) It should go away if you add a defattr reset line directly above %{_unitdir}/wallaby.service So that the section will look like: ... %defattr(0755,root,root,-) %{_bindir}/wallaby-agent %if %{want_systemd} %defattr(-,root,root,-) %{_unitdir}/wallaby.service %else %{_initrddir}/wallaby %endif systemd service files do not need to be chmod +x (as opposed to old sysvinit files which do). wallaby.noarch: W: no-manual-page-for-binary wallaby-agent wallaby-utils.noarch: W: no-manual-page-for-binary wallaby Safe to ignore for packaging purposes, but still recommended to do as a best practice for providing documentation. ruby-wallaby.noarch: W: no-documentation wallaby-python.noarch: W: no-documentation wallaby-http-server.noarch: W: no-documentation All safe to ignore. *** Fix those last minor items and I think this one is done.
Tom, thanks again for the review. I've posted an updated package here: http://packages.getwallaby.com/wallaby-0.11.0-3.fc14.src.rpm
The sizes match now, but the sums do not. I'm not sure what you're doing here to cause this, but it is a problem: [spot@pterodactyl ~]$ sha256sum rpmbuild/SOURCES/wallaby-0.11.0.tar.gz 867fe24e53da190f0ca080d3f691b64df463a046e505397eac43dcbf884c7453 rpmbuild/SOURCES/wallaby-0.11.0.tar.gz [spot@pterodactyl ~]$ wget https://fedorahosted.org/releases/g/r/grid/wallaby-0.11.0.tar.gz --2011-10-12 16:12:28-- https://fedorahosted.org/releases/g/r/grid/wallaby-0.11.0.tar.gz Resolving fedorahosted.org... 66.135.52.17 Connecting to fedorahosted.org|66.135.52.17|:443... connected. HTTP request sent, awaiting response... 200 OK Length: 355200 (347K) [application/x-gzip] Saving to: “wallaby-0.11.0.tar.gz” 100%[==========================================================================================================================>] 355,200 323K/s in 1.1s 2011-10-12 16:12:30 (323 KB/s) - “wallaby-0.11.0.tar.gz” saved [355200/355200] [spot@pterodactyl ~]$ sha256sum wallaby-0.11.0.tar.gz 10d760629bd4c09975a8bc2759f12b5b8ac646d01dd8469beffc5ec8a5715ab6 wallaby-0.11.0.tar.gz *** Aside from that, everything else seems fine.
Sorry about that. Wallaby's compressed-tarball script used to generate a git archive of the current release's tag and then pipe that to gzip. I didn't realize that gzip stored modification times in compressed files even when reading from standard input; now the script pipes the git archive to gzip -n. I've fixed the script and uploaded a new package here: http://packages.getwallaby.com/wallaby-0.11.0-4.fc14.src.rpm
Looks good, thanks for figuring that out. This package is APPROVED. I've also sponsored you in FAS. Pick up from step 14 here: http://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Install_the_Client_Tools_.28Koji.29 (You can skip step 15, I did that.)
New Package SCM Request ======================= Package Name: wallaby Short Description: configuration service for Condor pools Owners: willb Branches: f15 f16 InitialCC:
Git done (by process-git-requests).
wallaby-0.12.4-1.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/wallaby-0.12.4-1.fc16
wallaby-0.12.4-1.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/wallaby-0.12.4-1.fc15
wallaby-0.12.4-1.fc15 has been pushed to the Fedora 15 testing repository.
wallaby-0.12.4-1.fc16 has been pushed to the Fedora 16 stable repository.
wallaby-0.12.4-1.fc15 has been pushed to the Fedora 15 stable repository.