I don't have sponsor status, but can do the review and then kick someone who
does have sponsor status when the package is approved.

Based on my initial tour through the spec and rpmlint'ing:

1) Source: should be a URL if possible:
http://directory.fedora.redhat.com/sources/%{name}-%{version}.tar.gz

2) BuildRequires: for make and perl should both be removed, per
http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions

3) Please explain why "AutoReq: 0" is necessary

4) Quotes around $RPM_OPT_FLAGS are recommended

5) Use %configure instead of ./configure

6) Should be smp_mflags instead of smp_flags

7) I believe DESTDIR is only relevant for make install (rpmlint complains).

8) Use macros for things like /etc (%{_sysconfdir}), /usr/sbin (%{_sbindir}), etc.

9) Watch out for lib/lib64 issues, you have a hard-coded /usr/lib in there,
*must* be %{_libdir} so that its properly set to /usr/lib64 on 64-bit platforms.

10) don't use install -s, let rpm do the stripping so we get a valid debuginfo
package.

11) don't create directories in %post, create them in the package, or they
aren't owned by the package, which violates FE packaging policy

12) the %ifarch stuff around copying libnssckbi.so should be removed, this is
another case where %{_libdir} is your friend. However, copying a file into place
in %post is also a no-no. Just duplicate it within the package if you absolutely
must, otherwise the file isn't owned by the package.

13) Oy. Just realized the file being copied in %post is from another package.
That's also a no-no. I think a relative symlink (while still ugly) is okay
though, and at least it would be owned by the package.

14) secmod.db (and cert8.db, key3.db) can be created in the build and marked
%config(noreplace) instead of creating unowned files in %post. Note that this
does add a BuildRequires: on nss-tools though. Of course, it also means the
buildhost name winds up in the file instead of the target host. This one may
need some more thought... I suppose doing it the way you have it may be best,
given that mod_ssl does essentially the same. Ah! Just had an idea... I think
creating dummy files in the buildroot and including them in the package itself
and then creating actual files in %post may be the sanest thing.

15) Need to add to %files some to account for the above (and switch to using macros)

Okay, all of the above are addressed (I believe) by the following spec diff,
including the tweak to let mod_nss own the .db files:

----------
--- mod_nss-orig.spec   2006-07-13 17:38:26.000000000 -0400
+++ mod_nss.spec        2006-07-13 19:12:45.000000000 -0400
@@ -1,13 +1,12 @@
 Name: mod_nss
 Version: 1.0.3
-Release: 1
+Release: 1%{?dist}
 Summary: SSL/TLS module for the Apache HTTP server
 Group: System Environment/Daemons
 License: Apache Software License
 URL: http://directory.fedora.redhat.com/wiki/Mod_nss
-Source: %{name}-%{version}.tar.gz
+Source: http://directory.fedora.redhat.com/sources/%{name}-%{version}.tar.gz
 BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
-BuildRequires: make, perl
 BuildRequires: nspr-devel >= 4.6, nss-devel >= 3.11
 BuildRequires: httpd-devel >= 0:2.0.52, apr-devel, apr-util-devel
 Requires: httpd >= 0:2.0.52
@@ -36,7 +35,7 @@
 # regenerate configure scripts
 autoconf || exit 1

-CFLAGS=$RPM_OPT_FLAGS
+CFLAGS="$RPM_OPT_FLAGS"
 export CFLAGS

 NSPR_INCLUDE_DIR=`/usr/bin/pkg-config --variable=includedir nspr`
@@ -47,24 +46,35 @@

 NSS_BIN=`/usr/bin/pkg-config --variable=exec_prefix nss`

-./configure --with-nss-lib=$NSS_LIB_DIR --with-nss-inc=$NSS_INCLUDE_DIR
--with-nspr-lib=$NSPR_LIB_DIR --with-nspr-inc=$NSPR_INCLUDE_DIR
--with-apr-config --enable-ecc
+%configure \
+    --with-nss-lib=$NSS_LIB_DIR \
+    --with-nss-inc=$NSS_INCLUDE_DIR \
+    --with-nspr-lib=$NSPR_LIB_DIR \
+    --with-nspr-inc=$NSPR_INCLUDE_DIR \
+    --with-apr-config --enable-ecc
+#./configure --with-nss-lib=$NSS_LIB_DIR --with-nss-inc=$NSS_INCLUDE_DIR
--with-nspr-lib=$NSPR_LIB_DIR --with-nspr-inc=$NSPR_INCLUDE_DIR
--with-apr-config --enable-ecc

 make %{?_smp_flags} DESTDIR=$RPM_BUILD_ROOT all

 %install
 rm -rf $RPM_BUILD_ROOT

