Bug 1193923 - Review Request: mbedtls - polarssl package renaming process
Summary: Review Request: mbedtls - polarssl package renaming process
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-02-18 14:54 UTC by Morten Stevens
Modified: 2015-05-26 03:49 UTC (History)
5 users (show)

Fixed In Version: mbedtls-1.3.10-1.fc22
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-05-26 03:49:38 UTC
Type: ---
zbyszek: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Comment 1 Christopher Meng 2015-03-06 13:17:21 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?

Comment 2 Morten Stevens 2015-03-06 22:51:53 UTC
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

Comment 3 Bas Mevissen 2015-03-10 14:48:42 UTC
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.

Comment 4 Morten Stevens 2015-04-28 08:38:47 UTC
All complaints are resolved with https://bugzilla.redhat.com/show_bug.cgi?id=1193923#c2

We would like to complete the package review reqest.

Comment 5 Zbigniew Jędrzejewski-Szmek 2015-05-09 19:38:03 UTC
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

Comment 6 Morten Stevens 2015-05-09 22:27:19 UTC
(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

Comment 7 Zbigniew Jędrzejewski-Szmek 2015-05-12 16:50:46 UTC
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)

Comment 8 Morten Stevens 2015-05-12 21:37:43 UTC
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

Comment 9 Michael Schwendt 2015-05-13 08:28:27 UTC
> 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.

Comment 10 Michael Schwendt 2015-05-13 08:30:43 UTC
> %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.

Comment 11 Michael Schwendt 2015-05-13 08:33:53 UTC
> %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.)

Comment 12 Morten Stevens 2015-05-13 13:43:54 UTC
(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

Comment 13 Morten Stevens 2015-05-13 13:44:26 UTC
(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

Comment 14 Morten Stevens 2015-05-13 13:48:05 UTC
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

Comment 15 Zbigniew Jędrzejewski-Szmek 2015-05-13 14:16:14 UTC
(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.

Comment 16 Zbigniew Jędrzejewski-Szmek 2015-05-13 14:18:09 UTC
Please add a link to https://tls.mbed.org/foss-license-exception in the spec file, though.

Comment 17 Michael Schwendt 2015-05-13 14:59:33 UTC
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.

Comment 18 Morten Stevens 2015-05-13 15:20:25 UTC
@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

Comment 19 Michael Schwendt 2015-05-13 15:41:44 UTC
> %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.

Comment 20 Morten Stevens 2015-05-14 13:52:24 UTC
(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

Comment 21 Morten Stevens 2015-05-14 14:09:55 UTC
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

Comment 22 Gwyn Ciesla 2015-05-14 19:05:30 UTC
Git done (by process-git-requests).

Comment 23 Fedora Update System 2015-05-14 21:30:09 UTC
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

Comment 24 Fedora Update System 2015-05-16 10:31:44 UTC
mbedtls-1.3.10-1.fc22 has been pushed to the Fedora 22 testing repository.

Comment 25 Fedora Update System 2015-05-26 03:49:38 UTC
mbedtls-1.3.10-1.fc22 has been pushed to the Fedora 22 stable repository.


Note You need to log in before you can comment on or make changes to this bug.