Re: [Buildroot] [PATCHv2 2/2] package/efitools: efitools is a set of utilities to manipulate efi variables

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Guillaume,

On Thu,  5 Sep 2024 19:26:07 +0200
"Guillaume GC. Chaye" <guillaume.chaye@zeetim.com> wrote:

> Signed-off-by: Guillaume GC. Chaye <guillaume.chaye@zeetim.com>
> ---
>  package/efitools/Config.in   |  6 ++++++
>  package/efitools/efitools.mk | 24 ++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
>  create mode 100644 package/efitools/Config.in
>  create mode 100644 package/efitools/efitools.mk

Thanks for this patch! I have a number of comments.

First of all, your patch does not really add the package completely,
because you forgot the change in package/Config.in, to include
package/efitools/Config.in.

Then, you also need to add an entry in the DEVELOPERS file for this new
package.

> 
> diff --git a/package/efitools/Config.in b/package/efitools/Config.in
> new file mode 100644
> index 0000000000..acc41cd49b
> --- /dev/null
> +++ b/package/efitools/Config.in
> @@ -0,0 +1,6 @@
> +config BR2_PACKAGE_EFITOOLS
> +	bool "efitools"
> +    select BR2_PACKAGE_GNU_EFI
> +    select BR2_PACKAGE_OPENSSL
> +    help

The indentation of those lines is incorrect. If you run "make
check-package", you will have a coding style check that will spot this
kind of issues.

Also, when you "select" an option, you need to replicate its
dependencies. In this case, you need to replicate:

	depends on BR2_PACKAGE_GNU_EFI_ARCH_SUPPORTS

due to the fact that you select BR2_PACKAGE_GNU_EFI.

> +        A set of utilities to manipulate efi variables
> \ No newline at end of file

Please add a new line at the end of the file.

You also need to add the upstream URL of the project in the help text.
See the help texts of other packages for examples of this.

> diff --git a/package/efitools/efitools.mk b/package/efitools/efitools.mk
> new file mode 100644
> index 0000000000..5b249e81e4
> --- /dev/null
> +++ b/package/efitools/efitools.mk
> @@ -0,0 +1,24 @@
> +################################################################################
> +#
> +# efitools
> +#
> +################################################################################
> +
> +EFITOOLS_VERSION = 1.9.2
> +EFITOOLS_SITE = https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git/snapshot
> +EFITOOLS_SOURCE = efitools-$(EFITOOLS_VERSION).tar.gz

This EFITOOLS_SOURCE is not needed, as
efitools-$(EFITOOLS_VERSION).tar.gz is anyway its default value.

> +EFITOOLS_LICENSE = GPL-2.0+, LGPL-2.1+
> +EFITOOLS_DEPENDENCIES = openssl gnu-efi host-perl-file-slurp
> +
> +EFITOOLS_INCDIR=-I$(@D)/include/ -I$(STAGING_DIR)/usr/include/efi -I$(STAGING_DIR)/usr/include/efi/$(ARCH) -I$(STAGING_DIR)/usr/include/efi/protocol

Why is this needed?

> +EFITOOLS_LIB_INCDIR=-I$(TARGET_DIR)/usr/include

This is wrong, TARGET_DIR does not have header files, so pointing to
TARGET_DIR/usr/include for header files is always incorrect.

> +
> +define EFITOOLS_BUILD_CMDS
> +$(TARGET_MAKE_ENV) INCDIR="$(EFITOOLS_LIB_INCDIR)" $(MAKE) -C $(@D) INCDIR="$(EFITOOLS_INCDIR)"

Please indent with one tab.

But this will not cross-compile the code: you are not even telling to
use the cross-compiler. The canonical invocation is:

	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D)

indeed $(TARGET_CONFIGURE_OPTS) defines CC, LD, AS, AR, CFLAGS,
LDFLAGS, and a whole bunch of other useful variables.

And then starting from that, depending on how the efitools Makefile is
written, additional tweaks may be needed.

> +endef
> +
> +define EFITOOLS_INSTALL_TARGET_CMDS
> +$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) install DESTDIR=$(TARGET_DIR)

Indentation with one tab here.

> +endef
> +
> +$(eval $(generic-package))
> \ No newline at end of file

Please add the newline at end of the file.

Also, could you test this new package with ./utils/test-pkg ?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot