Re: [Buildroot] [PATCH 1/4] package/libxmlsec1: Add libxmlsec1 used by openSCAP

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

Hello Alexis,

Thanks for your patch! See some review below.

On Wed, 30 Jul 2025 14:47:13 +0200
Alexis Lothoré <alexis.lothore@bootlin.com> wrote:

> Introduce the libxmlsec1 library package. libxmlsec implements XML
> security standards.
> 
> The library has only a few mandatory dependencies(libxml2 and libxslt
> and a crypto library).  It needs one of the following cryptographic
> libraries: OpenSSL, NSS, or Gcrypt/GNUTLS. Default to openssl for now to
> keep the package simple.
> 
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>

By convention, the title of the patch should be:

	package/libxmlsec1: new package

> ---
>  package/Config.in                  |  1 +
>  package/libxmlsec1/Config.in       | 13 +++++++++++++
>  package/libxmlsec1/libxmlsec1.hash |  4 ++++
>  package/libxmlsec1/libxmlsec1.mk   | 28 ++++++++++++++++++++++++++++
>  4 files changed, 46 insertions(+)

You need to add an entry to the DEVELOPERS file for this package.


> diff --git a/package/libxmlsec1/libxmlsec1.hash b/package/libxmlsec1/libxmlsec1.hash
> new file mode 100644
> index 0000000000000000000000000000000000000000..156e1310a3609465d7914b2c9e830e5157b88f99
> --- /dev/null
> +++ b/package/libxmlsec1/libxmlsec1.hash
> @@ -0,0 +1,4 @@
> +# Locally computed
> +sha256  d82e93b69b8aa205a616b62917a269322bf63a3eaafb3775014e61752b2013ea  xmlsec1-1.3.7.tar.gz
> +# Locally comp

"Locally comp" and then your keyboard was blo? :-)

Not need to repeat this comment, if it's applicable for both lines.

> +sha256  d5e9388534256360c6a009d4f19ab2b8d6ec0aa7ff32a51e22d899ed7beb5c48  Copyright
> diff --git a/package/libxmlsec1/libxmlsec1.mk b/package/libxmlsec1/libxmlsec1.mk
> new file mode 100644
> index 0000000000000000000000000000000000000000..b05465dd10fffcf490db40d12b7a66ac33da7d47
> --- /dev/null
> +++ b/package/libxmlsec1/libxmlsec1.mk
> @@ -0,0 +1,28 @@
> +################################################################################
> +#
> +# libxmlsec1
> +#
> +################################################################################
> +
> +LIBXMLSEC1_VERSION = 1.3.7
> +LIBXMLSEC1_SOURCE = xmlsec1-$(LIBXMLSEC1_VERSION).tar.gz
> +LIBXMLSEC1_SITE = https://github.com/lsh123/xmlsec/releases/download/$(LIBXMLSEC1_VERSION)
> +LIBXMLSEC1_LICENSE = MIT

OK, license verified.

> +LIBXMLSEC1_LICENSE_FILES = Copyright
> +LIBXMLSEC1_INSTALL_STAGING = YES
> +LIBXMLSEC1_DEPENDENCIES = libxml2 libxslt openssl
> +HOST_LIBXMLSEC1_DEPENDENCIES = host-libxml2 host-libxslt host-openssl
> +
> +LIBXMLSEC1_CONF_OPTS =	--disable-static \
> +			--enable-crypto-dl=no \
> +			--with-openssl \
> +			--with-gnutls=no \
> +			--with-gcrypt=no \
> +			--with-nss=no

This should be a bit more nicely formatted, and use --disable/--without
instead of --enable-..=no and --with-...=no. Also, you shouldn't pass
--disable-static unless you have a very good reason to. Indeed,
Buildroot is passing --{enable,disable}-{shared,static} automatically
based on the BR2_SHARED_LIBS/BR2_STATIC_LIBS settings.

So:

LIBXMLSEC1_CONF_OPTS = \
	--disable-crypto-dl \
	--with-openssl \
	--without-gnutls \
	--without-gcrypt \
	--without-nss

> +
> +LIBXMLSEC1_AUTORECONF = YES

Perhaps put this "earlier", as it's valid for both the target and host
variants.

> +HOST_LIBXMLSEC1_CONF_OPTS = $(LIBXMLSEC1_CONF_OPTS)

I'm not sure I like re-using like this target options for the host
variant. I think I'd prefer them to be duplicated.

Thanks!

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