Bug 1745478 - Review Request: libbpf - The bpf library from kernel source
Summary: Review Request: libbpf - The bpf library from kernel source
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dan Horák
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-08-26 08:12 UTC by Jiri Olsa
Modified: 2020-06-14 07:55 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-06-14 07:55:48 UTC
Type: ---
dan: fedora-review+


Attachments (Terms of Use)

Description Jiri Olsa 2019-08-26 08:12:11 UTC
Spec URL: http://people.redhat.com/~jolsa/libbpf/libbpf.spec
SRPM URL: http://people.redhat.com/~jolsa/libbpf/libbpf-0.0.3-1.fc30.src.rpm
Description:
A mirror of bpf-next linux tree bpf-next/tools/lib/bpf directory plus its
supporting header files. The version of the package reflects the version of
ABI.

Fedora Account System Username: jolsa

Comment 1 Dan Horák 2019-08-27 07:32:23 UTC
first round of comments
- drop Group: - it's ignored
- drop Packager: - filled by the build system
- drop %clean - rpmbuild does it automagically
- drop all %attr() - rpmbuild set them automagically
- License tag must use Fedora identifiers - "LGPLv2 or BSD"
- %{_libdir}/libbpf.so belongs to -devel subpackage
- -devel subpackage should own %{_includedir}/bpf directory, you might use wildcard to include the individual headers, or even list only the "bpf" directory
- ideally the static library shouldn't be packaged, if it's a must, then it belong to a separate -static subpackage (https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries)
- omit %dist in changelog

Comment 2 Iñaki Ucar 2019-08-27 10:22:56 UTC
Why is this superseding libbpf in kernel-tools? Won't it break compatibility with the official kernel?

Adding the maintainer of kernel-tools to the loop.

