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

[Arnout Vandecappelle]

  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 at 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 at je-eigen-domein.nl>
>   F:	package/ipmitool/
>   F:	package/odhcploc/
>   
> +N:	Francis Laniel <flaniel at linux.microsoft.com>
> +F:	package/pahole/
> +
>   N:	Francisco Gonzalez <gzmorell at 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.

> +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.

> +# 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.

  Regards,
  Arnout


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