-mkdir -p $RPM_BUILD_ROOT/etc/httpd/conf
-mkdir -p $RPM_BUILD_ROOT/etc/httpd/conf.d
-mkdir -p $RPM_BUILD_ROOT/usr/lib/httpd/modules
-mkdir -p $RPM_BUILD_ROOT/usr/sbin
-
-install -m 644 nss.conf $RPM_BUILD_ROOT/etc/httpd/conf.d
-install -s -m 755 .libs/libmodnss.so $RPM_BUILD_ROOT/usr/lib/httpd/modules
-install -s -m 755 nss_pcache $RPM_BUILD_ROOT/usr/sbin
-install -m 755 gencert $RPM_BUILD_ROOT/usr/sbin
+mkdir -p $RPM_BUILD_ROOT%{_sysconfdir}/httpd/conf
+mkdir -p $RPM_BUILD_ROOT%{_sysconfdir}/httpd/conf.d
+mkdir -p $RPM_BUILD_ROOT%{_libdir}/httpd/modules
+mkdir -p $RPM_BUILD_ROOT%{_sbindir}
+mkdir -p $RPM_BUILD_ROOT%{_sysconfdir}/httpd/alias
+
+install -m 644 nss.conf $RPM_BUILD_ROOT%{_sysconfdir}/httpd/conf.d/
+install -m 755 .libs/libmodnss.so $RPM_BUILD_ROOT%{_libdir}/httpd/modules/
+install -m 755 nss_pcache $RPM_BUILD_ROOT%{_sbindir}/
+install -m 755 gencert $RPM_BUILD_ROOT%{_sbindir}/
+ln -s ../../..%{_libdir}/libnssckbi.so $RPM_BUILD_ROOT%{_sysconfdir}/httpd/alias/
+echo "temporary file" > $RPM_BUILD_ROOT%{_sysconfdir}/httpd/alias/secmod.db
+echo "temporary file" > $RPM_BUILD_ROOT%{_sysconfdir}/httpd/alias/cert8.db
+echo "temporary file" > $RPM_BUILD_ROOT%{_sysconfdir}/httpd/alias/key3.db

-perl -pi -e "s:$NSS_LIB_DIR:$NSS_BIN:" $RPM_BUILD_ROOT/usr/sbin/gencert
+perl -pi -e "s:$NSS_LIB_DIR:$NSS_BIN:" $RPM_BUILD_ROOT%{_sbindir}/gencert

 %clean
 rm -rf $RPM_BUILD_ROOT
@@ -72,36 +82,35 @@
 %post
 umask 077

-if [ $1 -eq 1 ]; then
-    if [ ! -d /etc/httpd/alias ] ; then
-        mkdir /etc/httpd/alias
-    fi
-
-    if [ ! -f /etc/httpd/alias/secmod.db ] ; then
-        /usr/sbin/gencert /etc/httpd/alias > /etc/httpd/alias/install.log 2>&1
-        echo ""
-        echo "Certificate database generated."
-        echo ""
-    fi
+if [ "$1" -eq 1 ] ; then
+    for file in %{_sysconfdir}/httpd/alias/{secmod,cert8,key3}.db
+    do
+        # Just to be safe...
+        if [ `grep -c "temporary file" $file` -eq 1 ]; then
+            rm -f $file
+        else
+            mv $file $file.rpmsave
+            echo "%{_sysconfdir}/httpd/alias/$file saved as
%{_sysconfdir}/httpd/alias/$file.rpmsave"
+        fi
+    done
+    %{_sbindir}/gencert %{_sysconfdir}/httpd/alias >
%{_sysconfdir}/httpd/alias/install.log 2>&1
+    echo ""
+    echo "%{name} certificate database generated."
+    echo ""
 fi

-# copy the root certificate library to our database location
-%ifarch x86_64
-cp -p /usr/lib64/libnssckbi.so /etc/httpd/alias
-%else
-cp -p /usr/lib/libnssckbi.so /etc/httpd/alias
-%endif
-
 %files
 %defattr(-,root,root,-)
-
 %doc README LICENSE docs/mod_nss.html
-
-%config(noreplace) /etc/httpd/conf.d/nss.conf
-
-/usr/lib/httpd/modules/libmodnss.so
-/usr/sbin/nss_pcache
-/usr/sbin/gencert
+%config(noreplace) %{_sysconfdir}/httpd/conf.d/nss.conf
+%config(noreplace) %{_sysconfdir}/httpd/alias/secmod.db
+%config(noreplace) %{_sysconfdir}/httpd/alias/cert8.db
+%config(noreplace) %{_sysconfdir}/httpd/alias/key3.db
+%{_libdir}/httpd/modules/libmodnss.so
+%dir %{_sysconfdir}/httpd/alias/
+%{_sysconfdir}/httpd/alias/libnssckbi.so
+%{_sbindir}/nss_pcache
+%{_sbindir}/gencert

 %changelog
 * Tue Jun 20 2006 Rob Crittenden <rcritten> 1.0.3-1
----------

This leaves only two warnings from rpmlint:
W: mod_nss dangling-relative-symlink /etc/httpd/alias/libnssckbi.so
../../../usr/lib64/libnssckbi.so
W: mod_nss dangerous-command-in-%post rm

I think we'll probably have to live with those though.

However, I just now noticed that the Makefile actually contains an install
target. Why isn't it being used here?

Time to go home, more tomorrow, or after you finish digesting all this... :)

Thanks for the thorough review!

The spec and SRPM files are updated.

Here are specific responses:

1. Done, added source url
2. Removed extraneous BuildRequires (left over from the httpd spec file I
initially used as a template)
3. AutoReq isn't needed, removed
4. Added quotes around $RPM_OPT_FLAGS
5. Using %configure over ./configure
6. Switched to smp_mflags
7. $DESTDIR removed fro make
8. Using appropriate macros
9. Yes, I was worried about the lib64 thing, macros make things much easier
10. Ok, removed -s from install.
11. Yes, better to have RPM own the alias dir
12. using macros instead to resolve issue
13. Using symlink as recommended 
14. Keeping cert8.db, key3.db and secmod.db is desired when the RPM is removed.
These files contain any certificates that were issued and any private key
material. One should have to be very conscious when removing these files as
certificates can be an expensive proposition.
15. Added %dir for alias directory

The only warning I see now is the dangling symlink.

Because this depends on the NSS package this file will always exist, so I think
it's relatively safe.

While it's true that mod_nss has an install target it is cleaner to do it by
hand. We are only installing a couple of files anyway. If it really causes
heartburn I can see about switching to that but most Apache modules are
installed by hand this way.

Additional review, based on new spec and details in comment #13:

1) The switch to %configure actually eliminates the need to pass --libdir,
--sbindir and --sysconfdir (which based on ./configure --help, should actually
be /etc, not /etc/httpd/conf.d). The %configure macro sets those automatically.
So those three lines after %configure should be removed.

2) Going back to item 14 in comment #12 and #13... The 'create dummy files then
replace in %post' trick I threw together actually does leave the files around
when the RPM is removed, %config(noreplace) makes that happen. They're renamed
w/an appended .rpmsave if the package is removed.

# rpm -e mod_nss
warning: /etc/httpd/alias/secmod.db saved as /etc/httpd/alias/secmod.db.rpmsave
warning: /etc/httpd/alias/key3.db saved as /etc/httpd/alias/key3.db.rpmsave
warning: /etc/httpd/alias/cert8.db saved as /etc/httpd/alias/cert8.db.rpmsave

So this route leaves us not deleting the files, addressing your (very valid)
concern and also makes them owned by the package. Win-win, no? :)

3) For consistency, you shouldn't have spaces between lines within a single
%files section, I'd cut the extra lines after %defattr and %doc.

