[Bug 638647] Review Request: mom - Dynamically manage system resources on virtualization hosts

[bugzilla at redhat.com]

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 at bachelot.org> 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.