[Bug 1745478] New: Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

Bug ID: 1745478
           Summary: Review Request: libbpf - The bpf library from kernel
                    source
           Product: Fedora
           Version: rawhide
          Hardware: All
                OS: Linux
            Status: NEW
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
  Target Milestone: ---
    Classification: Fedora

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

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

Dan Horák  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |[e-mail]
           Assignee|[e-mail]    |[e-mail]

--- Comment #1 from Dan Horák  ---
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-...)
- omit %dist in changelog

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

Dan Horák  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |fedora-review?

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

Iñaki Ucar  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail],
                   |                            |[e-mail]

--- Comment #2 from Iñaki Ucar  ---
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.

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

--- Comment #3 from Jiri Olsa  ---
(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

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

--- Comment #4 from Dan Horák  ---
For the record, it was Jiri who introduced the libbpf subpackage in
kernel-tools.

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

--- Comment #5 from Iñaki Ucar  ---
(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").

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

--- Comment #6 from Jiri Olsa  ---
(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

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

--- Comment #7 from Jiri Olsa  ---
(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

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

--- Comment #8 from Dan Horák  ---
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

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

--- Comment #9 from Jiri Olsa  ---
(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

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

Dan Horák  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-review?              |fedora-review+

--- Comment #10 from Dan Horák  ---
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.

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

--- Comment #11 from Jiri Olsa  ---
thanks, requesting repo in here:
https://pagure.io/releng/fedora-scm-requests/issue/17074

jirka

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

--- Comment #12 from Gwyn Ciesla  ---
(fedscm-admin):  The Pagure repository was created at
https://src.fedoraproject.org/rpms/libbpf

[Bug 1745478] Review Request: libbpf - The bpf library from kernel source

https://bugzilla.redhat.com/show_bug.cgi?id=1745478

Mattia Verga  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|---                         |ERRATA
        Last Closed|                            |2020-06-14 07:55:48

--- Comment #13 from Mattia Verga  ---
This package was approved and imported in repositories, but this review ticket
was never closed.
I'm closing it now.