Bug 1085752

Spec URL: https://www.dropbox.com/s/cupn2ukc35omi09/check-create-certificate.spec
SRPM URL: https://www.dropbox.com/s/njvj4qh8logqr4q/check-create-certificate-0.5-1.fc20.src.rpm

%changelog
* Wed Apr 09 2014 Martin Gansser <martinkg> - 0.5-2
- Added fsf-fix patch that fixes rpmlint error
- specfile cleanup

rpmlint check-create-certificate-0.5-1.fc20.noarch.rpm
check-create-certificate.noarch: W: spelling-error %description -l en_US existance -> existence, assistance, Resistance
check-create-certificate.noarch: W: no-manual-page-for-binary check-create-certificate
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

Spec URL: https://www.dropbox.com/s/cupn2ukc35omi09/check-create-certificate.spec

SRPM URL: https://www.dropbox.com/s/xjjty2exra02pgk/check-create-certificate-0.5-3.fc20.src.rpm

%changelog
* Wed Apr 09 2014 Martin Gansser <martinkg> - 0.5-3
- checked out new sources
- removed fsf-fix patch
- changed to new Url address

Spec URL: http://martinkg.fedorapeople.org/Review/SPECS/check-create-certificate.spec

SRPM URL: http://martinkg.fedorapeople.org/Review/SRPMS/check-create-certificate-0.5-4.fc20.src.rpm

%changelog
* Wed Apr 09 2014 Martin Gansser <martinkg> - 0.5-4
- removed attr in file section

I will review your package.

Here are some comments.


The %defattr macro is obsolete for a long time (see https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions). You can drop it in %files. You can also drop the %attr(...) macro on %{_sbindir}/%{name}, the right permissions on this files are already defined at package build automatically here.


You don't need to add coreutils to the BuildRequires: this package is already part of the minimal build environment (see https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2).


You can also drop perl as Requires: The auto Provides system will detected Perl requirements at build time. You can see it in the build logs:
[...]
Processing files: check-create-certificate-0.5-4.fc20.noarch
[...]
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Requires: /usr/bin/perl perl(File::Basename) perl(Getopt::Long) perl(strict)
[...]
As you can see, /usr/bin/perl, as well as the File::Basename/Getopt::Long/strict Perl libraries, are detected and added as automatic Provides to your package.


The check-create-certificate script calls the c_rehash command, which is provided by the openssl-perl package. As a result, you can replace openssl by openssl-perl in Requires (openssl-perl already requires openssl).


The install command can create on the fly the destination folder if it doesn't exist (see -D option). You can replace these lines:
   mkdir -p %{buildroot}%{_prefix}/sbin
   install -m 755 script/%{name} %{buildroot}%{_sbindir}/
by this simpler one:
   install -Dpm 755 script/%{name} %{buildroot}%{_sbindir}/%{name}
(notice the -p option to preserver the timestamp).


You don't need to manually deploy in %install the COPYING file (or any other documentation file not installed by default). Just use the %doc macro in %files: it will automatically deploy all its arguments to %{_docdir}/%{name} (or %{_docdir}/%{name}-%{version} in Fedora < 20):
%files
%doc COPYING
%{_sbindir}/%{name}


The upstream versionning is quite weird. The upstream source packaging relies on a .spec file which contains the current version for the project (0.5 now). The project history shows instead that the version 0.5 was released 4 years ago (see https://github.com/jdsn/check-create-certificate/commit/160fc42e77d44ef39ba84dd6226f61184332a255).
Moreover, the way to retrieve the sources you gave in comments will always bring you the latest snapshot available, it doesn't correspond to a defined snapshot.
I suggest you:
- to rely instead on a Git commit to version your package; you can consider here the latest snapshot available today ([d0971baf5d13e06aaa600581efe3adba6631e06a]) which brings some good improvements
- as a result, to use a postrelease numbering schema for the release tag (see https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Post-Release_packages).
You can have a look at this .spec file to inspire you:
   http://cbb.fedorapeople.org/packages/dex.spec

(In reply to Mohamed El Morabity from comment #5)
> Here are some comments.

Thanks for your time and for the review
>
> The %defattr macro is obsolete for a long time (see
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions). You
> can drop it in %files. You can also drop the %attr(...) macro on
> %{_sbindir}/%{name}, the right permissions on this files are already defined
> at package build automatically here.
> 
already done in package 0.5-4

> You don't need to add coreutils to the BuildRequires: this package is
> already part of the minimal build environment (see
> https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2).
> 
done
 
> You can also drop perl as Requires: The auto Provides system will detected
> Perl requirements at build time. You can see it in the build logs:
> [...]
> Processing files: check-create-certificate-0.5-4.fc20.noarch
> [...]
> Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests)
> <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
> Requires: /usr/bin/perl perl(File::Basename) perl(Getopt::Long) perl(strict)
> [...]
> As you can see, /usr/bin/perl, as well as the
> File::Basename/Getopt::Long/strict Perl libraries, are detected and added as
> automatic Provides to your package.
>

done 
> 
> The check-create-certificate script calls the c_rehash command, which is
> provided by the openssl-perl package. As a result, you can replace openssl
> by openssl-perl in Requires (openssl-perl already requires openssl).
> 

done
> 
> The install command can create on the fly the destination folder if it
> doesn't exist (see -D option). You can replace these lines:
>    mkdir -p %{buildroot}%{_prefix}/sbin
>    install -m 755 script/%{name} %{buildroot}%{_sbindir}/
> by this simpler one:
>    install -Dpm 755 script/%{name} %{buildroot}%{_sbindir}/%{name}
> (notice the -p option to preserver the timestamp).
> 

done
> 
> You don't need to manually deploy in %install the COPYING file (or any other
> documentation file not installed by default). Just use the %doc macro in
> %files: it will automatically deploy all its arguments to %{_docdir}/%{name}
> (or %{_docdir}/%{name}-%{version} in Fedora < 20):
> %files
> %doc COPYING
> %{_sbindir}/%{name}
> 

