[Bug 634025] New: Review Request: PolarSSL - Light-weight cryptographic and SSL/TLS library
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: PolarSSL - Light-weight cryptographic and SSL/TLS library
https://bugzilla.redhat.com/show_bug.cgi?id=634025
Summary: Review Request: PolarSSL - Light-weight cryptographic and SSL/TLS library Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: mads@kiilerich.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
Spec URL: http://people.fedoraproject.org/~kiilerix/polarssl.spec SRPM URL: http://people.fedoraproject.org/~kiilerix/polarssl-0.14.0-1.fc13.src.rpm Description: PolarSSL is a light-weight open source cryptographic and SSL/TLS library written in C. PolarSSL makes it easy for developers to include cryptographic and SSL/TLS capabilities in their (embedded) applications with as little hassle as possible.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
Golo Fuchert packages@golotop.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |packages@golotop.de
--- Comment #1 from Golo Fuchert packages@golotop.de 2010-11-12 17:50:55 EST --- The package looks quite clean to me, except of one issue (see below). But since I am not yet sponsored I can only make an inofficial review.
Inofficial review:
[+] = ok [o] = does not apply [-] = not ok [?] = see comments
[+] rpmlint is quiet rpmlint polarssl-0.14.0-1.fc13.src.rpm ../RPMS/i686/polarssl-devel-0.14.0-1.fc14.i686.rpm ../RPMS/i686/polarssl-0.14.0-1.fc14.i686.rpm ../SPECS/polarssl.spec 3 packages and 1 specfiles checked; 0 errors, 0 warnings.
[+] The package is named according to the guidelines [+] Spec file name matches base package name [+] The package follows the Packaging Guidelines [+] The license is an approved licence [+] The License field matches the actual licence [+] License file from source file is included in %doc [+] The spec file is written in American English [+] The spec file is legible [+] Used sources match width upstream sources (md5) [+] Package build at least on one primary architecture (i686) [?] ExecludeArch for x86_64?? SEE COMMENT BELOW [+] All build dependencies are listed in the BuildRequires section [o] No locales for the package [+] Package stores shared libraries and calls ldconfig in %post/%postun [+] Package does not bundle copies of system libraries [o] Package is not relocatable [o] Package does not install any directories to be owned [+] No files are listed more then once in the %files section [+] File permissions are set properly (%defattr(...) is used) [+] Consistent use of macros [+] Package contains code and documentation only, no content [o] No large documentation files [+] %doc files do not affect runtime [o] No Header files included [o] No static libraries [+] Library files ending with .so correctly in a -devel package [+] -devel package requires the package correctly as %{name} = %{version}-%{release} [+] No libtool .la archives included [o] No GUI application [+] Package does not own files or directories that are owned by other packages [+] All filenames are valid UTF-8
-----
Comments:
- Wouldn't it be good practice to comment on the patches (i.e. where do they come from, why are they needed and does upstream know about them)? I think the guidelines encourage you to do this (http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_a...).
- The package fails to build here on the x86_64 architecture. Some files seem to be put in $BUILDROOT/usr/lib but later %{_libdir} is expanded to /usr/lib64 (of course). My fault or a bug?
-----
So in my opinion the package is ready after either dealing with the x86_64 issue, excluding this arch or tell me what I did wrong. ;-)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
--- Comment #2 from Golo Fuchert packages@golotop.de 2010-11-12 17:52:08 EST --- The package looks quite clean to me, except of one issue (see below). But since I am not yet sponsored I can only make an inofficial review.
Inofficial review:
[+] = ok [o] = does not apply [-] = not ok [?] = see comments
[+] rpmlint is quiet rpmlint polarssl-0.14.0-1.fc13.src.rpm ../RPMS/i686/polarssl-devel-0.14.0-1.fc14.i686.rpm ../RPMS/i686/polarssl-0.14.0-1.fc14.i686.rpm ../SPECS/polarssl.spec 3 packages and 1 specfiles checked; 0 errors, 0 warnings.
[+] The package is named according to the guidelines [+] Spec file name matches base package name [+] The package follows the Packaging Guidelines [+] The license is an approved licence [+] The License field matches the actual licence [+] License file from source file is included in %doc [+] The spec file is written in American English [+] The spec file is legible [+] Used sources match width upstream sources (md5) [+] Package build at least on one primary architecture (i686) [?] ExecludeArch for x86_64?? SEE COMMENT BELOW [+] All build dependencies are listed in the BuildRequires section [o] No locales for the package [+] Package stores shared libraries and calls ldconfig in %post/%postun [+] Package does not bundle copies of system libraries [o] Package is not relocatable [o] Package does not install any directories to be owned [+] No files are listed more then once in the %files section [+] File permissions are set properly (%defattr(...) is used) [+] Consistent use of macros [+] Package contains code and documentation only, no content [o] No large documentation files [+] %doc files do not affect runtime [o] No Header files included [o] No static libraries [+] Library files ending with .so correctly in a -devel package [+] -devel package requires the package correctly as %{name} = %{version}-%{release} [+] No libtool .la archives included [o] No GUI application [+] Package does not own files or directories that are owned by other packages [+] All filenames are valid UTF-8
-----
Comments:
- Wouldn't it be good practice to comment on the patches (i.e. where do they come from, why are they needed and does upstream know about them)? I think the guidelines encourage you to do this (http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_a...).
- The package fails to build here on the x86_64 architecture. Some files seem to be put in $BUILDROOT/usr/lib but later %{_libdir} is expanded to /usr/lib64 (of course). My fault or a bug?
-----
So in my opinion the package is ready after either dealing with the x86_64 issue, excluding this arch or tell me what I did wrong. ;-)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
--- Comment #3 from Mads Kiilerich mads@kiilerich.com 2010-11-12 19:39:58 EST --- Thank you for the comments.
Please verify that the issues are addressed with: http://people.fedoraproject.org/~kiilerix/polarssl.spec http://people.fedoraproject.org/~kiilerix/polarssl-0.14.0-2.fc14.src.rpm http://koji.fedoraproject.org/koji/taskinfo?taskID=2598431
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
--- Comment #4 from Golo Fuchert packages@golotop.de 2010-11-13 04:13:31 EST --- First I like to clarify my comment "[o] No Header files included". This sounds misleading and should of course be "[+] ... outside of the -devel package".
The package now builds fine and from my side there are no further objections. So let's see what the Gurus say!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
Martin Gieseking martin.gieseking@uos.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.gieseking@uos.de
--- Comment #5 from Martin Gieseking martin.gieseking@uos.de 2010-11-13 04:45:13 EST --- Here are two more (minor) things to consider:
- Please prefix the patch filenames with "polarssl-". This way it's much easier to identify them in rpmbuild/SOURCES.
- I also recommend to be more specific in %files: %{_libexecdir}/* => %{_libexecdir}/polarssl/ %{_includedir}/* => %{_includedir}/polarssl/
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
Andy Bailey bailey@akamai.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |bailey@akamai.com
--- Comment #6 from Andy Bailey bailey@akamai.com 2010-12-08 12:04:21 EST --- Here's the culprit for the broken x86_64 build:
--- a/library/CMakeLists.txt +++ b/library/CMakeLists.txt @@ -26,3 +26,7 @@ x509parse.c xtea.c ) + +INSTALL(TARGETS polarssl + DESTINATION lib + PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
--- Comment #7 from Mads Kiilerich mads@kiilerich.com 2010-12-08 12:11:01 EST --- Yes, thanks, I have fixed that patch but was waiting for more review before creating new builds.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
--- Comment #8 from Jason Tibbitts tibbs@math.uh.edu 2011-01-28 10:49:20 EST --- Golo, since you seem to be sponsored now, did you want to finish this review? Or, heck, anyone else who has commented here?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
Martin Gieseking martin.gieseking@uos.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |martin.gieseking@uos.de Flag| |fedora-review+
--- Comment #9 from Martin Gieseking martin.gieseking@uos.de 2011-01-29 05:49:03 EST --- OK, I'm volunteering. :) The package looks fine. However, I recommend to apply the minor improvements mentioned in comment #5. Also, please add short comments above the Patch fields telling what the patches do. These are no blockers though.
$ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm 4 packages and 0 specfiles checked; 0 errors, 0 warnings.
--------------------------------- key:
[+] OK [.] OK, not applicable [X] needs work ---------------------------------
[+] MUST: The package must be named according to the Package Naming Guidelines. [+] MUST: The spec file name must match the base package %{name}. [+] MUST: The package must meet the Packaging Guidelines. [+] MUST: The package must be licensed with a Fedora approved license. GPLv2+
[+] MUST: The License field in the package spec file must match the actual license. [+] MUST: The file containing the text of the license(s) for the package must be included in %doc. [+] MUST: The spec file must be written in American English. [+] MUST: The spec file for the package MUST be legible. [+] MUST: The sources used to build the package must match the upstream source. $ md5sum polarssl-0.14.0-gpl.tgz* 669a0582a27a5ec381542f0c67e966b7 polarssl-0.14.0-gpl.tgz 669a0582a27a5ec381542f0c67e966b7 polarssl-0.14.0-gpl.tgz.1
[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. koji scratch build (f15): http://koji.fedoraproject.org/koji/taskinfo?taskID=2748827
[.] MUST: If the package does not successfully compile, build or work on an architecture, ... [+] MUST: All build dependencies must be listed in BuildRequires. [+] MUST: When compiling C, C++, and Fortran files, %{optflags} must be applied. [.] MUST: The spec file MUST handle locales properly. [.] MUST: If a package installs files below %{_datadir}/icons, the icon cache must be updated. [+] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun. [+] MUST: Packages must NOT bundle copies of system libraries. [.] MUST: If the package is designed to be relocatable, ... [+] MUST: A package must own all directories that it creates. [+] MUST: A Fedora package must not list a file more than once in %files. [+] MUST: Permissions on files must be set properly. [.] MUST: Packages must not provide RPM dependency information when that information is not global in nature, or are otherwise handled. [.] MUST: When filtering automatically generated RPM dependency information, the filtering system implemented by Fedora must be used. [+] MUST: Each package must consistently use macros. [+] MUST: The package must contain code, or permissable content. [.] MUST: Large documentation files must go in a -doc subpackage. [+] MUST: Files in %doc must not affect the runtime of the application. [+] MUST: Header files must be in a -devel package. [.] MUST: Static libraries must be in a -static package. [+] MUST: .so (without suffix) must go in a -devel package. [+] MUST: devel packages must require the base package using a fully versioned dependency [+] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built. [.] MUST: Packages containing GUI applications must include a %{name}.desktop file [+] MUST: Packages must not own files or directories already owned by other packages. [+] MUST: All filenames in rpm packages must be valid UTF-8.
EPEL <= 5 only: [+] MUST: The spec file must contain a valid BuildRoot field. [+] MUST: At the beginning of %install, each package must run rm -rf %{buildroot}. [+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}. [.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
[.] SHOULD: If the source package does not include license text(s) as a separate file from upstream,... [X] SHOULD: Patch files should be prefixed with %{name}- [X] SHOULD: All patches should be commented in the spec file. [+] SHOULD: Timestamps of files should be preserved. [+] SHOULD: The reviewer should test that the package builds in mock. [+] SHOULD: The package should compile and build into binary rpms on all supported architectures. [+] SHOULD: The reviewer should test that the package functions as described. [+] SHOULD: If scriptlets are used, those scriptlets must be sane. [.] SHOULD: subpackages other than devel should require the base package using a fully versioned dependency. [.] SHOULD: pkgconfig(.pc) should be placed in a -devel pkg. [.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [.] SHOULD: your package should contain man pages for binaries/scripts.
---------------- Package APPROVED ----------------
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
Mads Kiilerich mads@kiilerich.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #10 from Mads Kiilerich mads@kiilerich.com 2011-01-29 20:20:31 EST --- New Package SCM Request ======================= Package Name: polarssl Short Description: Light-weight cryptographic and SSL/TLS library Owners: kiilerix Branches: f14 InitialCC:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
--- Comment #11 from Dennis Gilmore dennis@ausil.us 2011-01-30 19:02:48 EST --- Git done (by process-git-requests).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
--- Comment #12 from Fedora Update System updates@fedoraproject.org 2011-01-31 19:35:37 EST --- polarssl-0.14.0-4.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/polarssl-0.14.0-4.fc14
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
--- Comment #13 from Mads Kiilerich mads@kiilerich.com 2011-01-31 19:40:29 EST --- Thanks for the review(s).
Upstream is very friendly and competent but also very busy, quiet and private, so it will be interesting to see how this will work out ...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #14 from Fedora Update System updates@fedoraproject.org 2011-02-01 15:55:01 EST --- polarssl-0.14.0-4.fc14 has been pushed to the Fedora 14 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update polarssl'. You can provide feedback for this update here: https://admin.fedoraproject.org/updates/polarssl-0.14.0-4.fc14
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
--- Comment #15 from Fedora Update System updates@fedoraproject.org 2011-02-09 15:20:23 EST --- polarssl-0.14.0-4.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=634025
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Fixed In Version| |polarssl-0.14.0-4.fc14 Resolution| |ERRATA Last Closed| |2011-02-09 15:20:29
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla