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 #5 from Xavier Bachelot <xavier@xxxxxxxxxxxx> 2010-10-14 05:04:16 EDT --- 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. - The initscript part of the patch turns 2 lines into 1 which then will span over 80 characters, please fix. - 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. - 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. - The install of the initscript could be made shorter : install -Dp contrib/momd.init $RPM_BUILD_ROOT/%_initddir/momd - No need to copy the LICENSE and README files to the docdir, just using %doc LICENSE README in the %files section will be enough. - 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. - Use macros everywhere possible. When moving the examples, replace /usr/share/doc by %{_defaultdocdir}. - It might be better to alter setup.py to install the doc to the proper location rather than move them after install. - Be consistent with macros. You use both %_initddir and %{_defaultdocdir}. Choose one style and stick with it. The preferred style is usually %{_sbindir}. - No need for %{_defaultdocdir}/%{name}-%{version} in the %files section, this will automatically be picked up. - Adding a blank line between changelog entries improve readability. - 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}. - There are some useless comments in the spec (first line and in %post). - Please consider providing a default/dummy conf file and mark it as %config(noreplace) in the %files section. - 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. -- 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