[Buildroot] [PATCH 10/16 v3] core: introduce per br2-external ID

Arnout Vandecappelle arnout at mind.be
Sat Aug 27 21:49:51 UTC 2016


 I guess you're about to send a v3 so I'll try to still do a quick review of the
remaining things.


On 17-07-16 12:34, Yann E. MORIN wrote:
> This unique ID is used to construct a per br2-external tree variable,
> BR2_EXTERNAL_$(ID), which contains the path to the br2-external tree.
> 
> This variable is available both from Kconfig (set in the Kconfig
> snippet) and from the .mk files.
> 
> Also, display the BR2_EXTERNAL_$(ID) and its path as a comment in the
> menuconfig.
> 
> This will ultimately allow us to support multiple br2-external trees at
> once, with that ID (and thus BR2_EXTERNAL_$(ID)) uniquely defining which
> br2-external tree is being used.
> 
> Note: since the variables in the Makefile and in Kconfig are named the
> same, the one we computed early on (second hunk) will be overridden by
> the one in .config when we have it. Thus, even though they are set to
> the same raw value, the one from .config is quoted and, being included
> later in the Makefile, will take precedence, so we must un-quote it
> before we include the br2-external's makefile (third hunk). That's
> unfortunate, but there is no easy way around that as we want the two
> variables to be named the same in Makefile and Kconfig (and we can't
> ask the user to un-quote that variable himself either), hence the
> little dirty trick.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998 at free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni at free-electrons.com>
> Cc: Peter Korsgaard <jacmet at uclibc.org>
> Cc: Thomas De Schampheleire <patrickdepinguin at gmail.com>
> Cc: Arnout Vandecappelle <arnout at mind.be>
> ---
>  Makefile | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index be2f586..72b55cb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -148,6 +148,9 @@ $(if $(BASE_DIR),, $(error output directory "$(O)" does not exist))
>  #
>  # When BR2_EXTERNAL is set to an empty value (e.g. explicitly in command
>  # line), the .br-external file is removed.
> +#
> +# If the br2-external tree defines its ID, then export the path in the
> +# BR2_EXTERNAL_$(ID) variable.

 I agree with Thomas that NAME is better than ID.

 The name can be anything, including, for example, MK - which would create a
conflict with BR2_EXTERNAL_MK. Therefore I think there should be an additional
prefix, e.g. BR2_EXTERNAL_DIR_$(NAME).

>  
>  BR2_EXTERNAL_FILE = $(BASE_DIR)/.br-external
>  -include $(BR2_EXTERNAL_FILE)
> @@ -160,6 +163,13 @@ else
>    endif
>    override BR2_EXTERNAL := $(_BR2_EXTERNAL)
>    $(shell echo BR2_EXTERNAL ?= $(BR2_EXTERNAL) > $(BR2_EXTERNAL_FILE))
> +  ifneq ($(wildcard $(BR2_EXTERNAL)/external.id),)
> +    BR2_EXTERNAL_ID := $(shell cat $(BR2_EXTERNAL)/external.id 2>/dev/null)

 BR2_EXTERNAL_ID is an internal variable, so it should be EXTERNAL_NAME I think.
Admittedly many of the currently existing BR2_EXTERNAL variables wrongly have
the BR2 prefix, but let's not make it worse :-)


> +    ifeq ($(BR2_EXTERNAL_ID),)
> +      $(error $(BR2_EXTERNAL) has no ID (in file 'external.id'))
> +    endif
> +    BR2_EXTERNAL_$(BR2_EXTERNAL_ID) = $(BR2_EXTERNAL)
> +  endif
>    BR2_EXTERNAL_MK = $(BR2_EXTERNAL)/external.mk
>  endif
>  
> @@ -457,6 +467,10 @@ include boot/common.mk
>  include linux/linux.mk
>  include fs/common.mk
>  
> +# If the br2-external tree defines its ID, then the BR2_EXTERNAL_$(ID)
> +# variable is also present in .config, so it is quoted. We must unquote
> +# it before feeding it to the br2-external makefile.
> +BR2_EXTERNAL_$(BR2_EXTERNAL_ID) := $(call qstrip,$(BR2_EXTERNAL_$(BR2_EXTERNAL_ID)))
>  # Nothing to include if no BR2_EXTERNAL tree in use
>  include $(BR2_EXTERNAL_MK)
>  
> @@ -882,7 +896,13 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
>  	$(Q)if [ -n '$(BR2_EXTERNAL)' ]; then \
>  		printf "#\n# Automatically generated file; DO NOT EDIT.\n#\n\n"; \
>  		printf 'menu "User-provided options"\n\n'; \
> -		printf 'source "%s/Config.in"\n\n' $$(cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd); \
> +		if [ -z "$(BR2_EXTERNAL_ID)" ]; then \
> +			printf 'source "%s/Config.in"\n\n' $$(cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd); \
> +		else \
> +			printf 'config BR2_EXTERNAL_%s\n' $(BR2_EXTERNAL_ID); \
> +			printf '\tstring\n\tdefault "%s"\n\n' $(BR2_EXTERNAL_$(BR2_EXTERNAL_ID)); \

 Why is this config still needed? It was there before because that's the only
way to use it in the 'source' statement, but now that we generate the external
Config.in, I don't think it's needed anymore. Which also removes the need for
the unquoting.

 Regards,
 Arnout

> +			printf 'source "$$BR2_EXTERNAL_%s/Config.in"\n\n' $(BR2_EXTERNAL_ID); \
> +		fi; \
>  		printf 'endmenu # User-provided options\n'; \
>  	fi >$@
>  
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF



More information about the buildroot mailing list