Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 190664
Summary: | Review Request: keyutils - Kernel key management userspace utilities | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | David Howells <dhowells> |
Component: | Package Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | j, k.georgiou |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-06-08 09:40:08 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: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
David Howells
2006-05-04 11:25:10 UTC
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 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? (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). > 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. (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. 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. 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. (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. If the static library is dropped, this package should be fine. Ralf? (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. 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. Is that sufficient, Ralf? Ralf? 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. 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 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. 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 Looks good; the two issues I had are fixed. APPROVED 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. 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. |