done
> 
> The upstream versionning is quite weird. The upstream source packaging
> relies on a .spec file which contains the current version for the project
> (0.5 now). The project history shows instead that the version 0.5 was
> released 4 years ago (see
> https://github.com/jdsn/check-create-certificate/commit/
> 160fc42e77d44ef39ba84dd6226f61184332a255).
> Moreover, the way to retrieve the sources you gave in comments will always
> bring you the latest snapshot available, it doesn't correspond to a defined
> snapshot.
> I suggest you:
> - to rely instead on a Git commit to version your package; you can consider
> here the latest snapshot available today
> ([d0971baf5d13e06aaa600581efe3adba6631e06a]) which brings some good
> improvements
> - as a result, to use a postrelease numbering schema for the release tag
> (see
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Post-
> Release_packages).
> You can have a look at this .spec file to inspire you:
>    http://cbb.fedorapeople.org/packages/dex.spec

hope i translate it correct ?

New files:

Spec URL: http://martinkg.fedorapeople.org/Review/SPECS/check-create-certificate.spec

SRPM URL: http://martinkg.fedorapeople.org/Review/SRPMS/check-create-certificate-0.5-5.20140410gitd0971ba.fc20.src.rpm

%changelog
* Thu Apr 10 2014 Martin Gansser <martinkg> - 0.5-5.20140410gitd0971ba
- droped coreutils, perl BuildRequires
- replaced openssl by openssl-perl BuildRequires
- added -p option to preserver the timestamp in install section
- changed doc macro in files section
- used git commit for package version

(In reply to MartinKG from comment #6)
> hope i translate it correct ?
Perfectly, the release tag seems correct :). Just be careful, the date the snapshot was made is 2014/04/09, not 2014/04/10 (see https://github.com/jdsn/check-create-certificate/commit/d0971baf5d13e06aaa600581efe3adba6631e06a).

Github provides a convenient way to retrieve a snapshot from its commit: you should use the source URL below to download the source archive corresponding to a given snapshot:
  Source0:    https://github.com/jdsn/check-create-certificate/archive/%{commit}/%{name}-%{shortcommit}.tar.gz

Since the COPYING file is installed through the %doc macro, you don't need anymore to deploy it manually in %install: you can drop the lines below:
   mkdir -p ${RPM_BUILD_ROOT}/%{_docdir}/%{name}
   install -m 644 COPYING ${RPM_BUILD_ROOT}/%{_docdir}/%{name}

At last, in %description: "existence", not "existance" ;)

(In reply to Mohamed El Morabity from comment #7)
> (In reply to MartinKG from comment #6)
> > hope i translate it correct ?
> Perfectly, the release tag seems correct :). Just be careful, the date the
> snapshot was made is 2014/04/09, not 2014/04/10 (see
> https://github.com/jdsn/check-create-certificate/commit/
> d0971baf5d13e06aaa600581efe3adba6631e06a).

corrected

> Github provides a convenient way to retrieve a snapshot from its commit: you
> should use the source URL below to download the source archive corresponding
> to a given snapshot:
>   Source0:   
> https://github.com/jdsn/check-create-certificate/archive/%{commit}/%{name}-
> %{shortcommit}.tar.gz

corrected

> Since the COPYING file is installed through the %doc macro, you don't need
> anymore to deploy it manually in %install: you can drop the lines below:
>    mkdir -p ${RPM_BUILD_ROOT}/%{_docdir}/%{name}
>    install -m 644 COPYING ${RPM_BUILD_ROOT}/%{_docdir}/%{name}
> 
> At last, in %description: "existence", not "existance" ;)
done

Spec URL: https://www.dropbox.com/s/cupn2ukc35omi09/check-create-certificate.spec

SRPM URL: https://www.dropbox.com/s/blb27bx0v90c5hw/check-create-certificate-0.5-6.20140409gitd0971ba.fc20.src.rpm

