Re: [OE-core] [PATCH] recipes-kernel: add libbpf

["Bruce Ashfield" <bruce.ashfield@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6603 bytes --]

On Wed, Feb 17, 2021 at 9:32 PM Matteo Croce <mcroce@linux.microsoft.com>
wrote:

> On Mon, Feb 8, 2021 at 9:13 PM Bruce Ashfield <bruce.ashfield@gmail.com>
> wrote:
> >
> > On Mon, Feb 8, 2021 at 1:18 PM Matteo Croce <mcroce@linux.microsoft.com>
> wrote:
> > >
> > > From: Matteo Croce <mcroce@microsoft.com>
> > >
> > > Add a recipe to build libbpf from https://github.com/libbpf/libbpf
> > > The only patch fixes a build issue, and it's already merged upstream.
> >
> > Thanks for the submission! I have a few comments / questions.
> >
> > To get this into oe-core, we should be commenting / documenting why it
> > should be in core, versus another layer. The standard criteria is that
> > there are enough varied users and that the functionality is common
> > enough, that it belongs in core.
> >
> > There should also be some sort of oe-selftest for the functionality,
> > otherwise, it is hard to detect breakages. Some sort of application
> > that uses the library and that can be executed in qemu would be
> > enough.
> >
> > What are the kernel requirements ? CONFIG_BPF is selected by other
> > kernel configs (it has no menu entry, so it must be), is it that, or
> > something else that is the requirement (classic BFP?). If that option
> > is now always on, is that true for the reference kernel versions in
> > master (5.4 and 5.10).
> >
>
> Something like this will work? I see a similar trick in the uprobe recipe:
>
> do_compile() {
>  if grep -q "CONFIG_BPF_SYSCALL=y" ${STAGING_KERNEL_BUILDDIR}/.config
>  then
>   oe_runmake
>  else
>   bbnote "BFP syscall is not enabled"
>  fi
> }
>
>
Since it already depends on virtual/kernel, that will indeed work. I can't
say
that I'm a huge fan of checking the .config, since it leaves us open to
changing config option names, but that is way less common now than it
used to be, so it isn't a problem as a basic sanity check. As you mention,
uprobe already sets the prior art on it anyway :D



>
> > Finally, does this work across all the supported architectures ? if
> > not, we'll need compatibility settings.
> >
>
> I'm adding this:
>
> COMPATIBLE_HOST = "(x86_64.*|i.86.*|aarch64).*-linux"
>
>
That's reasonable. Interested folks can add other architectures if they
are willing to help out and support.



> > >
> > > Signed-off-by: Matteo Croce <mcroce@microsoft.com>
> > > ---
> > >  ...01-install-don-t-preserve-file-owner.patch | 29 +++++++++++++++++++
> > >  meta/recipes-kernel/libbpf/libbpf_0.3.bb      | 25 ++++++++++++++++
> > >  2 files changed, 54 insertions(+)
> > >  create mode 100644
> meta/recipes-kernel/libbpf/libbpf/0001-install-don-t-preserve-file-owner.patch
> > >  create mode 100644 meta/recipes-kernel/libbpf/libbpf_0.3.bb
> > >
> > > diff --git
> a/meta/recipes-kernel/libbpf/libbpf/0001-install-don-t-preserve-file-owner.patch
> b/meta/recipes-kernel/libbpf/libbpf/0001-install-don-t-preserve-file-owner.patch
> > > new file mode 100644
> > > index 0000000000..4e65d8d80a
> > > --- /dev/null
> > > +++
> b/meta/recipes-kernel/libbpf/libbpf/0001-install-don-t-preserve-file-owner.patch
> > > @@ -0,0 +1,29 @@
> > > +From 7df10d91db6f533cc0f6c09f4ae8ad92918c6160 Mon Sep 17 00:00:00 2001
> > > +From: Matteo Croce <mcroce@microsoft.com>
> > > +Date: Tue, 26 Jan 2021 12:41:47 +0100
> > > +Subject: [PATCH] install: don't preserve file owner
> > > +
> > > +'cp -p' preserve file ownership, this may leave files owned by the
> > > +current in user in /lib .
> > > +
> >
> > We need the upstream status documented here, not just in the commit
> message.
> >
>
> You want the upstream commit, or just saying that it will go into 0.4 is
> enough?
>
>
We normally do something like:

