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