Spec URL: http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softkn.spec SRPM URL: http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn-3.12.4-1.fc11.src.rpm Description: Network Security Services Cryptographic Module
nss-softkn is one of two packages, nss-util being the other, that are being proposed as spin-off of the full NSS package. nss-softkn requires nss-util and they in turn will be required by nss. Please hold-off reviewing them until I fix some problems.
(In reply to comment #0) Changed to source rpm to SRPM URL: http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn-3.12.3.99.3-1.fc11.src.rpm
Given that it is for f12 this one would be more appropriate http://fedorapeople.org/~emaldonado/nss-util/devel/nss-softokn-3.12.3.99.3-1.fc12.src.rpm
err. I believe Elio meant: SRPM URL: http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn-3.12.3.99.3-1.fc12.src.rpm bob
Taking this review at Bob's request. First, the source is just listed as a tarball, with no URL to verify where it came from. Also since the tarball has "stripped" in the name, I'm assuming that some work is being done to remove content. How that's done needs to be listed in the spec for verification purposes. There are a lot of interesting things being done in the spec with very little comments to explain what's going on. I'd suggest being a bit more verbose. When using install, it's preferred to preserve the timestamp with -p. Also, calling mkdir to make dirs, and then install to install files seems odd, when install can just as easily make dirs too. You're installing something into the prelink.conf.d/ dir but not Requiring prelink as far as I can tell. That needs to be fixed. Your %files section has you taking ownership of the prelink.conf.d directory. prelink and only prelink should own that. https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership Is there a particular reason why you use %dir for %{_includedir}/nss3 and then list a ton of files in it? Are there files that you don't want to package that wind up in that directory? I can't build this to test, because I'm missing nssutil-devel >= 3.12.3.99.3
Created attachment 356396 [details] shell script to split softokn and util out of the full nss tar ball The source tar balls for nss-softokn and nss-util are created from the full tar ball for nss via this script.
(In reply to comment #5) Yes, the source tarball has "stripped" in the name because some some work was done to remove non-free content. Attachment (id=356396) shoes how some of the processing is done. I'll add more comments to the both spec files. This spec file was derived from the big nss.spec file and I tried not to deviate too much from what it was already there and working. We do the install because there is no install target in the upstream makefile. Need to check with Kai about this. > You're installing something into the prelink.conf.d/ dir but not Requiring > prelink as far as I can tell. That needs to be fixed. Will do in the next iteration. > > Your %files section has you taking ownership of the prelink.conf.d directory. > prelink and only prelink should own that. > https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership Will respect ownership. > Is there a particular reason why you use %dir for %{_includedir}/nss3 and then > list a ton of files in it? Are there files that you don't want to package that > wind up in that directory? For backward compatibilty with the previous usage the %{_includedir}/nss3 directory is common to nss-util, nss-softkn, and nss. Nss-softkn doesn't populate the directory completely, nss-util would have placed files ther earlier and nss would do likewise later. I was thinking of having nss-util-devel own that directory so that the other two do not clobber it. Not sure whether I'm aswering your question adequately here. > I can't build this to test, because I'm missing nssutil-devel >= 3.12.3.99.3 Yes , we need to somehow chain the build of nss-softkn with that of nss-util on which it depends. I had problems using mosk but I was able to do rpmbuild -bi on each when in the same rpmb building area.
re nss-util, my bad. It also needs review. this bug is marked as a dependancy on the nss-util bug.
Additionally, the discussion in Bug 508479 is worth reading as it explains the rationale for this work.
(In reply to comment #7) > > > You're installing something into the prelink.conf.d/ dir but not Requiring > > prelink as far as I can tell. That needs to be fixed. > Will do in the next iteration. I object. NSS does not need prelink. Requiring prelink is wrong. But NSS needs to make sure that prelink won't touch certain files from NSS, therefore it needs to prepare for a potential installation of prelink.
(In reply to comment #7) > This spec > file was derived from the big nss.spec file and I tried not to deviate too > much from what it was already there and working. We do the install because > there is no install target in the upstream makefile. Elio, I believe Jesse doesn't refer to "make install" but rather to %{__mkdir_p} name_of_directory %{__install} ... file destination I think Jesse claims that "mkdir" is unnecessary, because "install" will create the directory automatically.
> I can't build this to test, because I'm missing nssutil-devel >= 3.12.3.99.3 This bug requests new package nss-softokn. Another bug requests new package nss-util. Perhaps the base package nss-util should be produced/reviewed/completed first.
Actually I meant that if you're going to use install to install the file, might as well use install to install the directory too. It's still two calls, but it's calls of the same application, and there is no need to macro it out. As far as the prelink issue, you may want to set that up as a trigger for if/when prelink is installed, and ghost the file. That way you don't wind up owning the prelink dir.
(In reply to comment #13) > As far as the prelink issue, you may want to set that up as a trigger for > if/when prelink is installed, and ghost the file. That way you don't wind up > owning the prelink dir. That sounds tricky. Do you remember a similar solution in another rpm package, we could look at as a reference? But I guess you refer to http://rpm.org/api/4.4.2.2/triggers.html and that may answer my question for an example already.
Yes, that's what I was talking about. It is fairly tricky, but it accomplishes what you wish.
Created attachment 356903 [details] New spec file This is update spec file. The same one that I have updated at http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn.spec and a nwe srpm built from it is at http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn-3.12.3.99.3-8.fc11.test.1.src.rpm It addresses most of the issues raised in the earlier review. Notice that: - Source3 is the script that makes the nss-softokn source tar ball out of the big nss source tar ball. - I have to save a couple of files to a temp location during build and restore them during install because now the buildroot gets removed at install whether we do it or not. (That had caused the f12 mass rebuild to fail for NSS) - I haven't implemented yet the suggestion of using Triggers.
(In reply to comment #12) > Perhaps the base package nss-util should be produced/reviewed/completed first. I agree with Kai, we should handle with Bug 515032 first. Having nss-util dealt with first will make this one a lot easier to handle.
Bob, nss-softokn is ready for review, I have updated the files at Spec URL: http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softkn.spec and SRPM URL: http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn-3.12.3.99.3-8.fc12.test.1.src.rpm
RPMLINT: rpmlint nss-softokn.spec nss-softokn.spec:171: W: rpm-buildroot-usage %build $RPM_BUILD_ROOT/%{unsupported_tools_directory}/shlibsign -i $RPM_BUILD_ROOT/%{_lib}/libsoftokn3.so \ nss-softokn.spec:172: W: rpm-buildroot-usage %build $RPM_BUILD_ROOT/%{unsupported_tools_directory}/shlibsign -i $RPM_BUILD_ROOT/%{_lib}/libfreebl3.so \ 0 packages and 1 specfiles checked; 0 errors, 2 warnings. NEEDSWORK: Some minor issues... 1. Remove the .test.1 from the version string. For test builds, you can use ~/.rpmmacros to set dist to your own test string (f12.elio.test for instance). 2. in the comment on getting the source you describe cvs nss-util, I think you mean cvs co nss-util. Also the directory name can be confused with the nss-util package perhaps the name nss-package-tools would be better. 3. A comment that the 'special install-post command actually gets executed as the last stip in the %install (so that the code operates on the stripped libraries). Also, I believe the %define needs to be a %global. bob
(In reply to comment #19) Moving the post unstall macro to the top fixes the rpmlint warning rpmlint nss-softokn.spec gives 0 packages and 1 specfiles checked; 0 errors, 0 warnings. Updated nss-softokn.spec file and made a new source rpm, this latter at http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn-3.12.3.99.3-8.fc12..src.rpm
NEEDSWORK. 1. I don't think you can use %{SOURCE4} in the trigger as the latter is part of the .srpm. You probably need to copy the file to an NSS private area and install it out of there. (verify this, if %{SOURCE4} is handled correctly, then I think it would be preferable. 2. You still have stuff from the examples that don't apply in your triggers. bob
(In reply to comment #21) Confirmed, %{SOURCE4} cannot be used in a trigger. Sections like %build and %install execute in the development/build machine when we are building the RPM's whereas sections like %pre, %post, %preun, %postun and %trigger{in|un|postun} execute in the user's system where the install, update, and uninstall occurs and where files that the %{SOURCEn} macros refer to do not exist. I will save the file in the unsupported tools directory and restore it from there when needed in a trigger. I'm fixing those "copy and paste" errors on triggers and others and will also install libnssdbm3.so as part of nss-softoken.
Bob, I updated the files at http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softkn.spec and http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn-3.12.3.99.3-8.fc12.test.1.src.rpm for your review
I've grabbed: SPEC: http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn.spec and SRPM: http://fedorapeople.org/~emaldonado/nss-softokn/devel/nss-softokn-3.12.3.99.3-8.fc12.src.rpm Are these the correct versions (the links given are bad).
NEEDS work: 1) the new directory should be called nss3 to avoid conflict with name switch service. 2) libnssdbm3 needs to be added in to other places: a) .chk for look in %install b) post-install define at the top 3) need to uncomment libnssdbm3.chk
(In reply to comment #25) > NEEDS work: > > 1) the new directory should be called nss3 to avoid conflict with name switch > service. I'm saving underneath the existing /usr/lib/nss under which we also keep the unsupported-tools. > > 2) libnssdbm3 needs to be added in to other places: > a) .chk for look in %install > b) post-install define at the top Added it to a) and b) > > 3) need to uncomment libnssdbm3.chk Done. Updated the spec and srpm.
ok, that looks good. bob
Thanks. I guess we should reassign this Bug to Jessie Keating for the next step in the process.
I don't think I'm needed here at all, since Elio is already sponsored. Just put in your CVS request stanza and build away!
New Package CVS Request ======================= Package Name: nss-softokn Short Description: Network Security Services Cryptographic Module Owners: emaldonado, kengert, rrelyea Branches: F-12 InitialCC: emaldonado, kengert, rrelyea
We cannot create F-12 branches at this time. Otherwise, CVS done.