Re: [Buildroot] [PATCH 1/5] package/alchemy: new host package

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

Hello Hervé,

On Wed,  3 Nov 2021 08:36:52 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> Alchemy is a build system developed by Parrot.
> 
> It is a new build system based on the one used in Android.
> A central makefile instance scans a workspace to find user
> makefiles, includes them and register modules to be built.
> 
> https://github.com/Parrot-Developers/alchemy
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Thanks for this. I think we should make it clear to other reviewers
that we intentionally did not add a new package infrastructure for this
new build system. Indeed, we do not expect to have a significant number
of packages that will rely on the Alchemy build system.

Of course, should we have more of these packages in the future, we can
consider adding a package infrastructure.


> diff --git a/package/alchemy/alchemy.mk b/package/alchemy/alchemy.mk
> new file mode 100644
> index 0000000000..95912a53a1
> --- /dev/null
> +++ b/package/alchemy/alchemy.mk
> @@ -0,0 +1,73 @@
> +################################################################################
> +#
> +# alchemy
> +#
> +################################################################################
> +
> +ALCHEMY_VERSION = d95b3c38cd37814a1b98d0bbf813de7adaaecfbc
> +ALCHEMY_SITE = $(call github,Parrot-Developers,alchemy,$(ALCHEMY_VERSION))
> +ALCHEMY_LICENSE = BSD-3-Clause

The kconfig code is under GPL-2.0.

> +ALCHEMY_LICENSE_FILES = COPYING
> +HOST_ALCHEMY_DEPENDENCIES = host-python3
> +
> +ALCHEMY_SDK_BASEDIR = $(STAGING_DIR)/usr/lib/alchemy/sdk
> +
> +define HOST_ALCHEMY_INSTALL_CMDS
> +	rm -rf $(HOST_DIR)/usr/bin/alchemy
> +	mkdir -p $(HOST_DIR)/usr/bin/alchemy
> +	cp -rf $(@D)/* $(HOST_DIR)/usr/bin/alchemy/

It feels really odd to have the entire Alchemy code base installed in
$(HOST_DIR)/usr/bin/alchemy/. Perhaps better installed in
$(HOST_DIR)/opt ?

Also $(HOST_DIR)/usr in fact no longer exists, it's a symlink to
$(HOST_DIR) itself, so instead of using $(HOST_DIR)/usr/bin, we prefer
using $(HOST_DIR)/bin directly.

> +	mkdir -p $(ALCHEMY_SDK_BASEDIR)

Is this required to be done here? It's just that this directory is in
the STAGING_DIR, so having it created during the installation of a host
package isn't nice (but admittedly we do have a number of packages who
already do this kind of things).

> +$(eval $(host-generic-package))
> +
> +# Variables used by other packages
> +
> +ALCHEMY_HOME = $(HOST_DIR)/usr/bin/alchemy
> +ALCHEMY_MAKE = $(ALCHEMY_HOME)/scripts/alchemake
> +
> +ALCHEMY_TARGET_CONFIGURE_ENV = \

You never have any CONFIGURE_CMDS in alchemy based  packages, and this
variable gets used in BUILD_CMDS, so the name
ALCHEMY_TARGET_CONFIGURE_ENV doesn't look very good. Perhaps just
ALCHEMY_TARGET_ENV ?

> +	$(TARGET_MAKE_ENV) \
> +	ALCHEMY_HOME=$(ALCHEMY_HOME) \
> +	ALCHEMY_WORKSPACE_DIR="$(@D)" \
> +	ALCHEMY_TARGET_OUT=alchemy-out \
> +	TARGET_OS=linux \
> +	TARGET_OS_FLAVOUR=buildroot \
> +	TARGET_CROSS="$(TARGET_CROSS)" \
> +	TARGET_ARCH=xxx \

Is this intended ?

> +	TARGET_GLOBAL_CXXFLAGS="$(TARGET_CXXFLAGS)" \
> +	TARGET_GLOBAL_LDFLAGS="$(TARGET_LDFLAGS)" \
> +	TARGET_GLOBAL_FFLAGS="$(TARGET_FCFLAGS)" \
> +	TARGET_GLOBAL_FCFLAGS="$(TARGET_FCFLAGS)"
> +
> +ifeq ($(BR2_STATIC_LIBS),y)
> +ALCHEMY_TARGET_CONFIGURE_ENV += TARGET_FORCE_STATIC=1
> +ALCHEMY_TARGET_CONFIGURE_ENV += TARGET_GLOBAL_CFLAGS="$(TARGET_CFLAGS)"

Just one assignment:

FOOBAR += \
	THIS=1 \
	THAT=2

> +else
> +ALCHEMY_TARGET_CONFIGURE_ENV += TARGET_GLOBAL_CFLAGS="$(TARGET_CFLAGS) -fPIC"
> +endif
> +
> +# Configure Alchemy package dependencies.
> +# This macro can be used by Alchemy packages
> +# $1: List of Buildroot alchemy package the caller depends on
> +define ALCHEMY_TARGET_CONFIGURE_SDKS
> +	ALCHEMY_TARGET_SDK_DIRS="$(addprefix $(ALCHEMY_SDK_BASEDIR)/,$(1))"
> +endef

I don't really like this idiom, I don't think we have constructs like
this in Buildroot so far, and I find it quite confusing. I think I
would prefer to keep it explicit, i.e in libfutils:

+define LIBFUTILS_BUILD_CMDS
+	$(ALCHEMY_TARGET_CONFIGURE_ENV) \
+	ALCHEMY_TARGET_SDK_DIRS="$(addprefix $(ALCHEMY_SDK_BASEDIR)/,ulog)"
+	$(LIBFUTILS_CONF_ENV) \
+	$(ALCHEMY_MAKE) libfutils
+endef

Indeed, this macro which generates an assignment that should be added
in the environment of ALCHEMY_MAKE feels too tricky.

> +# Install an Alchemy SDK file.
> +# This macro can be used by Alchemy packages
> +# $1: Buildroot package name
> +# $2: Alchemy module name
> +# $3: Alchemy module file name
> +define ALCHEMY_INSTALL_LIB_SDK_FILE
> +	mkdir -p $(ALCHEMY_SDK_BASEDIR)/$(strip $(1))
> +	( \
> +		echo 'LOCAL_PATH := $$(call my-dir)'; \
> +		echo 'include $$(CLEAR_VARS)'; \
> +		echo 'LOCAL_MODULE := $(strip $(2))'; \
> +		echo 'LOCAL_SDK := $(STAGING_DIR)'; \
> +		echo 'LOCAL_DESTDIR := usr/lib'; \
> +		echo 'LOCAL_MODULE_FILENAME := $(strip $(3))'; \
> +		echo 'include $$(BUILD_LIBRARY)'; \
> +	) > $(ALCHEMY_SDK_BASEDIR)/$(strip $(1))/atom.mk
> +endef

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