Bug 725905 - Review Request: p11-kit - Library for loading and sharing PKCS#11 modules
Summary: Review Request: p11-kit - Library for loading and sharing PKCS#11 modules
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: Tomas Mraz
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-07-26 22:36 UTC by Kalev Lember
Modified: 2011-08-17 15:59 UTC (History)
4 users (show)

Fixed In Version: p11-kit-0.3-2.fc16
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-08-17 15:59:48 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tmraz: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Kalev Lember 2011-07-26 22:36:47 UTC
Spec URL: http://kalev.fedorapeople.org/p11-kit.spec
SRPM URL: http://kalev.fedorapeople.org/p11-kit-0.2-1.fc15.src.rpm
Description:
p11-kit provides a way to load and enumerate PKCS#11 modules, as well
as a standard configuration setup for installing PKCS#11 modules in
such a way that they're discoverable.

Comment 1 Kalev Lember 2011-07-26 22:38:15 UTC
Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3232586

Comment 2 Kalev Lember 2011-07-26 22:45:43 UTC
Stef, by what name are applications supposed to look up the library if they want to use it as a PKCS11 proxy module? Should we be installing the libp11-kit.so symlink in the main package so that applications could dlopen() it using that name?

Also, are you aware that using Apache License 2.0 code makes the whole library incompatible with GPLv2-only code?

Comment 3 Stef Walter 2011-07-27 07:28:47 UTC
(In reply to comment #2)
> Stef, by what name are applications supposed to look up the library if they
> want to use it as a PKCS11 proxy module? Should we be installing the
> libp11-kit.so symlink in the main package so that applications could dlopen()
> it using that name?

That sounds like a good plan. I look at putting this into p11-kit upstream.

> Also, are you aware that using Apache License 2.0 code makes the whole library
> incompatible with GPLv2-only code?

Hmmm, that's true. I'll rewrite the bits that came from apr, and then we can get rid of this issue.

Comment 4 Stef Walter 2011-07-27 09:33:10 UTC
Fixed apache license issue in p11-kit master.

Comment 5 Stef Walter 2011-07-27 10:15:42 UTC
Added the symlink.

Kalev, could you please verify that this fixes the problems? Once you let me know, I'll package a new release.

Comment 6 Kalev Lember 2011-07-27 12:56:03 UTC
+# Proxy module is actually same as library, so install a link
+install-exec-hook:
+       $(LN_S) libp11-kit.so $(DESTDIR)$(libdir)/p11-kit-proxy.so

I think it would be better to create the symlink to libp11-kit.so.0 (note the .0), so that we could put the .so in -devel subpackage:
p11-kit rpm:
  p11-kit-proxy.so
  libp11-kit.so.0
  libp11-kit.so.0.0.0

p11-kit-devel rpm:
   libp11-kit.so


Otherwise looks very nice. Thanks for doing this, Stef!

Comment 7 Stef Walter 2011-07-27 13:43:39 UTC
Could you provide a patch to do this reliably even when the libtool versioning gets updated?

As far as I can tell libtool versioning is calculated within libtool, and I'm not sure how to access it from the makefile.

Comment 8 Kalev Lember 2011-07-27 21:21:32 UTC
I am not sure how to best do it either; anything I can come up with seems needlessly fragile. The following with readlink works at least:

 install-exec-hook:
-       $(LN_S) libp11-kit.so $(DESTDIR)$(libdir)/p11-kit-proxy.so
+       $(LN_S) `readlink $(DESTDIR)$(libdir)/libp11-kit.so` $(DESTDIR)$(libdir)/p11-kit-proxy.so

Comment 9 Stef Walter 2011-07-28 12:36:47 UTC
Good plan. Done.

Comment 10 Kalev Lember 2011-07-29 15:43:10 UTC
* Fri Jul 29 2011 Kalev Lember <kalevlember@gmail.com> - 0.3-1
- Update to 0.3
- Upstream rewrote the ASL 2.0 bits, which makes the whole package
  BSD-licensed

Spec URL: http://kalev.fedorapeople.org/p11-kit.spec
SRPM URL: http://kalev.fedorapeople.org/p11-kit-0.3-1.fc15.src.rpm

Comment 11 Tomas Mraz 2011-08-17 13:19:12 UTC
There is a problem that the -devel subpackage puts doc files in /usr/share/gtk-doc/html but this directory is not owned by anything that is required by p11-kit-devel.

Also parts of the documentation document the configuration file format and that should be perhaps documented somehow in the main package, or even some kind of example config files included in the main package.

Comment 12 Kalev Lember 2011-08-17 13:47:52 UTC
(In reply to comment #11)
> There is a problem that the -devel subpackage puts doc files in
> /usr/share/gtk-doc/html but this directory is not owned by anything that is
> required by p11-kit-devel.

That directory is owned by the p11-kit-devel package itself to avoid a runtime dependency on the gtk-doc package:
$ rpm -qlp p11-kit-devel-0.3-1.fc15.x86_64.rpm | grep gtk-doc
/usr/share/gtk-doc
/usr/share/gtk-doc/html
/usr/share/gtk-doc/html/p11-kit
/usr/share/gtk-doc/html/p11-kit/api-index-full.html
/usr/share/gtk-doc/html/p11-kit/config-format.html
...

See the section in the packaging guidelines on File and Directory Ownership [1] that specifically explains how to deal with gtk-doc directory ownership:
https://fedoraproject.org/wiki/Packaging/Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function


> Also parts of the documentation document the configuration file format and
> that should be perhaps documented somehow in the main package, or even
> some kind of example config files included in the main package.

I don't really want to put large html documentation in the main package. p11-kit is probably going to be included in the live CD and it's nice to keep the size down. However, instead of having the docs in the -devel package, it would be easy to split them out to a separate -doc package. In the future, when upstream has some example config files, we could also put these in the -doc subpackage. Would that be better than shipping all the docs in -devel?

Comment 13 Tomas Mraz 2011-08-17 14:48:56 UTC
You're right, I somehow overlooked that in the file list.

As for the documentation - I don't think splitting it to a separate -doc subpackage is a solution. The configuration file format is really simple and the documentation or example would be pretty minimal and doing a separate subpackage for it would be an overkill.

I'd really like to see the global and example module config file directly in the main package. And/or manual page for the p11-kit binary. However I will not block the package on this.

rpmlint output:
rpmlint -v SRPMS/p11-kit-0.3-1.fc14.src.rpm RPMS/x86_64/p11-kit*
p11-kit.src: I: checking
p11-kit.src: I: checking-url http://p11-glue.freedesktop.org/p11-kit.html (timeout 10 seconds)
p11-kit.src: I: checking-url http://p11-glue.freedesktop.org/releases/p11-kit-0.3.tar.gz (timeout 10 seconds)
p11-kit-debuginfo.x86_64: I: checking
p11-kit-debuginfo.x86_64: I: checking-url http://p11-glue.freedesktop.org/p11-kit.html (timeout 10 seconds)
p11-kit-devel.x86_64: I: checking
p11-kit-devel.x86_64: I: checking-url http://p11-glue.freedesktop.org/p11-kit.html (timeout 10 seconds)
p11-kit.x86_64: I: checking
p11-kit.x86_64: I: checking-url http://p11-glue.freedesktop.org/p11-kit.html (timeout 10 seconds)
p11-kit.x86_64: W: no-manual-page-for-binary p11-kit
4 packages and 0 specfiles checked; 0 errors, 1 warnings.

A manual page for the p11-kit would be nice however this is not a blocker.

The package is named according to the naming guidelines.
The package meets licensing and packaging guidelines.
The source tarball is the same as the upstream tarball.

Note for future - there will have to be probably added gettext if the localization of p11-kit is completed and configure also searches for the gtk-doc binaries. But they are both not needed to build the package now.

The requires on the base package should be changed to
 Requires: %{name}%{?_isa} = %{version}-%{release}
But this is again a nitpick, but please change it before the import.

The package is ACCEPTed.

Comment 14 Kalev Lember 2011-08-17 15:00:15 UTC
(In reply to comment #13)
> The requires on the base package should be changed to
>  Requires: %{name}%{?_isa} = %{version}-%{release}
> But this is again a nitpick, but please change it before the import.

Will do. Thanks for the review, Tomáš!

Comment 15 Kalev Lember 2011-08-17 15:01:56 UTC
New Package SCM Request
=======================
Package Name: p11-kit
Short Description: Library for loading and sharing PKCS#11 modules
Owners: kalev mclasen
Branches:
InitialCC:

Comment 16 Tomas Mraz 2011-08-17 15:26:30 UTC
Can you please add me as comaintainer?

Comment 17 Gwyn Ciesla 2011-08-17 15:28:49 UTC
Git done (by process-git-requests).

Comment 18 Kalev Lember 2011-08-17 15:38:17 UTC
Grr, I forgot to request the f16 branch. Tomas, I think you'll need to request the ACLs for the master branch in pkgdb, but the SCM admin request should cover the ACLs in f16.

Comment 19 Kalev Lember 2011-08-17 15:38:41 UTC
Package Change Request
======================
Package Name: p11-kit
New Branches: f16
Owners: kalev mclasen tmraz

Comment 20 Gwyn Ciesla 2011-08-17 15:40:36 UTC
Git done (by process-git-requests).

Comment 21 Kalev Lember 2011-08-17 15:59:48 UTC
Package imported and built; closing the ticket.


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