Upstream-status: submitted (0.4 release) <link to the pull request or
mailing list submission>

That way it is abundantly clear that we aren't carrying it ourselves, and
when we can drop it.



> > > +Signed-off-by: Matteo Croce <mcroce@microsoft.com>
> > > +---
> > > + Makefile | 2 +-
> > > + 1 file changed, 1 insertion(+), 1 deletion(-)
> > > +
> > > +diff --git a/Makefile b/Makefile
> > > +index da33613..ab66edc 100644
> > > +--- a/Makefile
> > > ++++ b/Makefile
> > > +@@ -130,7 +130,7 @@ define do_s_install
> > > +       $(Q)if [ ! -d '$(DESTDIR)$2' ]; then            \
> > > +               $(INSTALL) -d -m 755 '$(DESTDIR)$2';    \
> > > +       fi;
> > > +-      $(Q)cp -fpR $1 '$(DESTDIR)$2'
> > > ++      $(Q)cp -fR $1 '$(DESTDIR)$2'
> > > + endef
> > > +
> > > + install: all install_headers install_pkgconfig
> > > +--
> > > +2.29.2
> > > +
> > > diff --git a/meta/recipes-kernel/libbpf/libbpf_0.3.bb
> b/meta/recipes-kernel/libbpf/libbpf_0.3.bb
> > > new file mode 100644
> > > index 0000000000..402e57257f
> > > --- /dev/null
> > > +++ b/meta/recipes-kernel/libbpf/libbpf_0.3.bb
> > > @@ -0,0 +1,25 @@
> > > +SUMMARY = "Library for BPF handling"
> > > +DESCRIPTION = "Library for BPF handling"
> > > +HOMEPAGE = "https://github.com/libbpf/libbpf"
> > > +SECTION = "libs"
> > > +LICENSE = "LGPLv2.1+"
> > > +
> > > +LIC_FILES_CHKSUM =
> "file://../LICENSE.LPGL-2.1;md5=b370887980db5dd40659b50909238dbd"
> > > +
> > > +DEPENDS = "zlib elfutils"
> > > +
> > > +SRC_URI = "git://github.com/libbpf/libbpf.git;protocol=https"
> > > +SRCREV = "051a4009f94d5633a8f734ca4235f0a78ee90469"
> > > +
> > > +# Backported from version 0.4
> > > +SRC_URI += "file://0001-install-don-t-preserve-file-owner.patch"
> > > +
> > > +S = "${WORKDIR}/git/src"
> > > +
> > > +inherit pkgconfig
> >
> > I see inherit pkgconfig, but no PACKAGECONFIG setting. Is it actually
> > used here ?
> >
> > > +
> > > +do_install() {
> > > +       make install DESTDIR=${D} LIBDIR=${libdir}
> >
> > Rather than a bare 'make' call, this should likely be oe_runmake
> >
>
> Right.
> While at it, what about adding DESTDIR and LIBDIR to ${EXTRA_OEMAKE}?
> They are ignored in the build phase
>
>
yep. that is doable. oe_runmake will pass them along if in the right
variable.

Bruce



> > Bruce
> >
> > > +}
> > > +
> > > +BBCLASSEXTEND = "native"
> > > --
> > > 2.29.2
> > >
> > >
> > > 
> > >
> >
> >
> > --
> > - Thou shalt not follow the NULL pointer, for chaos and madness await
> > thee at its end
> > - "Use the force Harry" - Gandalf, Star Trek II
>
> Regards,
> --
> per aspera ad upstream
>


-- 
- Thou shalt not follow the NULL pointer, for chaos and madness await thee
at its end
- "Use the force Harry" - Gandalf, Star Trek II

[-- Attachment #2: Type: text/html, Size: 10750 bytes --]