%changelog
* Fri Apr 11 2014 Martin Gansser <martinkg> - 0.5-6.20140409gitd0971ba
- corrected snapshotdate
- corrected source url
- corrected git command
- droped %%install command for doc file in %%install section
- correted spelling in %%description

Your package doesn't build:

$ LANG=C rpmbuild -ba check-create-certificate.spec
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.dcnPEx
+ umask 022
+ cd /home/mohamed/rpmbuild/BUILD
+ cd /home/mohamed/rpmbuild/BUILD
+ rm -rf check-create-certificate-d0971ba
+ /usr/bin/gzip -dc /home/mohamed/rpmbuild/SOURCES/check-create-certificate-d0971ba.tar.gz
+ /usr/bin/tar -xf -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd check-create-certificate-d0971ba
/var/tmp/rpm-tmp.dcnPEx: line 37: cd: check-create-certificate-d0971ba: No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.dcnPEx (%prep)

Once uncompressed, the source folder is named %{name}-%{commit} in fact, not %{name}-%{shortcommit} . Please fix it in %setup.

(In reply to Mohamed El Morabity from comment #9)
> Your package doesn't build:
> 
> $ LANG=C rpmbuild -ba check-create-certificate.spec
> Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.dcnPEx
> + umask 022
> + cd /home/mohamed/rpmbuild/BUILD
> + cd /home/mohamed/rpmbuild/BUILD
> + rm -rf check-create-certificate-d0971ba
> + /usr/bin/gzip -dc
> /home/mohamed/rpmbuild/SOURCES/check-create-certificate-d0971ba.tar.gz
> + /usr/bin/tar -xf -
> + STATUS=0
> + '[' 0 -ne 0 ']'
> + cd check-create-certificate-d0971ba
> /var/tmp/rpm-tmp.dcnPEx: line 37: cd: check-create-certificate-d0971ba: No
> such file or directory
> error: Bad exit status from /var/tmp/rpm-tmp.dcnPEx (%prep)
> 
> Once uncompressed, the source folder is named %{name}-%{commit} in fact, not
> %{name}-%{shortcommit} . Please fix it in %setup.

fixed

Spec URL: https://www.dropbox.com/s/cupn2ukc35omi09/check-create-certificate.spec

SRPM URL: https://www.dropbox.com/s/sk5yrs24hp1sapg/check-create-certificate-0.5-7.20140409gitd0971ba.fc20.src.rpm

%changelog
https://www.dropbox.com/s/sk5yrs24hp1sapg/check-create-certificate-0.5-7.20140409gitd0971ba.fc20.src.rpm

sorry forgot to paste the correct changelog.

%changelog
* Fri Apr 11 2014 Martin Gansser <martinkg> - 0.5-7.20140409gitd0971ba
- corrected commit name in %%setup section
- removed checkout instructions

Here is at last the review:

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. No licenses
     found. Please check the source files for licenses manually.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 30720 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[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:
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[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).


Rpmlint
-------
Checking: check-create-certificate-0.5-7.20140409gitd0971ba.fc20.noarch.rpm
          check-create-certificate-0.5-7.20140409gitd0971ba.fc20.src.rpm
check-create-certificate.noarch: W: no-manual-page-for-binary check-create-certificate
2 packages and 0 specfiles checked; 0 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint check-create-certificate
check-create-certificate.noarch: W: no-manual-page-for-binary check-create-certificate
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'



Requires
--------
check-create-certificate (rpmlib, GLIBC filtered):
    /usr/bin/perl
    openssl-perl
    perl(File::Basename)
    perl(Getopt::Long)
    perl(strict)



Provides
--------
check-create-certificate:
    check-create-certificate



Source checksums
----------------
https://github.com/jdsn/check-create-certificate/archive/d0971baf5d13e06aaa600581efe3adba6631e06a/check-create-certificate-d0971baf5d13e06aaa600581efe3adba6631e06a.tar.gz :
  CHECKSUM(SHA256) this package     : 8b6e1fc13f2cd031cd3e679f6e0f8ce84c1dcee37f83b2ccfec1c51b444aafe3
  CHECKSUM(SHA256) upstream package : 8b6e1fc13f2cd031cd3e679f6e0f8ce84c1dcee37f83b2ccfec1c51b444aafe3


This package is APPROVED!

@Mohammed
Thanks for the review

New Package SCM Request
=======================
Package Name: check-create-certificate
Short Description: A script that checks for the existance of an SSL certificate
Owners: martinkg
Branches: f20 f21
InitialCC:

Git done (by process-git-requests).

check-create-certificate-0.5-7.20140409gitd0971ba.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/check-create-certificate-0.5-7.20140409gitd0971ba.fc20

package successfully build for rawhide and f20

check-create-certificate-0.5-7.20140409gitd0971ba.fc20 has been pushed to the Fedora 20 stable repository.