4) I poked around at the Makefile a bit to see if using make install was
feasible. The main issue appears to be that 'make install' uses apxs to put the
module in place, but apxs tries to be too smart for its own good, and install
and activate the module in the buildhost's httpd, rather than in the buildroot.
Would require a bit of Makefile hacking to get a viable 'make install', so
putting the bits in place by hand is understandable and acceptable.

The spec and SRPM files are updated.

1. Bah, my mistake. I was experimenting with using the make install target and
accidentally left that in. Removed.

2. Ok, you've convinved me. I made a few minor changes though. When determining
if we need to generate a database we only need to check one of the files. They
do not stand alone but work together in concert, so if one is temporary it is
safe to assume they all are. I switched to checking key3.db since that is the
most important file.

I also modified the deletion install test. I'm using if [ "$1" -eq 0 ] which
from the RPM docs means "Remove last version of package". I tested it and it
seems to work ok for me.

3. Removed the extra lines. Must be a style thing.

4. Great news.

(In reply to comment #5)
> 2. Ok, you've convinved me. I made a few minor changes though. When determining
> if we need to generate a database we only need to check one of the files. They
> do not stand alone but work together in concert, so if one is temporary it is
> safe to assume they all are. I switched to checking key3.db since that is the
> most important file.
> 
> I also modified the deletion install test. I'm using if [ "$1" -eq 0 ] which
> from the RPM docs means "Remove last version of package". I tested it and it
> seems to work ok for me.

Heh, actually, no it doesn't. :)

$1 being 0 isn't possible in the %post context, only %postun and %preun, see
"Running scriptlets only in certain situations" at
http://fedoraproject.org/wiki/Packaging/Guidelines. RPM itself does the work
that you're seeing, renaming them with .rpmsave. I added that extra little bit
for the edge case where the user has no mod_nss rpm installed, but does have the
.db files there, then installs the rpm, so they wouldn't be overwritten by
%post. It probably makes more sense to do nothing at all in %post on install if
we find .db files that don't have that temp string in them -- then we just end
up with .rpmnew files and the existing .db files. Now that I ponder it, I think
this makes the most sense for that section:

umask 077
if [ "$1" -eq 1 ] ; then
    if [ `grep -c "temporary file" %{_sysconfdir}/httpd/alias/key3.db` -eq 1 ]; then
        rm -f %{_sysconfdir}/httpd/alias/{secmod,cert8,key3}.db
        %{_sbindir}/gencert %{_sysconfdir}/httpd/alias >
%{_sysconfdir}/httpd/alias/install.log 2>&1
        echo ""
        echo "%{name} certificate database generated."
        echo ""
    fi
fi

Results on install with this tweak:

rpm -ivh /build/RPMS/x86_64/mod_nss-1.0.3-1.fc5.x86_64.rpm
Preparing...                ########################################### [100%]
   1:mod_nss                warning: /etc/httpd/alias/cert8.db created as
/etc/httpd/alias/cert8.db.rpmnew
warning: /etc/httpd/alias/key3.db created as /etc/httpd/alias/key3.db.rpmnew
warning: /etc/httpd/alias/secmod.db created as /etc/httpd/alias/secmod.db.rpmnew
########################################### [100%]

Files updated.

Ok, this method works for me. I think that the only time the rpmnew files would
be if someone went in and created a database BEFORE installing mod_nss for the
first time. I consider this fairly unlikely. And even so, there will be no data
loss.

Just moved on to trying to build in a mock chroot and got an error about
autoconf not being found, so the addition of "BuildRequires: autoconf" looks to
be necessary. Could have sworn that would always be present, but its not in the
Exceptions list... Trivial addition though.

One thing I forgot to mention that might be worth adding is a little
documentation just after %install, explaining why 'make install' isn't used.

I'll post the full details of a full review checklist once completed. Gimme one
more rev, and I think we're golden. :) 

Files updated.

Added BuildRequires: autoconf
Added some notes about 'make install' and purpose of 'temporary file' text in
key3.db, cert8.db and secmod.db

* package meets naming and packaging guidelines
* specfile is properly named, is cleanly written and uses macros consistently
* dist tag is present
* build root is correct
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
* license field matches the actual license: Apache
* license is open source-compatible, license text included in package
* source files match upstream:
      feb2d314983a72318cc08e0650501fac  mod_nss-1.0.3.tar.gz
* latest version is being packaged

* BuildRequires are proper:
  nspr-devel >= 4.6, nss-devel >= 3.11
  httpd-devel >= 0:2.0.52, apr-devel, apr-util-devel
  autoconf

Technically, the apr-devel BR could be left off, since apr-util-devel Requires:
apr-devel. Similarly, nspr-devel could be left off, as nss-devel Requires:
nspr-devel >= 4.6 already. Ah, one could get even cleaner: httpd-devel Requires:
apr-devel and apr-util-devel. So you could reduce BuildRequires: down to just:

  nss-devel >= 3.11, httpd-devel >= 0:2.0.52, autoconf

Up to you whether you want to do that or not though.

* package builds in mock (FC6/x86_64).

* rpmlint is (mostly) silent
W: mod_nss dangling-relative-symlink /etc/httpd/alias/libnssckbi.so
../../../usr/lib64/libnssckbi.so
-Not pretty, but better than copying the file over from another package, would
be optimal to configure mod_nss to simply look for the .so in /usr/lib(64)

W: mod_nss dangerous-command-in-%post rm
-We're safeguarding that rather tightly, necessary for proper cert creation, no
worries here

* final provides and requires are sane:
  config(mod_nss) = 1.0.3-1.fc6
  libmodnss.so()(64bit)
  mod_nss = 1.0.3-1.fc6
  =
  config(mod_nss) = 1.0.3-1.fc6
  httpd >= 0:2.0.52
  libnspr4.so()(64bit)
  libnss3.so()(64bit)
  libnss3.so(NSS_3.10.2)(64bit)
  libnss3.so(NSS_3.2)(64bit)
  libnss3.so(NSS_3.3)(64bit)
  libnss3.so(NSS_3.4)(64bit)
  libnss3.so(NSS_3.5)(64bit)
  libnss3.so(NSS_3.6)(64bit)
  libplc4.so()(64bit)
  libplds4.so()(64bit)
  libsmime3.so()(64bit)
  libsoftokn3.so()(64bit)
  libssl3.so()(64bit)
  libssl3.so(NSS_3.2)(64bit)
  libssl3.so(NSS_3.4)(64bit)
  libssl3.so(NSS_3.7.4)(64bit)
  nspr >= 4.6
  nss >= 3.11
  nss-tools >= 3.11

* no shared libraries are present
* package is not relocatable
* owns the directories it creates
* doesn't own any directories it shouldn't
* no duplicates in %files
* file permissions are appropriate
* %clean is present
* %check is present and all tests pass: n/a
* scriptlets are sane
* code, not content
* documentation is small, so no -docs subpackage is necessary
* %docs are not necessary for the proper functioning of the package
* no headers
* no pkgconfig files
* no libtool .la files lingering about
* not a GUI app
* not a web app


Package APPROVED, I'll ping someone about sponsorship...

Rob, I'll sponsor you.  Go ahead and apply for your cvsextras membership while I
double-check the above review.

Just a couple of questions before I can ACK this:

Rawhide is busted at the moment, but building on FC5 fails due to a missing
BuildRequires: pkgconfig.  This is probably a missing dependency in one of the
-devel packages you require.  For instance, nspr-devel has a .pc file but does
not require pkgconfig, a clear bug.  Anyway, to build on FC5 at least you'll
need to add that BR.

Why do you need to call autoconf?  Generally this is avoided if possible.  For
grins I removed it from the spec and things still built OK, although there's
always the possibility that something broke.

Files updated.

Added pkgconfig
Removed autoconf

Autoconf was a leftover from the httpd spec file I used as a template. It seemed
a bit paranoid but wasn't problematic. Should be fine, and build faster, without
it. It's gone.

I'll notify the nspr and nss maintainer(s) regarding pkgconfig.

How odd; I thought I had committed another comment to this ticket.  Oh, well.

The changes you made look good to me.

I went approve your cvsextras membership but saw that Warren had already done
it, which is odd since he hasn't made any comments on this ticket.  I don't
think it would be productive waiting for him to ACK this since that isn't likely
to happen, so I say we're ready to go.

By the way, in the future, please do bump the version on your package when you
make changes.  Otherwise it can be difficult for the reviewers to know if they
have the most current version, and browser caching can make things even more
difficult.

APPROVED

Thanks