[Bug 196146] Review Request: mod_nss
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/show_bug.cgi?id=196146 [EMAIL PROTECTED] changed: What|Removed |Added Severity|normal |medium Priority|normal |medium Product|Fedora Extras |Fedora Version|devel |rawhide -- Configure bugmail: https://bugzilla.redhat.com/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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] changed: What|Removed |Added OtherBugsDependingO||200958 nThis|| -- 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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] changed: What|Removed |Added Status|ASSIGNED|CLOSED Resolution||NEXTRELEASE --- Additional Comments From [EMAIL PROTECTED] 2006-07-20 10:09 EST --- Thanks -- 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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] 2006-07-17 11:08 EST --- 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. :) -- 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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] 2006-07-17 12:43 EST ---
* 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...
--
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@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] changed: What|Removed |Added AssignedTo|[EMAIL PROTECTED] |[EMAIL PROTECTED] --- Additional Comments From [EMAIL PROTECTED] 2006-07-17 13:35 EST --- Rob, I'll sponsor you. Go ahead and apply for your cvsextras membership while I double-check the above review. -- 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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] 2006-07-17 14:53 EST --- 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. -- 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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] 2006-07-17 15:34 EST --- 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. -- 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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] changed: What|Removed |Added OtherBugsDependingO|163778 |163779 nThis|| --- Additional Comments From [EMAIL PROTECTED] 2006-07-17 20:35 EST --- 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 -- 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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] 2006-07-14 14:01 EST --- 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. -- 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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] 2006-07-14 16:21 EST ---
(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 21
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_nsswarning: /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%]
--
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@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] 2006-07-14 16:53 EST --- 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. -- 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@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 196146] Review Request: mod_nss
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 [EMAIL PROTECTED] 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.0 -0400
+++ mod_nss.spec2006-07-13 19:12:45.0 -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
