[Bug 431277] Review Request: ocfs2-tools - programs for managing Ocfs2 file systems

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

Summary: Review Request: ocfs2-tools - programs for managing Ocfs2 file systems


https://bugzilla.redhat.com/show_bug.cgi?id=431277


jwilson at redhat.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|nobody at fedoraproject.org    |jwilson at redhat.com
             Status|NEW                         |ASSIGNED




------- Additional Comments From jwilson at redhat.com  2008-02-20 15:39 EST -------
Eric asked me to take a look over the package as well, and I do have the power
to sponsor... Well, first, a few things w/the spec:

1) Rather than the compile_console define, I'd suggest:

%define with_console %{?_without_console: 0} %{?!_without_console: 1}

With that, you can do 'rpmbuild -bb --without console' to disable building the
console if you want.

2) the dist tag should be '%{?dist}'. The addition of the question mark lets the
package also build if for some reason dist isn't defined.

3) in the Requires/BuildRequires section, its not a mandate, but I personally
prefer to see things wrapped at 80 chars, just use multiple R/BR lines.

4) For anything with an initscript, the chkconfig R should be:

Requires(post): chkconfig
Requires(preun): chkconfig

You should also have this:

Requires(preun): /sbin/service

These are safeguards to make sure these bits are present when called from the
%post and %preun scripts, as they could otherwise be included in the same rpm
transaction, but not available when this  package needs 'em (slim chance of it
happening, but it can). More on the need for /sbin/service in a bit...

5) I'd avoid the extra define for "config_console". I'd just tweak %config based
on the value of %with_console.

6) don't use '-n ocfs2-tools-devel', just use 'devel'. The %{name}- gets
automagically pre-pended.

7) your -devel package includes a .pc file, therefore, the -devel package must
Requires: pkgconfig.

8) drop the '-n ocfs2-tools-%{version}' from the %setup line, that's what it
does by default.

9) the spec should have a comment included as to why _smp_mflags can't be used.

10) CFLAGS and/or CPPFLAGS aren't being respected. This one appears to require
some source patching to get the build to use the standard Fedora flags.

11) no need to redirect anything to /dev/null on chkconfig commands

12) in %preun, before you chkconfig --del the services, you need to make sure
they're stopped first. Thus the need for /sbin/service.

13) %defattr's should be (-,root,root,-)

14) generally, one should use %dir for including a directory in a package, with
a subsequent line or lines covering what files in the dir should be included.


I believe just about everything, save the Fedora CFLAGS/CPPFLAGS not being
respected, should be fixed by the spec diff I'll attach in a second...

-- 
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, or are watching someone who is.