| Summary: | Review Request: libnl3 - convenience library for kernel netlink API | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Dan Williams <dcbw> |
| Component: | Package Review | Assignee: | Dan Horák <dan> |
| Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | jpirko, mschmidt, notting, package-review, pwouters, tgraf, upstream-release-monitoring |
| Target Milestone: | --- | Flags: | dan:
fedora-review+
|
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2012-01-19 17:33:54 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Bug Depends On: | |||
| Bug Blocks: | 781458 | ||
|
Description
Dan Williams
2012-01-06 17:06:45 UTC
What about /etc/libnl/? We should be providing defaults for /etc/libnl/classid and /etc/libnl/pktloc I updated spec and libnl version as well. The main difference is to put libnl3 files into /usr/ and to include -cli as separate package: Spec diff URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec.diff Spec URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec SRPM URL: http://people.redhat.com/jpirko/libnl3/libnl3-3.2.5-1.fc16.src.rpm Successfully built in koji (http://koji.fedoraproject.org/koji/taskinfo?taskID=3642248) I successfully built and run my libteam library against this. Dan, please let me know if you will accept Jiri's updates to the spec or what version should I review. In any case the changelog should be truncated for libnl3. (In reply to comment #3) > I updated spec and libnl version as well. The main difference is to put libnl3 > files into /usr/ and to include -cli as separate package: > > Spec diff URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec.diff > Spec URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec > SRPM URL: http://people.redhat.com/jpirko/libnl3/libnl3-3.2.5-1.fc16.src.rpm > > Successfully built in koji > (http://koji.fedoraproject.org/koji/taskinfo?taskID=3642248) > > I successfully built and run my libteam library against this. We've put the libs into /lib to make sure we can still boot the system when /usr is network mounted; NetworkManager for example requires libnl and if it were in /usr we wouldn't be able to boot a network-mounted-/usr system. I wasn't quite sure what to do about the cli tools so thanks for handling that. Can we keep the libraries in /lib or /lib64 for now to match the packaging of libnl 1.x? *** Bug 689753 has been marked as a duplicate of this bug. *** (In reply to comment #5) > (In reply to comment #3) > > I updated spec and libnl version as well. The main difference is to put libnl3 > > files into /usr/ and to include -cli as separate package: > > > > Spec diff URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec.diff > > Spec URL: http://people.redhat.com/jpirko/libnl3/libnl3.spec > > SRPM URL: http://people.redhat.com/jpirko/libnl3/libnl3-3.2.5-1.fc16.src.rpm > > > > Successfully built in koji > > (http://koji.fedoraproject.org/koji/taskinfo?taskID=3642248) > > > > I successfully built and run my libteam library against this. > > We've put the libs into /lib to make sure we can still boot the system when > /usr is network mounted; NetworkManager for example requires libnl and if it > were in /usr we wouldn't be able to boot a network-mounted-/usr system. Well I was told that eventually all libs will end up to be in /usr/lib. So that was the reason why I changed libnl3 files to be there. Dan Horak told me that as well. Anyway, if you have strong feeling about them being in /lib, please move them there. > > I wasn't quite sure what to do about the cli tools so thanks for handling that. Well in debian for example they fragment this even more. They have separate package for nf, route, cli, utils as well. But I think that the partitioning I proposed is good enough. > Can we keep the libraries in /lib or /lib64 for now to match the packaging of > libnl 1.x? I'll defer to Bill here... Bill, do we care about network-mounted-/usr much any more? If we don't, then I'm fine with /usr/lib. We originally moved glib2 and libnl and NetworkManager to /bin and /lib to support that case way back before F7 I think. (In reply to comment #9) > http://fedoraproject.org/wiki/Features/UsrMove Ah, no problem with /usr in that case. (In reply to comment #4) > Dan, please let me know if you will accept Jiri's updates to the spec or what > version should I review. In any case the changelog should be truncated for > libnl3. Jiri's updated version looks good to me. When importing I'll truncate the changelog. Dan, anything else we should fix up from Jiri's specfile? Or are we good to go? (in which case, fedora-review+ greatly appreciated :) formal review is here, see the notes explaining OK* and BAD statuses below:
OK source files match upstream:
f7994bf67452e2c11fe55ce7f4caa18a4f23e37d libnl-3.2.5.tar.gz
OK package meets naming and versioning guidelines.
OK specfile is properly named, is cleanly written and uses macros consistently.
OK dist tag is present.
OK license field matches the actual license.
OK license is open source-compatible (LGPLv2). License text included in package.
OK latest version is being packaged.
OK BuildRequires are proper.
OK compiler flags are appropriate.
OK package builds in mock (Rawhide/i386).
OK debuginfo package looks complete.
OK* rpmlint is silent.
OK final provides and requires look sane.
N/A %check is present and all tests pass.
OK shared libraries are added to the regular linker search paths with correct scriptlets
BAD owns the directories it creates.
OK doesn't own any directories it shouldn't.
OK no duplicates in %files.
OK file permissions are appropriate.
OK correct scriptlets present.
OK code, not content.
BAD documentation is small, so no -docs subpackage is necessary.
OK %docs are not necessary for the proper functioning of the package.
OK headers in devel subpackage
OK pkgconfig files in devel subpackage.
OK no libtool .la droppings.
OK not a GUI app.
- you could probably use --disable-static instead of --enable-static=no, but it's more cosmetic issue
- rpmlint complains a bit
libnl3.src: W: spelling-error Summary(en_US) netlink -> net link, net-link, nestling
libnl3.src: W: spelling-error %description -l en_US netlink -> net link, net-link, nestling
libnl3.i686: W: spelling-error Summary(en_US) netlink -> net link, net-link, nestling
libnl3.i686: W: spelling-error %description -l en_US netlink -> net link, net-link, nestling
libnl3-cli.i686: W: spelling-error Summary(en_US) utils -> utile, utilizes, utilize
libnl3-cli.i686: W: spelling-error %description -l en_US utils -> utile, utilizes, utilize
=> can be ignored
libnl3.src:117: W: macro-in-%changelog %{_lib}
libnl3.src:117: W: macro-in-%changelog %{_libdir}
libnl3.src:173: W: macro-in-%changelog %{_libdir}
=> will be resolved by dropping the old changelog
libnl3.i686: E: incorrect-fsf-address /usr/share/doc/libnl3-3.2.5/COPYING
libnl3-cli.i686: E: incorrect-fsf-address /usr/share/doc/libnl3-cli-3.2.5/COPYING
=> upstream should fix this
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/cls/cgroup.so cgroup.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/cls/basic.so basic.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/htb.so htb.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/bfifo.so bfifo.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/pfifo.so pfifo.so.0.0.0
libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/blackhole.so blackhole.so.0.0.0
=> doesn't seem as a bug, but are those files libraries or dlopen()-ed plugins? Shouldn't they be linked with -avoid-version libtool flag?
libnl3.i686: W: shared-lib-calls-exit /usr/lib/libnl-route-3.so.199.5.1 exit
libnl3-cli.i686: W: shared-lib-calls-exit /usr/lib/libnl-cli-3.so.199.5.1 exit
=> needs a comment
libnl3-cli.i686: W: summary-not-capitalized C libnl3 command line interface utils
=> could be reworded to "Command line interface utils for libnl3"
libnl3-cli.i686: W: manual-page-warning /usr/share/man/man8/nl-qdisc-add.8.gz 2: warning: macro `LO' not defined
libnl3-cli.i686: W: manual-page-warning /usr/share/man/man8/nl-classid-lookup.8.gz 2: warning: macro `LO' not defined
libnl3-cli.i686: W: manual-page-warning /usr/share/man/man8/nl-pktloc-lookup.8.gz 2: warning: macro `LO' not defined
=> upstream issue
libnl3-cli.i686: W: no-manual-page-for-binary nl-class-list
libnl3-cli.i686: W: no-manual-page-for-binary nl-class-delete
libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-add
libnl3-cli.i686: W: no-manual-page-for-binary nl-class-add
libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-list
libnl3-cli.i686: W: no-manual-page-for-binary nl-link-list
libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-delete
=> would be nice to have
- the directories under %{_libdir} (libnl, libnl/cli/...) are not owned by the package although they should
- the documentation makes 99% of the devel subpackage and it should be packaged separately
(In reply to comment #12) > libnl3.i686: E: incorrect-fsf-address /usr/share/doc/libnl3-3.2.5/COPYING > libnl3-cli.i686: E: incorrect-fsf-address > /usr/share/doc/libnl3-cli-3.2.5/COPYING > => upstream should fix this Will do > > libnl3-devel.i686: W: dangling-relative-symlink > /usr/lib/libnl/cli/cls/cgroup.so cgroup.so.0.0.0 > libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/cls/basic.so > basic.so.0.0.0 > libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/htb.so > htb.so.0.0.0 > libnl3-devel.i686: W: dangling-relative-symlink > /usr/lib/libnl/cli/qdisc/bfifo.so bfifo.so.0.0.0 > libnl3-devel.i686: W: dangling-relative-symlink > /usr/lib/libnl/cli/qdisc/pfifo.so pfifo.so.0.0.0 > libnl3-devel.i686: W: dangling-relative-symlink > /usr/lib/libnl/cli/qdisc/blackhole.so blackhole.so.0.0.0 > => doesn't seem as a bug, but are those files libraries or dlopen()-ed > plugins? Shouldn't they be linked with -avoid-version libtool flag? Those are dlopen() plugins. They are currently compiled like this: cli_qdisc_htb_la_LDFLAGS = -module -version-info 0:0:0 I will change it to -avoid-version. Thanks! > libnl3.i686: W: shared-lib-calls-exit /usr/lib/libnl-route-3.so.199.5.1 > exit This I do not understand. git grep exit also does not show anything unusual. > libnl3-cli.i686: W: shared-lib-calls-exit /usr/lib/libnl-cli-3.so.199.5.1 > exit > => needs a comment The CLI libraries do issue exit(), f.e. when parsing the argument string requests printing the help text. > libnl3-cli.i686: W: manual-page-warning /usr/share/man/man8/nl-qdisc-add.8.gz > 2: warning: macro `LO' not defined > libnl3-cli.i686: W: manual-page-warning > /usr/share/man/man8/nl-classid-lookup.8.gz 2: warning: macro `LO' not defined > libnl3-cli.i686: W: manual-page-warning > /usr/share/man/man8/nl-pktloc-lookup.8.gz 2: warning: macro `LO' not defined > => upstream issue Will fix those. > libnl3-cli.i686: W: no-manual-page-for-binary nl-class-list > libnl3-cli.i686: W: no-manual-page-for-binary nl-class-delete > libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-add > libnl3-cli.i686: W: no-manual-page-for-binary nl-class-add > libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-list > libnl3-cli.i686: W: no-manual-page-for-binary nl-link-list > libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-delete > => would be nice to have Volunteers needed :-) (In reply to comment #13) > (In reply to comment #12) > > > libnl3-devel.i686: W: dangling-relative-symlink > > /usr/lib/libnl/cli/cls/cgroup.so cgroup.so.0.0.0 > > libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/cls/basic.so > > basic.so.0.0.0 > > libnl3-devel.i686: W: dangling-relative-symlink /usr/lib/libnl/cli/qdisc/htb.so > > htb.so.0.0.0 > > libnl3-devel.i686: W: dangling-relative-symlink > > /usr/lib/libnl/cli/qdisc/bfifo.so bfifo.so.0.0.0 > > libnl3-devel.i686: W: dangling-relative-symlink > > /usr/lib/libnl/cli/qdisc/pfifo.so pfifo.so.0.0.0 > > libnl3-devel.i686: W: dangling-relative-symlink > > /usr/lib/libnl/cli/qdisc/blackhole.so blackhole.so.0.0.0 > > => doesn't seem as a bug, but are those files libraries or dlopen()-ed > > plugins? Shouldn't they be linked with -avoid-version libtool flag? > > Those are dlopen() plugins. They are currently compiled like this: > > cli_qdisc_htb_la_LDFLAGS = -module -version-info 0:0:0 > > I will change it to -avoid-version. Thanks! will also need some changes in the packaging, like filtering the Provides > > libnl3.i686: W: shared-lib-calls-exit /usr/lib/libnl-route-3.so.199.5.1 > > exit > > This I do not understand. git grep exit also does not show anything unusual. will try to find out what's rpmlint doing here > > libnl3-cli.i686: W: shared-lib-calls-exit /usr/lib/libnl-cli-3.so.199.5.1 > > exit > > => needs a comment > > The CLI libraries do issue exit(), f.e. when parsing the argument string > requests printing the help text. OK > > libnl3-cli.i686: W: no-manual-page-for-binary nl-class-list > > libnl3-cli.i686: W: no-manual-page-for-binary nl-class-delete > > libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-add > > libnl3-cli.i686: W: no-manual-page-for-binary nl-class-add > > libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-list > > libnl3-cli.i686: W: no-manual-page-for-binary nl-link-list > > libnl3-cli.i686: W: no-manual-page-for-binary nl-cls-delete > > => would be nice to have > > Volunteers needed :-) it's not a blocker, so it can wait for the right volunteer :-) Tomas issued 3.2.6 which fixes many of warnings. Working on re-spec that. Updated: Spec URL: http://people.redhat.com/jpirko/libnl3_v2/libnl3.spec SRPM URL: http://people.redhat.com/jpirko/libnl3_v2/libnl3-3.2.6-1.fc16.src.rpm All issues look to be resolved or explained, package is APPROVED. Only change the Group for the cli sub-package to Applications/System before uploading to dist-git. (In reply to comment #17) > Only change the Group for the cli sub-package to Applications/System before > uploading to dist-git. lib is there as well (I link that in libteam). As I discussed with Dan Horak on irc, this change is not desirable then. So please leave that as it is. Thanks. New Package SCM Request ======================= Package Name: libnl3 Short Description: Convenience library for kernel netlink sockets Owners: dcbw jirka Branches: f15 f16 InitialCC: Git done (by process-git-requests). Imported to git and built. Thanks everyone! Package Change Request ====================== Package Name: libnl3 New Branches: el6 Owners: pwouters InitialCC: Any comments from primary maintainers? sorry. I didn't spot it was already in rhel itself..... |