From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Guillaume Chaye <guillaume.chaye@zeetim.com>
Cc: buildroot@buildroot.org, Christopher McCrory <chrismcc@gmail.com>
Subject: Re: [Buildroot] [PATCH 2/2] package/efitools: efitools is a set of utilities to manipulate efi variables.
Date: Fri, 16 May 2025 21:14:49 +0200 [thread overview]
Message-ID: <20250516211449.0e04ef51@windsurf> (raw)
In-Reply-To: <20250325134609.610846-2-guillaume.chaye@zeetim.com>
Hello Guillaume,
On Tue, 25 Mar 2025 09:46:09 -0400
Guillaume Chaye <guillaume.chaye@zeetim.com> wrote:
> Signed-off-by: Guillaume Chaye <guillaume.chaye@zeetim.com>
The commit title should be:
package/efitools: new package
without any full stop ("point final" in French) at the end.
> diff --git a/package/efitools/0001-Add-EXTRA_LDFLAGS-variable-to-Makefile.patch b/package/efitools/0001-Add-EXTRA_LDFLAGS-variable-to-Makefile.patch
> new file mode 100644
> index 0000000000..d931e9965b
> --- /dev/null
> +++ b/package/efitools/0001-Add-EXTRA_LDFLAGS-variable-to-Makefile.patch
> @@ -0,0 +1,61 @@
> +From 4847a5a8a46462ee3c6386e2333fb2be87fb9ae6 Mon Sep 17 00:00:00 2001
> +From: Guillaume Chaye <guillaume.chaye@zeetim.com>
> +Date: Fri, 24 Jan 2025 10:05:27 +0100
> +Subject: [PATCH] efitools: Add EXTRA_LDFLAGS variable to Makefile
> +
> +This patch allows to build binaries with proper rpath
> +
> +Signed-off-by: Guillaume Chaye <guillaume.chaye@zeetim.com>
> +Upstream: N/A
There is an upstream. Not very active yes, but there is an upstream.
The upstream maintainer, James Bottomley is still very active in the
Linux kernel community and he has replied on the mailing list not long
ago.
See:
https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git/tree/README#n7
on how to contribute.
Also,
https://github.com/Wind-River/meta-secure-core/tree/master/meta-efi-secure-boot/recipes-bsp/efitools/efitools
is a layer that also packages efitools, and which has plenty of patches
doing more or less the same as yours. Could be a good idea to re-use
some of them.
> diff --git a/package/efitools/0003-Remove-EFISIGNED-variable-from-makefiles.patch b/package/efitools/0003-Remove-EFISIGNED-variable-from-makefiles.patch
> new file mode 100644
> index 0000000000..92d2c898e4
> --- /dev/null
> +++ b/package/efitools/0003-Remove-EFISIGNED-variable-from-makefiles.patch
> @@ -0,0 +1,31 @@
> +From f7b880dedc4f66c674406687fb6d357b146892ee Mon Sep 17 00:00:00 2001
> +From: Guillaume Chaye <guillaume.chaye@zeetim.com>
> +Date: Fri, 24 Jan 2025 11:55:05 +0100
> +Subject: [PATCH] efitools: remove EFISIGNED variable from makefiles
> +
> +Efitools sign efi files with a key generated during build.
> +We disable it to remove a build dependencie to sbsigntools.
> +These signed files were not actually installed to the target.
> +
> +Signed-off-by: Guillaume Chaye <guillaume.chaye@zeetim.com>
> +Upstream: N/A
I guess this patch will not be acceptable upstream.
> diff --git a/package/efitools/0004-Add-option-to-efi-updatevar-to-read-from-stdin.patch b/package/efitools/0004-Add-option-to-efi-updatevar-to-read-from-stdin.patch
> new file mode 100644
> index 0000000000..9704269116
> --- /dev/null
> +++ b/package/efitools/0004-Add-option-to-efi-updatevar-to-read-from-stdin.patch
> @@ -0,0 +1,74 @@
> +From 3c7fbc5e24634e229ea785e106382cef4c8eec5c Mon Sep 17 00:00:00 2001
> +From: "Guillaume GC. Chaye" <guillaume.chaye@zeetim.com>
> +Date: Wed, 11 Sep 2024 10:52:19 +0200
> +Subject: [PATCH] efitools: add option to efi-updatevar to read from stdin
> + using -f- option
> +
> +The option "-f /dev/stdin" is actually not working.
> +This patch allow to pipe keys in your script without having errors.
> +
> +Signed-off-by: Guillaume GC. Chaye <guillaume.chaye@zeetim.com>
> +Upstream: N/A
This clearly needs to be submitted upstream.
> ++ bool read_stdin=false;
Coding style: spaces around "=" sign.
> +
> +
> + while (argc > 1 && argv[1][0] == '-') {
> +@@ -97,6 +99,10 @@ main(int argc, char *argv[])
> + file = argv[2];
> + argv += 2;
> + argc -= 2;
> ++ } else if (strcmp(argv[1], "-f-") == 0) {
> ++ read_stdin=true;
Same.
> ++ argv += 1;
> ++ argc -= 1;
> + } else if (strcmp(argv[1], "-g") == 0) {
> + if (str_to_guid(argv[2], &guid)) {
> + fprintf(stderr, "Invalid GUID %s\n", argv[2]);
> +@@ -147,7 +153,7 @@ main(int argc, char *argv[])
> + exit(1);
> + }
> +
> +- if (delsig == -1 && (!!file + !!hash_mode + !!crt_file != 1)) {
> ++ if (delsig == -1 && !read_stdin && (!!file + !!hash_mode + !!crt_file != 1)) {
> + fprintf(stderr, "must specify exactly one of -f, -b or -c\n");
> + exit(1);
> + }
> +@@ -219,7 +225,14 @@ main(int argc, char *argv[])
> + buf = malloc(st.st_size);
> + read(fd, buf, st.st_size);
> + close(fd);
> +- } else {
> ++ }else if (read_stdin){
Space after {, space before {.
> ++ buf=malloc(0x400);
Space around "=".
> ++ st.st_size=0;
Ditto.
> ++ while (read(STDIN_FILENO,buf+st.st_size,1)){
Space before {. Spaces after , in the function call.
Not sure reading byte by byte stdin is the best solution. Also not sure
using st.st_size as the size counter is a good idea. Would make more
sense to have a separate size_t variable.
> diff --git a/package/efitools/efitools.mk b/package/efitools/efitools.mk
> new file mode 100644
> index 0000000000..1da3b88acf
> --- /dev/null
> +++ b/package/efitools/efitools.mk
> @@ -0,0 +1,37 @@
> +################################################################################
> +#
> +# efitools
> +#
> +################################################################################
> +
> +EFITOOLS_VERSION = b988d20
Only one space after =, and we need the full commit hash.
> +EFITOOLS_SITE = https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git
> +EFITOOLS_SITE_METHOD = git
> +EFITOOLS_LICENSE = GPL-2.0
> +EFITOOLS_LICENSE_FILES = COPYING
> +EFITOOLS_DEPENDENCIES = gnu-efi openssl
> +HOST_EFITOOLS_DEPENDENCIES = host-gnu-efi host-openssl host-perl-file-slurp
I'm still not sure to understand why the host needs
host-perl-file-slurp but not the target, even though you're building
both in (apparently) the same way.
> +define EFITOOLS_BUILD_CMDS
> + cd $(@D); $(TARGET_CONFIGURE_OPTS) $(MAKE) SYSROOT_DIR=$(STAGING_DIR) EXTRA_LDFLAGS="$(TARGET_LDFLAGS)"
Please use:
$(MAKE) -C $(@D)
instead of:
cd $(@D); $(MAKE)
(same below)
> +endef
> +
> +define HOST_EFITOOLS_BUILD_CMDS
> + cd $(@D); $(HOST_CONFIGURE_OPTS) $(MAKE) SYSROOT_DIR=$(HOST_DIR) EXTRA_LDFLAGS="$(HOST_LDFLAGS)"
> +endef
> +
> +define EFITOOLS_INSTALL_TARGET_CMDS
> + cd $(@D); $(TARGET_CONFIGURE_OPTS) $(MAKE) install DESTDIR=$(TARGET_DIR)
> +endef
> +
> +define HOST_EFITOOLS_INSTALL_CMDS
> + cd $(@D); $(HOST_CONFIGURE_OPTS) $(MAKE) install DESTDIR=$(HOST_DIR)
> +endef
> +
> +define EFITOOLS_APPLY_ADDITIONAL_PATCHES
> + $(APPLY_PATCHES) $(@D) $(EFITOOLS_PKGDIR)/target \*.patch
> +endef
> +EFITOOLS_POST_PATCH_HOOKS+= EFITOOLS_APPLY_ADDITIONAL_PATCHES
No, we don't want to apply a specific set of patches depending on
whether we're building the host or target package. There should be one
single series of patches.
> +
> +$(eval $(generic-package))
> +$(eval $(host-generic-package))
> diff --git a/package/efitools/target/0001-Build-only-binaries-when-cross-compiling.patch b/package/efitools/target/0001-Build-only-binaries-when-cross-compiling.patch
> new file mode 100644
> index 0000000000..9eeaaa1260
> --- /dev/null
> +++ b/package/efitools/target/0001-Build-only-binaries-when-cross-compiling.patch
> @@ -0,0 +1,43 @@
> +From 595a7dc8527f90ba6bf7e0218b00aa7f93885ef1 Mon Sep 17 00:00:00 2001
> +From: Guillaume Chaye <guillaume.chaye@zeetim.com>
> +Date: Fri, 24 Jan 2025 12:56:02 +0100
> +Subject: [PATCH] efitools: build only binaries when cross compiling
> +
> +We cannot execute binaries to generate "auth" files when compiling
> +for another architecture.
> +help2man is also not working properly when cross compiling.
> +
> +Signed-off-by: Guillaume Chaye <guillaume.chaye@zeetim.com>
> +Upstream: N/A
This unfortunately will not be upstreamable. You need to introduce
variables to allow disabling the build/installation of things that are
not possible/relevant.
Overall, the main concern is the set of patches that has not been
submitted upstream, while there is an upstream project.
Could you have a look at this, and submit patches upstream?
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
next prev parent reply other threads:[~2025-05-16 19:14 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-25 13:46 [Buildroot] [PATCH 1/2] package/perl-file-slurp: add host package Guillaume Chaye
2025-03-25 13:46 ` [Buildroot] [PATCH 2/2] package/efitools: efitools is a set of utilities to manipulate efi variables Guillaume Chaye
2025-05-16 19:14 ` Thomas Petazzoni via buildroot [this message]
2025-05-16 19:06 ` [Buildroot] [PATCH 1/2] package/perl-file-slurp: add host package Thomas Petazzoni via buildroot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250516211449.0e04ef51@windsurf \
--to=buildroot@buildroot.org \
--cc=chrismcc@gmail.com \
--cc=guillaume.chaye@zeetim.com \
--cc=thomas.petazzoni@bootlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox