Re: [Buildroot] [RFC PATCH v1 1/2] package/pahole: new host package

[Francis Laniel <flaniel@linux.microsoft.com>]

Hi.


Thank you for your reviews, I comment some inline.

Le mardi 21 décembre 2021 22:44:56 CET, vous avez écrit :
>   Hi Francis,
> 
>   Some relatively minor nitpicks.
> 
> On 21/12/2021 15:54, Francis Laniel wrote:
> > pahole is a tool used to show data structure embedded in debugging
> > information formats like DWARF.
> > It is notably needed by the Linux kernel to generate BPF Type Format (BTF)
> > information used by Compile Once - Run Everywhere (CO-RE) BPF tools.
> > 
> > Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
> > ---
> > 
> >   DEVELOPERS                    |  3 +++
> >   package/Config.in.host        |  1 +
> >   package/pahole/Config.in.host |  6 ++++++
> >   package/pahole/pahole.hash    |  2 ++
> >   package/pahole/pahole.mk      | 20 ++++++++++++++++++++
> >   5 files changed, 32 insertions(+)
> >   create mode 100644 package/pahole/Config.in.host
> >   create mode 100644 package/pahole/pahole.hash
> >   create mode 100644 package/pahole/pahole.mk
> > 
> > diff --git a/DEVELOPERS b/DEVELOPERS
> > index 64db6c51d0..70df957415 100644
> > --- a/DEVELOPERS
> > +++ b/DEVELOPERS
> > @@ -939,6 +939,9 @@ N:	Floris Bos <bos@je-eigen-domein.nl>
> > 
> >   F:	package/ipmitool/
> >   F:	package/odhcploc/
> > 
> > +N:	Francis Laniel <flaniel@linux.microsoft.com>
> > +F:	package/pahole/
> > +
> > 
> >   N:	Francisco Gonzalez <gzmorell@gmail.com>
> >   F:	package/ser2net/
> > 
> > diff --git a/package/Config.in.host b/package/Config.in.host
> > index 6e5a5c5fc5..ae512c5643 100644
> > --- a/package/Config.in.host
> > +++ b/package/Config.in.host
> > @@ -60,6 +60,7 @@ menu "Host utilities"
> > 
> >   	source "package/omap-u-boot-utils/Config.in.host"
> >   	source "package/openocd/Config.in.host"
> >   	source "package/opkg-utils/Config.in.host"
> > 
> > +	source "package/pahole/Config.in.host"
> > 
> >   	source "package/parted/Config.in.host"
> >   	source "package/patchelf/Config.in.host"
> >   	source "package/pigz/Config.in.host"
> > 
> > diff --git a/package/pahole/Config.in.host b/package/pahole/Config.in.host
> > new file mode 100644
> > index 0000000000..e427629632
> > --- /dev/null
> > +++ b/package/pahole/Config.in.host
> > @@ -0,0 +1,6 @@
> > +config BR2_PACKAGE_HOST_PAHOLE
> > +	bool "host pahole"
> > +	help
> > +	  Pahole and other DWARF utils.
> > +
> > +	  https://git.kernel.org/pub/scm/devel/pahole/pahole.git
> > diff --git a/package/pahole/pahole.hash b/package/pahole/pahole.hash
> > new file mode 100644
> > index 0000000000..2573fde8c9
> > --- /dev/null
> > +++ b/package/pahole/pahole.hash
> > @@ -0,0 +1,2 @@
> > +# Locally computed
> > +sha256 76b7eaf5747dbb7250a1a50185136d4639e0d70aa11c5d7c68139c0c8ca9be80
> > pahole-v1.22-br1.tar.gz
>   You should also add COPYING to the hash file.
> 
> > diff --git a/package/pahole/pahole.mk b/package/pahole/pahole.mk
> > new file mode 100644
> > index 0000000000..1f69f5391e
> > --- /dev/null
> > +++ b/package/pahole/pahole.mk
> > @@ -0,0 +1,20 @@
> > +########################################################################
> > +#
> > +# pahole
> > +#
> > +########################################################################
> > +
> > +PAHOLE_VERSION = v1.22
> 
>   There's a v1.23 now.

Thank you for it, I jumped to v1.23.

> > +PAHOLE_SITE = git://git.kernel.org/pub/scm/devel/pahole/pahole.git
> 
>   kernel.org also has a tarball download:
> 
> PAHOLE_SITE =
> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/snapshot
> 
> (cfr. e.g. f2fs-tools).
> 
>   That also allows you to set VERSION without the v, which we prefer because
> that way it can be used in CPE info and in release-monitoring.
> 
> > +PAHOLE_SITE_METHOD = git
> > +# pahole contains git submodule and relies on them to be built.
> 
>   Darn, so much for the tarball download :-(
> 
>   This is something we'd typically put in a comment or the commit message so
> later down the line people remember why we didn't choose a tarball
> download.
> 
>   However, we normally prefer to unbundle dependencies, and we already have
> libbpf (though not for the host at the moment). For host packages, the
> unbundling isn't terribly important, so if it's difficult, don't bother. But
> if it's easy to use an external bpf rather than the submodule, then please
> do that.

I tried to modify libbpf recipe to add host-libbpf and add it at dependencies 
to host-pahole but sadly I was not successful.
Thus, I stick with getting pahole from its git repository with its submodules.

> > +# We need to add this option to fetch the submodules before creating the
> > +# archive.
> 
>   This comment is redundant, the first sentence was enough.
> 
> > +PAHOLE_GIT_SUBMODULES = YES
> > +# Better to build it statically so we do not rely on the host having
> > +# corresponding libraries.
> 
>   This doesn't make a whole lot of sense to me... We're building it, so the
> libraries are there, otherwise it would fail to build, right? Again, not
> terribly important for a host package, but we prefer to do special stuff if
> not needed.
> 
> > +HOST_PAHOLE_CONF_OPTS = -DBUILD_SHARED_LIBS=OFF -D__LIB=lib
> 
>   Why is the __LIB=lib needed? What does it do? It's enough to mention this
> in the commit message.

This seems needed as directory for some build artifact, it is described in 
pahole README to build it.
I added a comment in pahole.mk as well as in commit message.

>   Regards,


Best regards.
>   Arnout
> 
> > +PAHOLE_LICENSE = GPL-2.0
> > +PAHOLE_LICENSE_FILES = COPYING
> > +
> > +$(eval $(host-cmake-package))




_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot