[Bug 469931] Review Request: ipmiutil - IPMI Management Utilities

[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=469931

Hans de Goede <hdegoede at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
             Blocks|177841(FE-NEEDSPONSOR)      |
         AssignedTo|nobody at fedoraproject.org    |hdegoede at redhat.com
               Flag|needinfo+, needinfo?        |fedora-review?

--- Comment #35 from Hans de Goede <hdegoede at redhat.com> 2010-07-12 08:47:28 EDT ---
Hi Andy,

Here is a full review of this package.

- rpmlint checks return:
 <not run build fails>
- package meets naming guidelines
- package meets packaging guidelines
- license (BSD) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
  FAIL, see below
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
  MUST FIX: %{_datadir}/%{name} is not owned. To fix this either do
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- other remarks:
  I don't like the creation of %{_var}/lib/%{name} from %post, this should
  be done in %install and then added to the file list.
  I would prefer for the extra <space> between the - and the , in %defattr to
  be dropped.

All in all I must say this packages looks good. Except that it does not build,
the initscripts get installed in /etc/init.d but %files looks for them in
/etc/rc.d/init.d (which is the correct location, on Fedora /etc/init.d is a
compatibility symlink to /etc/rc.d/init.d .

Summarizing, the following must be fixed:
1. Does not build
2. %{_datadir}/%{name}
3. Creation of %{_var}/lib/%{name} from %post

And the following could be fixed:
1. The extra <space> between the - and the , in %defattr


Normally when we get a new packager for Fedora we require him/her to do a
number of package reviews to show understanding of the guidelines. But given
that you are upstream for ipmiutils which is sort of special and that the
current spec file looks quite good I'm willing to sponsor you once the
remaining issues are solved. I've one special request though, if you ever
decide to submit another package (please do), then please let me know and I'll
look over your shoulder (for the first 1 or 2 other packages).

Regards,

Hans

-- 
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.