Comment 3 Jiri Olsa 2019-08-27 11:45:15 UTC
(In reply to Iñaki Ucar from comment #2)
> Why is this superseding libbpf in kernel-tools? Won't it break compatibility
> with the official kernel?
> 
> Adding the maintainer of kernel-tools to the loop.

the true source of libbpf is kernel tree, that's why it
initially landed in kernel-tools with other kernel tree
libraries

however, upstream team decided to keep and maintain GitHub libbpf
sources (which are regirarly sync from kernel tree) to maintain
compatibility in between distributions and mainly to have stable
relases of libbpf based on github tree tags rather then on random
kernel versions (from libbpf point of view)

jirka

Comment 4 Dan Horák 2019-08-27 12:42:07 UTC
For the record, it was Jiri who introduced the libbpf subpackage in kernel-tools.

Comment 5 Iñaki Ucar 2019-08-27 12:59:48 UTC
(In reply to Jiri Olsa from comment #3)
> however, upstream team decided to keep and maintain GitHub libbpf
> sources (which are regirarly sync from kernel tree) to maintain
> compatibility in between distributions and mainly to have stable
> relases of libbpf based on github tree tags rather then on random
> kernel versions (from libbpf point of view)

But that GitHub mirror follows bpf-next, right? So the question is whether it could happen that an update of the standalone libbpf has a soname bump that makes it incompatible with the kernel versions packaged in Fedora (which cannot happen if libbpf is a subpackage of kernel-tools, because it has the same versioning and provides from the same "snapshot").

Comment 6 Jiri Olsa 2019-08-27 13:09:35 UTC
(In reply to Iñaki Ucar from comment #5)
> (In reply to Jiri Olsa from comment #3)
> > however, upstream team decided to keep and maintain GitHub libbpf
> > sources (which are regirarly sync from kernel tree) to maintain
> > compatibility in between distributions and mainly to have stable
> > relases of libbpf based on github tree tags rather then on random
> > kernel versions (from libbpf point of view)
> 
> But that GitHub mirror follows bpf-next, right? So the question is whether

right

> it could happen that an update of the standalone libbpf has a soname bump
> that makes it incompatible with the kernel versions packaged in Fedora
> (which cannot happen if libbpf is a subpackage of kernel-tools, because it
> has the same versioning and provides from the same "snapshot").

well at the moment nothing depends on libbpf and libbpf itself
checks on kernel features before using them, so I don't see any
possible incompatibility issue

new libbpf has the 'Epoch:1' set, so it takes over old libbpf smoothly

jirka

Comment 7 Jiri Olsa 2019-08-27 13:28:35 UTC
(In reply to Dan Horák from comment #1)
> first round of comments
> - drop Group: - it's ignored
> - drop Packager: - filled by the build system
> - drop %clean - rpmbuild does it automagically
> - drop all %attr() - rpmbuild set them automagically
> - License tag must use Fedora identifiers - "LGPLv2 or BSD"
> - %{_libdir}/libbpf.so belongs to -devel subpackage
> - -devel subpackage should own %{_includedir}/bpf directory, you might use
> wildcard to include the individual headers, or even list only the "bpf"
> directory
> - ideally the static library shouldn't be packaged, if it's a must, then it
> belong to a separate -static subpackage
> (https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-
> libraries)
> - omit %dist in changelog

I put new version (spec and srpm) in here:
  http://people.redhat.com/~jolsa/libbpf/v2/

thanks,
jirka

Comment 8 Dan Horák 2019-09-10 11:48:50 UTC
further comments
- missing BuildRequires: gcc
- fails to rebuild on ppc64le, because it installs the libs into /usr/lib/ - https://github.com/libbpf/libbpf/blob/master/src/Makefile#L57 is wrong
- distro-wide CFLAGS and LDFLAGS are not used, adding CFLAGS=%{build_cflags} LDFLAGS=%{build_ldflags} to "make" might help

Comment 9 Jiri Olsa 2019-09-13 22:31:06 UTC
(In reply to Dan Horák from comment #8)
> further comments
> - missing BuildRequires: gcc
> - fails to rebuild on ppc64le, because it installs the libs into /usr/lib/ -
> https://github.com/libbpf/libbpf/blob/master/src/Makefile#L57 is wrong
> - distro-wide CFLAGS and LDFLAGS are not used, adding CFLAGS=%{build_cflags}
> LDFLAGS=%{build_ldflags} to "make" might help

new version is in here:
  http://people.redhat.com/~jolsa/libbpf/v3/

thanks,
jirka

Comment 10 Dan Horák 2019-09-16 11:59:06 UTC
formal review is here, see the notes explaining OK* and BAD statuses below:

OK      source files match upstream:
            105e4914bc7b9cb29a5b81be51693e7567405478  v0.0.3.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 or BSD). License text not included in package.
OK      latest version is being packaged.
OK      BuildRequires are proper.
OK      compiler flags are appropriate.
OK      package builds in mock (Rawhide/ppc64le).
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.
OK      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      no scriptlets present.
OK      code, not content.
OK      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.

- the license texts should included in the source archive and then distributed with the built rpms
- rpmlint output is harmless
libbpf-devel.ppc64le: W: no-documentation
libbpf-static.ppc64le: W: no-documentation
libbpf.ppc64le: W: name-repeated-in-summary C Libbpf
libbpf.ppc64le: W: spelling-error %description -l en_US bpf -> bf, pf, bps
libbpf.ppc64le: W: incoherent-version-in-changelog 0.0.3-1 ['1:0.0.3-1.fc32', '1:0.0.3-1']
libbpf.ppc64le: W: no-documentation
libbpf.src: W: name-repeated-in-summary C Libbpf
libbpf.src: W: spelling-error %description -l en_US bpf -> bf, pf, bps
6 packages and 0 specfiles checked; 0 errors, 8 warnings.

Two nitpicks
- you can drop the slash from the LIBDIR definition, the "dir" macros start with a slash
- you can drop the redefinition of OBJDIR in the make_flags, actually it makes it worse when doing a local "rpmbuild" (it puts files into /home/dan/rpmbuild/BUILD which isn't cleaned up). It works well for me when the default is used.

The package is APPROVED.

Comment 11 Jiri Olsa 2019-09-23 08:54:06 UTC
thanks, requesting repo in here:
https://pagure.io/releng/fedora-scm-requests/issue/17074

jirka

Comment 12 Gwyn Ciesla 2019-09-23 13:24:52 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/libbpf

Comment 13 Mattia Verga 2020-06-14 07:55:48 UTC
This package was approved and imported in repositories, but this review ticket was never closed.
I'm closing it now.


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