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

[Herve Codina <herve.codina@bootlin.com>]

Hi,

On Wed, 3 Nov 2021 16:44:31 +0100
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> Hello Hervé,

[...]

> > +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.

I missed this one.

The GPL-2.0 license file is not provided by Alchemy.
They mention 'kconfig source code is GPLv2' in their README file.

ALCHEMY_LICENSE = BSD-3-Clause (Alchemy), GPL-2.0 (kconfig)
ALCHEMY_LICENSE_FILES = COPYING README

Is that correct from Buildroot even if the GLP-2.0 license file is not
provided ?

> 
> > +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 ?

Ok, I will change in v2 to $(HOST_DIR)/opt/alchemy/

> 
> 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).

I will simply remove this mkdir in v2 as it is done already done in
ALCHEMY_INSTALL_LIB_SDK_FILE macro. 

> 
> > +$(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 ?

I will change in v2 to 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 ?

Yes, it is to avoid the internal Alchemy arch management.

It can lead to incorrect results with the toolchain we
provided in TARGET_CROSS. Indeed they add some CFLAGS
depending on given TARGET_ARCH.
To avoid issues, this was the simplest way I found to say
'Hey Alchemy, do not care about the arch, I do it for you'.

Maybe this breaks some internal parts such as building a
CMake or an autotools package but in the buildroot context,
A CMake or an autotools package will not be built by Alchemy.

I will add a comment in v2.

> 
> > +	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

Will be changed in v2.

> 
> > +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.

I will remove the macro in v2 and make the explicit assignment in each
packages.

> 
> > +# 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

Thanks for the review.

Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot