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@xxxxxxxxxx changed: What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@xxxxxxxxxxxxxxxxx |jwilson@xxxxxxxxxx Status|NEW |ASSIGNED ------- Additional Comments From jwilson@xxxxxxxxxx 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review