This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 190664 - Review Request: keyutils - Kernel key management userspace utilities
Review Request: keyutils - Kernel key management userspace utilities
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-05-04 07:25 EDT by David Howells
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-08 05:40:08 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description David Howells 2006-05-04 07:25:10 EDT
Spec URL: http://people.redhat.com/~dhowells/keyutils/keyutils-1.0-2.fc5/keyutils.spec
SRPM URL: http://people.redhat.com/~dhowells/keyutils/keyutils-1.0-2.fc5/keyutils-1.0-2.fc5.src.rpm
Description:
This package provides userspace utility programs to allow the user to manipulate the kernel key management facility.  Libraries are provided so that other userspace programs may use the facility as well.  The binaries need to go in /bin, /sbin and /lib so that they can be used to authorise/authenticate the mounting of /usr.
This package is included as part of RHEL-4 and has been tested there.
Comment 1 Ralf Corsepius 2006-05-04 08:10:39 EDT
Before we can continue, please fix these issues:
# rpmlint keyutils-*.rpm W: keyutils no-url-tag
E: keyutils standard-dir-owned-by-package /usr/share/man/man5
E: keyutils standard-dir-owned-by-package /usr/share/man/man8
E: keyutils executable-marked-as-config-file /etc/request-key.conf
E: keyutils script-without-shellbang /etc/request-key.conf
E: keyutils standard-dir-owned-by-package /usr/share/man/man1
W: keyutils-debuginfo no-url-tag
W: keyutils-devel no-dependency-on keyutils
W: keyutils-devel no-url-tag
W: keyutils-devel no-documentation
E: keyutils-devel script-without-shellbang /usr/include/keyutils.h
W: keyutils-libs no-url-tag

Besides these, the follow issues caught my eye: 
* Unowned directory:
/usr/share/keyutils

* BuildRoot not conforming to FE guidelines

* Why this (IMO weird) library naming?
/lib/libkeyutils-1.0.2.*.so
/lib/libkeyutils.so.1
Comment 2 David Howells 2006-05-04 15:27:01 EDT
I've put a new SPEC and SRPM up for download:

SPEC URL: 
http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-1/keyutils.spec
SRPM URL: 
http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-1/keyutils-1.1-1.src.rpm

