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=638647 --- Comment #6 from Adam Litke <agl@xxxxxxxxxx> 2010-10-26 13:01:15 EDT --- Thank you for your review. The latest files can be found below: http://github.com/downloads/aglitke/mom/mom.spec http://github.com/downloads/aglitke/mom/mom-0.2.1-3.fc13.src.rpm In reply to comment #5) > I'm not a sponsor, so I won't be able to approve the package, but here > are a couple more comments. > > - I'd rather see the patch split into 2 patches, one for the initscript > and one for the spec. Actually, the spec part is useless, as the bundled > spec file is not used. Done. I want to keep the spec file patch so I can track the changes that need to be committed to the MOM tree once this review process is complete. > - The initscript part of the patch turns 2 lines into 1 which then will > span over 80 characters, please fix. Done. > - Patches need to have an upstream reference (bug/commit/...).This is > intended to help keep track of the patches. As you're upstream and this > will probably make it in the leext release, this is not a big deal, but > I thought I'd mention it. Thanks. These patches are a convenience for me so I don't have to keep changing upstream for each iteration of this review. > - The %build and %install section are not following the template. The build > part should not be done from within the %install section. See > /etc/rpmdevtools/spectemplate-python.spec. Fixed. > - The install of the initscript could be made shorter : > install -Dp contrib/momd.init $RPM_BUILD_ROOT/%_initddir/momd Ok. Thanks. > - No need to copy the LICENSE and README files to the docdir, just using > %doc LICENSE README in the %files section will be enough. Got it. > - The INSTALL file doesn't contain any useful content, it's not needed > to include it currently. This might be changed later if/when its content > evolves. Are you referring to the README file? I was hoping I could leave it included since I will be updating it soon. I don't like the idea of simply deleting an installed file. > - Use macros everywhere possible. When moving the examples, replace > /usr/share/doc by %{_defaultdocdir}. This should be consistent now. > - It might be better to alter setup.py to install the doc to the proper > location rather than move them after install. Will look into this for a future upstream release. > - BuildRoot and %clean are not needed anymore with recent Fedora > releases. However, this is still needed for EL5, so keep it if you want > to also have EPEL branches. In this case, you'll also want to replace > %{_initddir} by %{_initrddir}. I chose to go the backward-compatible route. > - There are some useless comments in the spec (first line and in %post). Gone now. > - Please consider providing a default/dummy conf file and mark it as > %config(noreplace) in the %files section. I now duplicate one of the example config files and install it as /etc/momd.conf (which agrees with the init script). > - You might want to use %{version} and possibly %{name] macros in the > Source0 tag, so you don't have to change the version in 2 places when > upgrading to a later version. Agreed. -- 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