Bug 1193923
| Summary: | Review Request: mbedtls - polarssl package renaming process | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Morten Stevens <mstevens> |
| Component: | Package Review | Assignee: | Zbigniew Jędrzejewski-Szmek <zbyszek> |
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | abuse, i, mads, package-review, zbyszek |
| Target Milestone: | --- | Flags: | zbyszek:
fedora-review+
gwync: fedora-cvs+ |
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | mbedtls-1.3.10-1.fc22 | Doc Type: | Bug Fix |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2015-05-26 03:49:38 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Morten Stevens
2015-02-18 14:54:31 UTC
1. URL: https://tls.mbed.org/ 2. Conflicts: polarssl I think this is incorrect, as from the rename, OBsoletes tag should be used: Obsoletes: polarssl < %{version}-%{release} 3. %cmake -D CMAKE_BUILD_TYPE:String="Release" . make %{?_smp_mflags} all apidoc %cmake -D CMAKE_BUILD_TYPE:String="Release" -D USE_SHARED_MBEDTLS_LIBRARY:BOOL=1 . make %{?_smp_mflags} all apidoc Duplicate? 4. %doc LICENSE ChangeLog Use %license macro. 5. No tests available for %check? Hi, Here is the latest mbedtls.spec file: https://mstevens.fedorapeople.org/mbedtls/mbedtls.spec https://mstevens.fedorapeople.org/mbedtls/mbedtls-1.3.10-1.fc23.src.rpm (In reply to Christopher Meng from comment #1) > 1. URL: https://tls.mbed.org/ fixed > 2. Conflicts: polarssl > > I think this is incorrect, as from the rename, OBsoletes tag should be used: > > Obsoletes: polarssl < %{version}-%{release} fixed > 3. %cmake -D CMAKE_BUILD_TYPE:String="Release" . > make %{?_smp_mflags} all apidoc > > %cmake -D CMAKE_BUILD_TYPE:String="Release" -D > USE_SHARED_MBEDTLS_LIBRARY:BOOL=1 . > make %{?_smp_mflags} all apidoc > > Duplicate? Yes, fixed. > 4. %doc LICENSE ChangeLog > > Use %license macro. fixed > 5. No tests available for %check? There is a check available. I've added the check, but the check is currently disabled due a bug. # check temporarily disabled due a bug # %check # LD_LIBRARY_PATH=$PWD/library ctest --output-on-failure -V Hi,
I have two questions:
> Provides: polarssl = %{version}-%{release}
1) When will the name "polarssl" be dropped? Can't that be done in F22 already?
There are currently only a few packages that require polarssl.
2) What is going to happen with the polarssl package in Fedora 21? Are we going to switch to mbedtls? Current version (1.3.9) is outdated and needs an update urgently.
Thanks,
Bas.
All complaints are resolved with https://bugzilla.redhat.com/show_bug.cgi?id=1193923#c2 We would like to complete the package review reqest. Yeah, package looks good. @Christopher: would you mind if I finish the review? - Consider using %make_install instead of the open-coded make install ... fedora-review complains: - Sources used to build the package match the upstream source, as provided in the spec URL. Note: Upstream MD5sum check error, diff is in /var/tmp/review- rc/1193923-mbedtls/diff.txt See: http://fedoraproject.org/wiki/Packaging/SourceURL - Static libraries in -static or -devel subpackage, providing -devel if present. Note: Package has .a files: mbedtls-devel. Does not provide -static: mbedtls-devel. See: http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries - Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 40683520 bytes in 1423 files. See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation (In reply to Zbigniew Jędrzejewski-Szmek from comment #5) Here is the latest mbedtls.spec file: https://mstevens.fedorapeople.org/mbedtls/mbedtls.spec https://mstevens.fedorapeople.org/mbedtls/mbedtls-1.3.10-1.fc23.src.rpm > - Consider using %make_install instead of the open-coded make install ... fixed > fedora-review complains: > - Sources used to build the package match the upstream source, as provided > in the spec URL. > Note: Upstream MD5sum check error, diff is in /var/tmp/review- > rc/1193923-mbedtls/diff.txt > See: http://fedoraproject.org/wiki/Packaging/SourceURL The URL has been changed to: Source0: https://tls.mbed.org/download/%{name}-%{version}-gpl.tgz > - Static libraries in -static or -devel subpackage, providing -devel if > present. > Note: Package has .a files: mbedtls-devel. Does not provide -static: > mbedtls-devel. > See: http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries new subpackage added: mbedtls-static > - Large documentation must go in a -doc subpackage. Large could be size > (~1MB) or number of files. > Note: Documentation size is 40683520 bytes in 1423 files. > See: > http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation new subpackage added: mbedtls-doc OK, since this a re-review, just the relevant points: - spec file is nice and clean - license is OK, but there's an additional exception. I think the license should be 'GPLv2+ with exceptions', and a link provided to https://tls.mbed.org/foss-license-exception in a comment. This extends available options (compared to GPLv2+). - provides/obsoletes are not OK: According to https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages, Obsoletes should have a concrete version number. It seems that %{?_isa} should also be used, as the package is an arch dependent libary. Obsoletes: polarssl%{?_isa} < 1.3.10 Provides: polarssl%{?_isa} = %{version}-%{release} I tried upgrading a system with polarssl-devel installed. It fails: Error: package polarssl-devel-1.3.9-3.fc22.x86_64 requires libpolarssl.so.7()(64bit), but none of the providers can be installed It seems that more obs/provs should be added: In -devel: Obsoletes: polarssl-devel%{?_isa} < 1.3.10 Provides: polarssl-devel%{?_isa} = %{version}-%{release} In -utils: Obsoletes: polarssl-utils < 1.3.10 Provides: polarssl-utils = %{version}-%{release} I'm not 100% sure about the exact form though. Please check that upgrading works :) - doc subpackage should be noarch. - I'd suggest adding: %global _docdir_fmt %{name} Currently there's /usr/share/doc/mbedtls with one file, and /usr/share/doc/mbedtls-doc with the rest. No use to have two directories. Some details pointed out by rpmlint: mbedtls.src:67: W: macro-in-comment %check mbedtls.src:45: W: mixed-use-of-spaces-and-tabs (spaces: line 26, tab: line 45) Okay, please check the latest srpm and spec file. All complaints should be resolved. https://mstevens.fedorapeople.org/mbedtls/mbedtls-1.3.10-1.fc23.src.rpm https://mstevens.fedorapeople.org/mbedtls/mbedtls.spec > I'm not 100% sure about the exact form though. :-/ > Please check that upgrading works :) As the reviewer, you should have done that prior to requesting the submitter to change something. %_isa Obsoletes do _not_ work. Obsoletes work on physical package %names. A simple test-update with the builds in a local repo would serve as demonstration. It would also show that polarssl-devel.x86_64 and polarssl-devel.i686 suffer from an implicit conflict. Please test mbedtls-devel for such a conflict during this review. > %package doc > Summary: Documentation files for %{name} > roup: Documentation > Requires: %{name}%{?_isa} = %{version}-%{release} > BuildArch: noarch > %files doc > %doc apidoc/* Please keep plain Documentation packages free of such base dependencies. There is no reason this -doc package needs a strict dependency on the base library. Such dependencies are a pain, if one only wants to peruse the documentation without dragging in an unneeded dep-chain. > %build
> %cmake -D CMAKE_BUILD_TYPE:String="Release" -D USE_SHARED_MBEDTLS_LIBRARY:BOOL=1 .
> make %{?_smp_mflags} all apidoc
>
> # check temporarily disabled due a bug
> # %check
> # LD_LIBRARY_PATH=$PWD/library ctest --output-on-failure -V
>
> %install
As %check is executed after %install, placing the %check section below %install would be the more logical choice. (Also note that some packages perform %checks on the buildroot contents, and that wouldn't work before %install.)
(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #10) > Please keep plain Documentation packages free of such base dependencies. > There is no reason this -doc package needs a strict dependency on the base > library. Such dependencies are a pain, if one only wants to peruse the > documentation without dragging in an unneeded dep-chain. fixed (In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #11) > As %check is executed after %install, placing the %check section below > %install would be the more logical choice. (Also note that some packages > perform %checks on the buildroot contents, and that wouldn't work before > %install.) fixed Please check the latest spec/srpm: https://mstevens.fedorapeople.org/mbedtls/mbedtls-1.3.10-1.fc23.src.rpm https://mstevens.fedorapeople.org/mbedtls/mbedtls.spec The upgrade from polarssl to mbedtls works fine for me. [root@fc23 mbedtls]# dnf install mbedtls-1.3.10-1.fc23.x86_64.rpm mbedtls-utils-1.3.10-1.fc23.x86_64.rpm mbedtls-devel-1.3.10-1.fc23.x86_64.rpm Last metadata expiration check performed 1:22:32 ago on Wed May 13 14:15:47 2015. Dependencies resolved. ============================================================================================================================================================================= Package Arch Version Repository Size ============================================================================================================================================================================= Installing: mbedtls x86_64 1.3.10-1.fc23 @commandline 243 k replacing polarssl.x86_64 1.3.9-3.fc22 mbedtls-devel x86_64 1.3.10-1.fc23 @commandline 112 k replacing polarssl-devel.x86_64 1.3.9-3.fc22 mbedtls-utils x86_64 1.3.10-1.fc23 @commandline 130 k replacing polarssl-utils.x86_64 1.3.9-3.fc22 Transaction Summary ============================================================================================================================================================================= Install 3 Packages Total size: 485 k Installed size: 1.8 M Is this ok [y/N]: y Downloading Packages: Running transaction check Transaction check succeeded. Running transaction test Transaction test succeeded. Running transaction Installing : mbedtls-1.3.10-1.fc23.x86_64 1/6 Installing : mbedtls-utils-1.3.10-1.fc23.x86_64 2/6 Installing : mbedtls-devel-1.3.10-1.fc23.x86_64 3/6 Obsoleting : polarssl-devel-1.3.9-3.fc22.x86_64 4/6 Obsoleting : polarssl-utils-1.3.9-3.fc22.x86_64 5/6 Obsoleting : polarssl-1.3.9-3.fc22.x86_64 6/6 Verifying : mbedtls-1.3.10-1.fc23.x86_64 1/6 Verifying : mbedtls-utils-1.3.10-1.fc23.x86_64 2/6 Verifying : mbedtls-devel-1.3.10-1.fc23.x86_64 3/6 Verifying : polarssl-devel-1.3.9-3.fc22.x86_64 4/6 Verifying : polarssl-utils-1.3.9-3.fc22.x86_64 5/6 Verifying : polarssl-1.3.9-3.fc22.x86_64 6/6 Installed: mbedtls.x86_64 1.3.10-1.fc23 mbedtls-devel.x86_64 1.3.10-1.fc23 mbedtls-utils.x86_64 1.3.10-1.fc23 (In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #9) Thank you for your comments. I *was* hoping that somebody more knowledgeable would look at this. > It would also show that polarssl-devel.x86_64 and polarssl-devel.i686 suffer > from an implicit conflict. Please test mbedtls-devel for such a conflict > during this review. Testing now... mbedtls and mbedtls-devel from x86_64 and i686 can be installed without any trouble. So can mbedtls-utils.x86_64 and mbedtls-utils.i686, and rpm -V says they are both fine, which I don't understand at all. Is this some rpm magic? > As the reviewer, you should have done that prior to requesting the submitter > to change something. I disagree here. I think it's acceptable for the reviewer to point out an issue without knowing how to, and without trying to, solve it. If I can, I'll definitely try to help, but sometimes I'm not able to devote the time necessary to propose a solution. Anyway, all issues seem to be solved. Package is APPROVED. Please add a link to https://tls.mbed.org/foss-license-exception in the spec file, though. The Obsoletes/Provides pairs are still not correct. > # replace polarssl with mbedtls > > Obsoletes: polarssl < 1.3.10 > Provides: polarssl%{?_isa} = %{version}-%{release} "Provides: polarssl = …" is missing. That's the one from the Renaming Guidelines that makes available the new package under the old name. Try "dnf install polarssl", for example. It should install "mbedtls" instead of choosing the old polarssl package. > %package devel > Summary: Development files for %{name} > Group: Development/Libraries > Requires: %{name}%{?_isa} = %{version}-%{release} > Obsoletes: polarssl-devel < 1.3.10 > Provides: polarssl-devel%{?_isa} = %{version}-%{release} Same here. Also note that | Provides: polarssl-devel%{?_isa} = %{version}-%{release} is not needed at all, if nothing depends on polarssl-devel%{?_isa} (e.g. other -devel packages sometimes depend on %_isa -devel packages). And it's not polarssl-devel.x86_64 either. So, if currently somebody runs "dnf install polarssl-devel.x86_64", your Obs/Prov don't cover that yet and don't install mbedtls-devel.x86_64. > %package static > Summary: Static files for %{name} > Group: Development/Libraries > Requires: %{name}%{?_isa} = %{version}-%{release} It would make more sense to have it depend on the -devel package. @Michael All right. Please check my latest srpm/spec. https://mstevens.fedorapeople.org/mbedtls/mbedtls-1.3.10-1.fc23.src.rpm https://mstevens.fedorapeople.org/mbedtls/mbedtls.spec diff: --- mbedtls.spec.old 2015-05-13 15:31:59.000000000 +0200 +++ mbedtls.spec 2015-05-13 17:13:49.239760329 +0200 @@ -5,7 +5,7 @@ Version: 1.3.10 Release: 1%{?dist} Summary: Light-weight cryptographic and SSL/TLS library Group: System Environment/Libraries -License: GPLv2+ with exceptions +License: GPLv2+ with exceptions (https://tls.mbed.org/foss-license-exception) URL: https://tls.mbed.org/ Source0: https://tls.mbed.org/download/%{name}-%{version}-gpl.tgz @@ -16,14 +16,13 @@ BuildRequires: graphviz # replace polarssl with mbedtls Obsoletes: polarssl < 1.3.10 -Provides: polarssl%{?_isa} = %{version}-%{release} +Provides: polarssl = %{version}-%{release} %description Mbed TLS is a light-weight open source cryptographic and SSL/TLS library written in C. Mbed TLS makes it easy for developers to include cryptographic and SSL/TLS capabilities in their (embedded) applications with as little hassle as possible. -FOSS License Exception: https://tls.mbed.org/foss-license-exception %package utils Summary: Utilities for %{name} @@ -40,7 +39,7 @@ Summary: Development files for %{ Group: Development/Libraries Requires: %{name}%{?_isa} = %{version}-%{release} Obsoletes: polarssl-devel < 1.3.10 -Provides: polarssl-devel%{?_isa} = %{version}-%{release} +Provides: polarssl-devel = %{version}-%{release} %description devel The %{name}-devel package contains libraries and header files for @@ -49,7 +48,7 @@ developing applications that use %{name} %package static Summary: Static files for %{name} Group: Development/Libraries -Requires: %{name}%{?_isa} = %{version}-%{release} +Requires: %{name}-devel = %{version}-%{release} %description static The %{name}-static package contains static files for > %package static > -Requires: %{name}%{?_isa} = %{version}-%{release} > +Requires: %{name}-devel = %{version}-%{release} Here you're free to keep the %?_isa in that dependency, since your base package dep in the -devel package also adds %?_isa (as covered by the guidelines). That would make the entire depchain -static => -devel => base package arch-specific. (In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #19) > > %package static > > > -Requires: %{name}%{?_isa} = %{version}-%{release} > > +Requires: %{name}-devel = %{version}-%{release} > > Here you're free to keep the %?_isa in that dependency, since your base > package dep in the -devel package also adds %?_isa (as covered by the > guidelines). That would make the entire depchain -static => -devel => base > package arch-specific. All right. Please check my latest srpm/spec: https://mstevens.fedorapeople.org/mbedtls/mbedtls-1.3.10-1.fc23.src.rpm https://mstevens.fedorapeople.org/mbedtls/mbedtls.spec New Package SCM Request ======================= Package Name: mbedtls Short Description: Light-weight cryptographic and SSL/TLS library Upstream URL: https://tls.mbed.org/ Owners: mstevens Branches: rawhide f22 f21 f20 el5 el6 epel7 InitialCC: mstevens Git done (by process-git-requests). mbedtls-1.3.10-1.fc22 has been submitted as an update for Fedora 22. https://admin.fedoraproject.org/updates/mbedtls-1.3.10-1.fc22 mbedtls-1.3.10-1.fc22 has been pushed to the Fedora 22 testing repository. mbedtls-1.3.10-1.fc22 has been pushed to the Fedora 22 stable repository. |