This should fix everything barring the "weird" library naming - with that, I 
followed the Ulrich Drepper guide to library naming.  Btw, did you mean a 
literal '*' in the name of a library?
Comment 3 Ralf Corsepius 2006-05-04 23:22:26 EDT
(In reply to comment #2)
Looks *much* better now.

> This should fix everything barring the "weird" library naming - with that, I 
> followed the Ulrich Drepper guide to library naming.
Well, how comes the rest of the world is not following this proposal?

The only thing that matters is the SONAME, the library's filename is largely
ignorable (c.f. info libtool, for why this naming is considered harmful).

>  Btw, did you mean a 
> literal '*' in the name of a library?
Nope, your are encoding the rpm's Release-tag into the library name. I am
building under fc4 and changed the spec's Release:-tag into FE conforming:
Release: 1%{?dist}

With this, I end up with
 /lib/libkeyutils-1.1.1.fc4.so

=> Bug in your Makefile: The spec's Release tag overrides the Makefile's RELEASE
variable.

Further (minor) issue:
# rpm -qvlp keyutils-libs-devel-1.1-1.fc4.i386.rpm
...
/usr/lib/libkeyutils.so -> //lib/libkeyutils.so.1
...
Note the double slash at the beginning. I am not aware about this being harmful
under Linux, but under other OS it is harmful.

Further general issue:
* Shipping a static library (Discouraged with FE).
Comment 4 David Howells 2006-05-05 08:00:30 EDT
> Well, how comes the rest of the world is not following this proposal?

glibc and binutils both use it.

> The only thing that matters is the SONAME, the library's filename is largely
> ignorable (c.f. info libtool, for why this naming is considered harmful).

Well, in my libtool info page, in section 6.4 (Managing release information), 
it holds up `libbfd-2.9.0.so' as the example of naming.

> With this, I end up with
>  /lib/libkeyutils-1.1.1.fc4.so

The library's filename is, as you said, largely ignorable; and the fact that 
the library version number contains 'fc4' will not cause binary 
incompatibility, since the SONAME is set to the interface symlink 
(/lib/keyutils.so.N).

What would you suggest? I want the release number in there since the library 
may well be different between two compilations. I suppose I could edit out the 
distribution tag, but that's messy.

Anyway, I've fixed the Makefile problem and the double-slash problem:

SPEC URL: 
http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-2/keyutils.spec
SRPM URL: 
http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-2/keyutils.spec

The static library should be there as this library wraps some system calls 
that aren't available through glibc.

Btw, note that PAM and mount may both need to use the library in this package 
at some point.
Comment 5 Ralf Corsepius 2006-05-05 08:16:01 EDT
(In reply to comment #4)
> > Well, how comes the rest of the world is not following this proposal?
> 
> glibc and binutils both use it.

> > The only thing that matters is the SONAME, the library's filename is largely
> > ignorable (c.f. info libtool, for why this naming is considered harmful).
> 
> Well, in my libtool info page, in section 6.4 (Managing release information), 
> it holds up `libbfd-2.9.0.so' as the example of naming.

Yes, binutils is a counter example of good practice and glibc is Drepper's
playground - Almost everybody else is not following this bad habits.

> > With this, I end up with
> >  /lib/libkeyutils-1.1.1.fc4.so
> 
> The library's filename is, as you said, largely ignorable; and the fact that 
> the library version number contains 'fc4' will not cause binary 
> incompatibility, since the SONAME is set to the interface symlink 
> (/lib/keyutils.so.N).
> 
> What would you suggest?
You should learn to destinguish library API-versioning from package versioning.

> Anyway, I've fixed the Makefile problem and the double-slash problem:
> 
> SPEC URL: 
> http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-2/keyutils.spec
> SRPM URL: 
> http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-2/keyutils.spec
> 
> The static library should be there as this library wraps some system calls 
> that aren't available through glibc.
Hmm? I fail to understand this, because your binaries are dynamically linked
against libkeyutils.so.

I am not going to approve this package in it's current shape.
Comment 6 David Woodhouse 2006-05-05 09:15:35 EDT
Should be OK if you drop the static library, and drop the weirdness with
$(RELEASE) in the library filename.

I don't think that using Uli's (weird, IMO) suggested library naming should be a
barrier to Extras approval, but there's no need for you to make it even
_weirder_ by including $(RELEASE) in the filename.
Comment 7 David Howells 2006-05-05 09:49:15 EDT
SPEC URL: 
http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-3/keyutils.spec
SRPM URL: 
http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-3/keyutils-1.1-3.src.rpm

That's removed the release number from the library filename and disabled the 
static library.

> Hmm? I fail to understand this, because your binaries are dynamically linked
> against libkeyutils.so.

The main user of these libraries is not intended to be my utilities; the main 
user is meant to be things like mount, PAM modules, NFS4, encrypted 
filesystems. Some of these things may require static tools to be installed to 
make a key available for an in-kernel filesystem or other service available, 
so that you can mend your system when it's broken.

It's possible that I *should* include a static version of keyctl and make 
request-key statically linked, but I'll defer that decision for now.
Comment 8 David Woodhouse 2006-05-05 17:22:03 EDT
(In reply to comment #7) 
> The main user of these libraries is not intended to be my utilities; the main 
> user is meant to be things like mount, PAM modules, NFS4, encrypted 
> filesystems. Some of these things may require static tools to be installed to 
> make a key available for an in-kernel filesystem or other service available, 
> so that you can mend your system when it's broken.

To my knowledge, none of those things are statically linked. We discourage
static linking in Fedora.
Comment 9 David Woodhouse 2006-05-12 09:45:15 EDT
If the static library is dropped, this package should be fine. Ralf?
Comment 10 Ralf Corsepius 2006-05-13 23:57:34 EDT
(In reply to comment #9)
> If the static library is dropped, this package should be fine. Ralf?
Well, I don't know.

The OP claimed there were technical requirements to ship the static libs. So,
from my (package review focused) perspective, a minimum requirement to proceed
would be him to either drop the static libs or to prove the necessity of
includeing them. 
I.e. yes, dropping the static lib would remove one road blocker.

The other issue (shared library names) is my personal preference (I consider
"Drepper-style" library naming useless featuritis), which will cause me not
approve a package. But it's not a technical problem, which should prevent others
from approving a package.

Comment 11 David Howells 2006-05-15 08:04:55 EDT
As stated in comment #7, I've disabled the building of the static library for 
the moment (I can always bring it back later if it becomes necessary), and 
I've removed the release number from the library name.
Comment 12 David Howells 2006-05-22 10:32:09 EDT
Is that sufficient, Ralf?
Comment 13 David Howells 2006-06-01 08:49:06 EDT
Ralf?
Comment 14 Jason Tibbitts 2006-06-02 21:52:09 EDT
David, do you have an updated SRPM?  Ralf never committed to a review but from
from reading this ticket it looks like the package should be fine.  I'll be
happy to do a formal review if I have the final SRPM to work with.
Comment 15 David Howells 2006-06-03 06:17:55 EDT
The SRPM linked to by comment #7 should be it: 
 
http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-3/keyutils-1.1-3.src.rpm  
  
Thanks,  
David  
Comment 16 Jason Tibbitts 2006-06-03 12:12:00 EDT
The Source0 URL doesn't exist.  You're the upstream so it doesn't seem to matter
much for the purposes of the review, but it would be good to make sure that the
source is always where the srpm says it is.

You install a shared libary into a system location but you don't call ldconfig.
 I believe this is a blocker.  

Review:
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
? source files match upstream (can't check)
? latest version is being packaged (I assume so since you're the upstream)
* BuildRequires are proper.
* package builds in mock (development, x86_64).
* rpmlint is silent.
* final provides and requires are sane:
  keyutils-1.1-3.fc6.x86_64.rpm
   config(keyutils) = 1.1-3.fc6
   keyutils = 1.1-3.fc6
  =
   /bin/sh
   config(keyutils) = 1.1-3.fc6
   libkeyutils.so.1()(64bit)
   libkeyutils.so.1(KEYUTILS_0.3)(64bit)
   libkeyutils.so.1(KEYUTILS_1.0)(64bit)

  keyutils-libs-1.1-3.fc6.x86_64.rpm
   libkeyutils.so.1()(64bit)
   libkeyutils.so.1(KEYUTILS_0.3)(64bit)
   libkeyutils.so.1(KEYUTILS_1.0)(64bit)
   keyutils-libs = 1.1-3.fc6
  =
   libkeyutils.so.1()(64bit)

  keyutils-libs-devel-1.1-3.fc6.x86_64.rpm
   keyutils-libs-devel = 1.1-3.fc6
  =
   keyutils-libs = 1.1-3.fc6
X shared libraries are present (in -libs subpackage) but ldconfig is not called.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* %clean is present.
* %check is not present; no test suite upstream.
? no scriptlets present (but there should be)
* code, not content.
* documentation is small, so no -docs subpackage is necessary. (Most of the
documentation is in the -devel subpackage).
* %docs are not necessary for the proper functioning of the package.
* headers are in -devel subpackage.
* no pkgconfig files.
* no libtool .la droppings.
* not a GUI app.
Comment 17 David Howells 2006-06-05 08:16:36 EDT
Okay, I've made the source tarball available at the Source0 URL and I've fixed 
the ldconfig lack.

SPEC URL: 
http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-4/keyutils.spec
SRPM URL: 
http://people.redhat.com/~dhowells/keyutils/keyutils-1.1-4/keyutils-1.1-4.src.rpm
Comment 18 Jason Tibbitts 2006-06-05 22:02:54 EDT
Looks good; the two issues I had are fixed.

APPROVED
Comment 19 Jason Tibbitts 2006-06-07 09:38:53 EDT
I didn't realize that you required sponsorship.  I've gone ahead and sponsored
your cvsextras membership; you should be ready to check in once your key
percolates through the system.
Comment 20 David Howells 2006-06-08 05:29:33 EDT
There doesn't seem to be any way to avoid the requirement for sponsorship. 
Anyway, thanks! I've now built my packages, though they don't seem to be 
available to yum yet.

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