Spec URL: https://raw.githubusercontent.com/garlsecurity/nss_securepass/master/nss_securepass.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/5370/8065370/nss-securepass-0.1-1.src.rpm Description: NSS (Name Service Switch) module for SecurePass SecurePass provides identity management and web single sign-on. Fedora Account System Username: gpaterno Note: It builds fine in koji for rawhide and epel6 and epel7 Also Debian and Ubuntu included this module recently in their core. Thanks.
Why not using %{buildroot} macro instead $RPM_BUILD_ROOT env variable !?
Next update I will change the spec with %{buildroot}. The $RPM_BUILD_ROOT is still valid and builds fine in koji with all releases and platforms.
Updated the package at 0.2 UID lookup fixed in sources Buildroot macro have been used instead of $RPM_BUILD_ROOT New SRPM here: https://kojipkgs.fedoraproject.org//work/tasks/6937/8146937/nss-securepass-0.2-1.src.rpm Spec file location is unchanged
Location for SPEC and SRPM have changed: SPEC: http://pubs.gpaterno.com/external/nss_securepass.spec SRPM: http://pubs.gpaterno.com/external/nss-securepass-0.2-1.el6.src.rpm Thanks.
This is un-official review of the package Couple of points from me: * spec file name doesn't match with srpm name. spec file name should be nss-securepass.spec instead of nss_securepass.spec http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Spec_file_name * Running rpmlint on source package also gives warning "nss-securepass.src: W: summary-ended-with-dot C NSS library for SecurePass." To avoid it, remove trailing period(.) from summary. http://fedoraproject.org/wiki/Common_Rpmlint_issues#summary-ended-with-dot * Instead of using ./configure using macro %configure to avoid warning "nss_securepass.spec:26: W: configure-without-libdir-spec " * %license macro should be used instead of %doc for License files https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text %license LICENSE LICENSE_APACHE2 LICENSE_GNUGPL LICENSE_MIT * Instead of using %attr(0755,root,root) /usr/%{_lib}/*.so* try using %{_libdir} macro https://fedoraproject.org/wiki/Packaging:RPMMacros?rd=Packaging/RPMMacros * Spec file contains instruction to install shared libraries files. %attr(0755,root,root) /usr/%{_lib}/*.so* So, you need to add %post -p /sbin/ldconfig %postun -p /sbin/ldconfig http://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries * Changelog should be as per Fedora guideline http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
Dear Sinny, thanks for the review, probably the best one I had up to now. I hope I have corrected most of the issues you mentioned. The only think I had to create a workaround is about license. As my concern is to build also on EPEL for RHEL6, I needed to create a macro to use %doc for RHEL6 and %license for other more recent releases. %license without bugs is only from RPM 4.11 AFAIK. These are the updated files, I'd appreciate if you can cross-check: SPEC: http://pubs.gpaterno.com/external/nss-securepass.spec SRPM: http://pubs.gpaterno.com/external/nss-securepass-0.2-2.el6.src.rpm Thanks, Giuseppe
(In reply to Giuseppe Paterno' from comment #6) > Dear Sinny, > thanks for the review, probably the best one I had up to now. > I hope I have corrected most of the issues you mentioned. Yes, you have made changes for most of issues pointed out by me. Few more feedback: * For Changelog I have seen using angle bracket instead of parenthesis for specifying email address http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs . Should be Wed Jan 28 2015 Giuseppe Paterno' <gpaterno> 0.2-2 * It would be good and consistent too if you update source tar file name to nss-securepass-%{commit}.tar.gz from nss_securepass-%%{commit}.tar.gzin order to keep consistent with spec and srpm file Rest I don't see any major issue. It would be good if fedora packager can review it further. Thanks
I've updated parenthesis on the mail address. Unfortunately in order to change the source code tarball, we need to modify the github name of the project and we need to hold on for that as also debian and ubuntu are using this project. I hope that won't be a show-stopper for this to be into EPEL and Fedora. Thanks.
(In reply to Giuseppe Paterno' from comment #8) > I've updated parenthesis on the mail address. > Unfortunately in order to change the source code tarball, we need to modify > the github name of the project and we need to hold on for that as also > debian and ubuntu are using this project. > I hope that won't be a show-stopper for this to be into EPEL and Fedora. Okay, No problem. Someone from Fedora Packager sponsor group will let you know if it is a blocker.
Note %license macro works in all versions of Fedora and in EPEL7 only. For other epel releases don't add this macro in spec file. Giuseppe, I will suggest you to please find some time and go through each point given on http://fedoraproject.org/wiki/Packaging:Guidelines and you will find many changes still needed for your package. I am busy currently but if I will get some time soon I may have a look at your package but in the meantime read guidelines and update your package. No need to rename source tarball name.
SRPM/Spec download links are not working currently: Error 524 Ray ID: 1b0478f441c31497 A timeout occurred Consider requesting fedorapeople upload storage: https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Upload_Your_Package
I asked for space in the fedoraproject. I had a DDoS on the server, now should be up.
New location of SPEC/SRPM, thanks to fedorapeople.org: SPEC: https://gpaterno.fedorapeople.org/nss-securepass.spec SRPM: https://gpaterno.fedorapeople.org/nss-securepass-0.2-2.el6.src.rpm
Reviewed the package guideline again. I put consistent %{buildroot} macro and removed %clean section that is no longer required. Also made use of correct and consistent %{_libdir} macro through spec and adjusted a couple of other minor things. Here's the source. SPEC: https://gpaterno.fedorapeople.org/nss-securepass.spec SRPM: https://gpaterno.fedorapeople.org/nss-securepass-0.2-3.el6.src.rpm Could please re-review it and tell me what are the gaps (if any) to fill?? Thanks.
This is un-official review of the package Complete review of this package using fedora-review tool Rpmlint issues --------------- * Explicit Require of libcurl is not needed https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires * Encoding of file LICENSE_APACHE2 and LICENSE_MIT are wrong. Maybe create it on Linux http://fedoraproject.org/wiki/Common_Rpmlint_issues#wrong-script-end-of-line-encoding * Files LICENSE LICENSE_APACHE2 LICENSE_GNUGPL LICENSE_MIT have execute permission. Execute permission from these files from actual source source tarball needs to be removed. * libnss_sp.so has been packaged with main package. It should go into devel sub-pacakge. Check http://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages Other issues ------------ * spec file shouldn't be added in source tarball. * Does Source tarball is created for a particular commit id or for a released version? If created for a particular commit ID then Release field and Changelog needs to be updated https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages * %defattr is not needed in rpm 4.4 or later version in beginning of %file section https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#File_Permissions * %attr(0755,root,root) %{_libdir}/*.so* No need to specify explicitly %attr(0755,root,root) default value. Keeping %{_libdir}/*.so* should be fine. Also, better to specify explicitly which all shared libraries are getting installed. It will avoid adding unwanted .so files being added. * Don't use BuildRoot tag becasue it will be ignored in Fedora > 10 and EPEL > 5 * make should be used with macro %{?_smp_mflags} Note: Few issues mentioned by me are not valid for EPEL5 or older Fedora version. If you have plan to build your package also for EPEL5 then I think "if" condition for EPEL version needs to be added or such issues mentioned by me can be ignored. Can any existing packagers confirm this? Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files directly in %_libdir. See: http://fedoraproject.org/wiki/Packaging/Guidelines#DevelPackages ===== MUST items ===== C/C++: [x]: ldconfig called in %post and %postun if required. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package do not use a name that already exist [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [!]: Buildroot is not present Note: Invalid buildroot found: %{_tmppath}/%{name}-root See: http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag [!]: Uses parallel make %{?_smp_mflags} macro. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: Reviewer should test that the package builds in mock. [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: Dist tag is present (not strictly required in GL). [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: nss-securepass-0.2-3.fc21.x86_64.rpm nss-securepass-0.2-3.fc21.src.rpm nss-securepass.x86_64: E: explicit-lib-dependency libcurl nss-securepass.x86_64: W: spurious-executable-perm /usr/share/doc/nss-securepass/LICENSE_APACHE2 nss-securepass.x86_64: E: wrong-script-end-of-line-encoding /usr/share/doc/nss-securepass/LICENSE_APACHE2 nss-securepass.x86_64: W: spurious-executable-perm /usr/share/doc/nss-securepass/LICENSE_GNUGPL nss-securepass.x86_64: E: non-readable /etc/securepass.conf 0600L nss-securepass.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libnss_sp.so nss-securepass.x86_64: W: spurious-executable-perm /usr/share/doc/nss-securepass/LICENSE nss-securepass.x86_64: W: spurious-executable-perm /usr/share/doc/nss-securepass/LICENSE_MIT nss-securepass.x86_64: E: wrong-script-end-of-line-encoding /usr/share/doc/nss-securepass/LICENSE_MIT 2 packages and 0 specfiles checked; 4 errors, 5 warnings. Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Requires -------- nss-securepass (rpmlib, GLIBC filtered): /sbin/ldconfig config(nss-securepass) libc.so.6()(64bit) libcurl libcurl.so.4()(64bit) libnss_sp.so.2()(64bit) rtld(GNU_HASH) Provides -------- nss-securepass: config(nss-securepass) libnss_sp.so.2()(64bit) nss-securepass nss-securepass(x86-64) Unversioned so-files -------------------- nss-securepass: /usr/lib64/libnss_sp.so Source checksums ---------------- https://github.com/garlsecurity/nss_securepass/archive/c1bf10da1873bc212caa857653bef0b1e899703a/nss_securepass-c1bf10da1873bc212caa857653bef0b1e899703a.tar.gz : CHECKSUM(SHA256) this package : d5879f3afbad52fa2fa722a2913d5e04373a93ab9b2b1a651a623bfe28d67b36 CHECKSUM(SHA256) upstream package : d5879f3afbad52fa2fa722a2913d5e04373a93ab9b2b1a651a623bfe28d67b36 Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14 Command line :/bin/fedora-review -n nss-securepass Buildroot used: fedora-21-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG
Hi Sinny, thanks for the review! I tried to address all the issues. The only strange thing is about LICENSE_APACHE2 that looks ok on my Linux system. Please note that libnss_sp.so is not a developer library, but is actually the plugin for the NSS subsystem, like libnss_nisplus.so SPEC: https://gpaterno.fedorapeople.org/nss-securepass.spec SRPM: https://gpaterno.fedorapeople.org/nss-securepass-0.2-4.el6.src.rpm For the commit, we are moving towards a "standard" release cycle shortly with the newer version, using tags in git, so it's not a real value changing now. What do you think? Thanks.
I extracted tar from your SRPM link using command $ rpm2cpio nss-securepass-0.2-4.el6.src.rpm | cpio -dium which gave me tar file nss_securepass-1415738661.c1bf10d.tar.gz In order to check permission of files inside tar , I executed $ tar pvxf nss_securepass-1415738661.c1bf10d.tar.gz -rwxrwxr-x root/root 294 2014-11-12 02:14 nss_securepass-c1bf10da1873bc212caa857653bef0b1e899703a/LICENSE -rwxrwxr-x root/root 10791 2014-11-12 02:14 nss_securepass-c1bf10da1873bc212caa857653bef0b1e899703a/LICENSE_APACHE2 -rwxrwxr-x root/root 17982 2014-11-12 02:14 nss_securepass-c1bf10da1873bc212caa857653bef0b1e899703a/LICENSE_GNUGPL -rwxrwxr-x root/root 1077 2014-11-12 02:14 nss_securepass-c1bf10da1873bc212caa857653bef0b1e899703a/LICENSE_MIT I see files LICENSE, LICENSE_APACHE2, LICENSE_GNUGPL and LICENSE_MIT has execute permission as well. Can you use same command in your machine and something different?
I had a look at last updated srpm given comment#16. Here is my review 1) The rpmlint output is nss-securepass.x86_64: W: spurious-executable-perm /usr/share/doc/nss-securepass/LICENSE_APACHE2 nss-securepass.x86_64: E: wrong-script-end-of-line-encoding /usr/share/doc/nss-securepass/LICENSE_APACHE2 nss-securepass.x86_64: W: spurious-executable-perm /usr/share/doc/nss-securepass/LICENSE_GNUGPL nss-securepass.x86_64: E: non-readable /etc/securepass.conf 0600L nss-securepass.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libnss_sp.so nss-securepass.x86_64: W: spurious-executable-perm /usr/share/doc/nss-securepass/LICENSE nss-securepass.x86_64: W: spurious-executable-perm /usr/share/doc/nss-securepass/LICENSE_MIT nss-securepass.x86_64: E: wrong-script-end-of-line-encoding /usr/share/doc/nss-securepass/LICENSE_MIT 3 packages and 0 specfiles checked; 3 errors, 5 warnings. ==> See on how to fix wrong-script-end-of-line-encoding http://fedoraproject.org/wiki/Packaging_tricks#Remove_DOS_line_endings Then you definitely need to set license files permission to 644 as chmod 644 LICENSE LICENSE_APACHE2 LICENSE_GNUGPL LICENSE_MIT Then, in %files you are setting permission as non-readable file on /etc/securepass.conf which is not good. Remove %attr(0600,root,root) Any specific reason you want to set such a permission? Then, devel-file-in-non-devel-package message is okay as this is plugin. 2) macros definition are not followed. See http://fedoraproject.org/wiki/Packaging:RPMMacros Change mkdir -p %{buildroot}/etc to mkdir -p %{buildroot}%{_sysconfdir} 3) I see that Makefile.in already having rule to install files. Use that using make install DESTDIR=%{buildroot} INSTALL="install -p" but then this is failing as Makefile.in is having rule $(INSTALL) -o root -g root $(LIBNSS_SP) $(DESTDIR)/$(libdir)/$(LIBNSS_SP) we need to remove "-o root -g root" and installation will work. You can remove this in %prep as sed -i 's|-o root -g root||g' Makefile.in But then as there is no rule to install configuration file, let's keep existing installation of securepass.conf file. So your %install will look simple as %install make install DESTDIR=%{buildroot} INSTALL="install -p" mkdir -p %{buildroot}%{_sysconfdir} install -m 644 securepass.conf.template %{buildroot}/etc/securepass.conf 4) Group tag is not necessary now and can be removed. http://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag
Hello! For 1) I have to apologise as I forgot to sync with upstream fixes. In the new package I also moved to the new release schema that has been adopted upstream. The reason is 0600 as the securepass.conf contains the private keys to access the server. I want this to be secure by default, so that the sysadmin is forcing the read attribute only if he/she needs it for other applications. I think I fixed the other suggestions. Builds are ok on koji. New files: SPEC: https://gpaterno.fedorapeople.org/nss-securepass.spec SRPM: https://gpaterno.fedorapeople.org/nss-securepass-0.2-5.el6.src.rpm Thanks a lot!
Yes, Execute permission issue in License files are fixed now. I see one minor issue while running fedora-review tool on your latest package Generic: [!]: Spec file according to URL is the same as in SRPM. Note: Spec file as given by url is not the same as in SRPM (see attached diff). Url mentioned in Source0 field is different in spec file which you uploaded and one extracted from SRPM. It maybe due to spec file has been modified after generating SRPM.
As I was working on the SPEC, the file itself wasn't committed upstream... not worth until I heavily worked on it. Now the upstream file is sync'ed: https://github.com/garlsecurity/nss_securepass/blob/master/nss-securepass.spec Confirm it still builds on koji for f22, epel6 and epel7 on all the platforms. SPEC: https://gpaterno.fedorapeople.org/nss-securepass.spec SRPM: https://gpaterno.fedorapeople.org/nss-securepass-0.2.2-1.el6.src.rpm Hope it is all ok now and cleared to go :-)
Updated package, as it now includes PAM support as well. SPEC: https://gpaterno.fedorapeople.org/nss-securepass.spec SRPM: https://gpaterno.fedorapeople.org/nss-securepass-0.3-1.el6.src.rpm
Need a sponsor to push it in fedora please :)
Small update, fixed spec file for buildrequires, as now includes PAM module. SPEC: https://gpaterno.fedorapeople.org/nss-securepass.spec SRPM: https://gpaterno.fedorapeople.org/nss-securepass-0.3-2.el6.src.rpm
Hi, We have this process http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group to get sponsored into the packager group. Can you either submit few more packages and/or some full detailed package reviews? This is needed to make sure package submitter understands the rpm packaging well and follows the fedora packaging guidelines. Please go through the following links 1) http://fedoraproject.org/wiki/Package_Review_Process 2) https://fedoraproject.org/wiki/PackagingGuidelines 3) To find the packages already submitted for review, check http://fedoraproject.org/PackageReviewStatus/ 4) http://fedoraproject.org/wiki/Packaging:ReviewGuidelines and http://fedoraproject.org/wiki/Package_Review_Process#Reviewer is useful while doing package reviews. 5) https://fedorahosted.org/FedoraReview/ this is fedora-review tool to help review packages in fedora. You need to use this and do un-official package reviews of packages submitted by other contributors. While doing so mention "This is un-official review of the package." at top of your review comment. Good to review packages listed in http://fedoraproject.org/PackageReviewStatus/NEW.html When you do full package review of some packages, provide that review comment link here so that I can look how you have reviewed those packages. If you got any questions please ask :)
Hi Parag, in the meanwhile please accept another RPM packaging contribution on: https://bugzilla.redhat.com/show_bug.cgi?id=1259061 I will try making some more reviews. Best, Giuseppe
I did some additional reviews here: https://bugzilla.redhat.com/show_bug.cgi?id=1246974 https://bugzilla.redhat.com/show_bug.cgi?id=1084202 https://bugzilla.redhat.com/show_bug.cgi?id=1262645
Can you make 3 full package-reviews with all the markings and suggestions/fix to issues found in package spec?
Hi Giuseppe, You around?
well looks not around. Let's close this. When you want to continue with this, please reopen.
Hello! I'm sorry for this, but I was out for health issues. I'll assign it to someone that can assist on my behalf
Upstream package has been deprecated.