[Bug 196146] Review Request: mod_nss

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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: mod_nss


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





------- Additional Comments From jwilson@xxxxxxxxxx  2006-07-13 19:15 EST -------
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@xxxxxxxxxx> 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... :)

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.

_______________________________________________
Fedora-package-review mailing list
Fedora-package-review@xxxxxxxxxx
http://www.redhat.com/mailman/listinfo/fedora-package-review

[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]