Re: [Buildroot] [PATCH 1/1] package/spdm-utils: new package

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Wilfred, Alistair, All,

On 2024-03-15 07:39 +1000, Wilfred Mallawa via buildroot spake thusly:
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> ---
[--SNIP--]
> diff --git a/package/spdm-utils/Config.in b/package/spdm-utils/Config.in
> new file mode 100644
> index 0000000000..97dbc51c6d
> --- /dev/null
> +++ b/package/spdm-utils/Config.in
> @@ -0,0 +1,20 @@
> +config BR2_PACKAGE_SPDM_UTILS
> +	bool "spdm-utils"
> +	depends on BR2_PACKAGE_HOST_RUSTC_TARGET_ARCH_SUPPORTS
> +	depends on BR2_USE_WCHAR # eudev
> +	depends on !BR2_STATIC_LIBS
> +	depends on BR2_USE_MMU # eudev

MMU and !static-libs are also needed for python3, so it should
identified as well:

    depends on !BR2_STATIC_LIBS  # python3
    depends on BR2_USE_MMU  # eudev, python3

(but see below for eudev).

> +	select BR2_PACKAGE_HOST_RUSTC
> +	select BR2_PACKAGE_OPENSSL
> +	select BR2_PACKAGE_PCIUTILS
> +	select BR2_PACKAGE_EUDEV

eudev is the provider of a virtual package, so you can't select it.

Instead, you have two cases:

  - the package needs a udev daemon (or library), in which case it
    should "depends on BR2_PACKAGE_HAS_UDEV" (and on 'udev' in the .mk),

  - or the package really needs eudev, in which case it can only depend
    on it.

> +	select BR2_PACKAGE_PYTHON3

python3 needs threads, so it must be propagated as well.

> +	select BR2_PACKAGE_LIBSPDM

libspdm has architecture dependencies, so it should be propagated.

[--SNIP--]
> diff --git a/package/spdm-utils/spdm-utils.mk b/package/spdm-utils/spdm-utils.mk
> new file mode 100644
> index 0000000000..5129635b06
> --- /dev/null
> +++ b/package/spdm-utils/spdm-utils.mk
> @@ -0,0 +1,43 @@
> +################################################################################
> +#
> +# spdm-utils
> +#
> +################################################################################
> +
> +SPDM_UTILS_VERSION = 0.3.0
> +SPDM_UTILS_SITE = $(call github,westerndigitalcorporation,spdm-utils,v$(SPDM_UTILS_VERSION))
> +SPDM_UTILS_LICENSE = Apache-2.0 or MIT
> +SPDM_UTILS_POST_PATCH_HOOKS += SPDM_UTILS_FETCH_CRATES
> +SPDM_UTILS_POST_INSTALL_TARGET_HOOKS += SPDM_UTILS_INSTALL_CERTS

Keep the _HOOKS assignments close to where the hooks are actually
defined.

Also, it looks more sensible (and usual) that the variables are ordered
in a logical(ish) way:

  - metadata: VERSION and SITE, LICENSE and LICENSE_FILES, CPE ID (if
    any)...

  - then the build info: DEPNDENCIES, CMDS and related HOOKS

> +SPDM_UTILS_DEPENDENCIES += pciutils libspdm openssl
> +
> +# We want to run the cargo-post-process script which is
> +# manually run in dl-wrapper after downloading the tarball.
> +# This will re-vendor in the crates, which needs to be done
> +# after we patch in our extra dependencies.

Why do we need to patch the depenencies? This should be explained in the
commit log.

Also, this means that it is no longer possible to do off-line builds,
which is something we try to avoid.

But in fact, this change adds no patch to the package at all, so we are
not modifying the dependencies, so we should not have to re-run the
vendoring...

Can you clarify that point?

> +define SPDM_UTILS_FETCH_CRATES
> +	cd $(SPDM_UTILS_SRCDIR) && \
> +	cargo vendor \
> +	    --manifest-path Cargo.toml \
> +		--locked VENDOR
> +endef

Note that, if you really, really need to re-vendor the package, you'd
need more than just calling cargo.

First, cargo may not be installed system-wide, so you'd have to be using
the one built by Buildroot, which means passing PATH=${BR_PATH), and to
be sure that it looks for crates in the proper location, i.e. passing
CARGO_HOME=$(BR_CARGO_HOME)

But again, it looks like this really is not necessary, since the package
is not patched.

> +define SPDM_UTILS_INSTALL_CERTS
> +	$(INSTALL) -d -m 0755 $(TARGET_DIR)/root/certs
> +	cp -r $(@D)/certs/generate_certs.sh $(TARGET_DIR)/root/certs/
> +	cp -r $(@D)/certs/openssl.cnf $(TARGET_DIR)/root/certs/
> +	cp -r $(@D)/certs/setup_certs.sh $(TARGET_DIR)/root/certs/
> +	$(INSTALL) -d -m 0755 $(TARGET_DIR)/root/certs/slot0
> +	cp $(@D)/certs/slot0/immutable.der $(TARGET_DIR)/root/certs/slot0
> +	cp $(@D)/certs/slot0/device.cert.der $(TARGET_DIR)/root/certs/slot0
> +	cp $(@D)/certs/slot0/device.der $(TARGET_DIR)/root/certs/slot0
> +	cp $(@D)/certs/slot0/device.key $(TARGET_DIR)/root/certs/slot0
> +	cp $(@D)/certs/slot0/param.pem $(TARGET_DIR)/root/certs/slot0
> +	cp $(@D)/certs/slot0/bundle_responder.certchain.der $(TARGET_DIR)/root/certs/slot0
> +	$(INSTALL) -d -m 0755 $(TARGET_DIR)/root/manifest
> +	cp $(@D)/manifest/manifest.out.cbor $(TARGET_DIR)/root/manifest

Why are those installed in the home for the root user, rather than
installed system-wide?

Also, why would we need the generate_certs.sh and setup_certs.sh
scripts, if certificates are installed at build time?

Regards,
Yann E. MORIN.

> +endef
> +
> +$(eval $(cargo-package))
> -- 
> 2.44.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot