| Message ID | a161d4b17c131b471418e4fa979aa73129ad102c.1468750623.git.yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Yann, Le 17/07/2016 à 12:34, Yann E. MORIN a écrit : > 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. Well, I have no other proposal to avoid the "little dirty trick". Reviewed-by: Romain Naour <romain.naour@gmail.com> Best regards, Romain > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Peter Korsgaard <jacmet@uclibc.org> > Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com> > Cc: Arnout Vandecappelle <arnout@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. > > 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) > + 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)); \ > + printf 'source "$$BR2_EXTERNAL_%s/Config.in"\n\n' $(BR2_EXTERNAL_ID); \ > + fi; \ > printf 'endmenu # User-provided options\n'; \ > fi >$@ > >
Hello, On Sun, 17 Jul 2016 12:34:30 +0200, 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. I would suggest to use the terminology "name" rather than "ID". I think it's much easier to understand when using "name" that using "ID". It's a bit longer, but not much, and will make things clearer. Thomas
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@free.fr> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Peter Korsgaard <jacmet@uclibc.org> > Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com> > Cc: Arnout Vandecappelle <arnout@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, All, On 2016-08-27 23:49 +0200, Arnout Vandecappelle spake thusly: > I guess you're about to send a v3 so I'll try to still do a quick review of the > remaining things. Not really "about" to. But I will, yes. Thanks! :-) > 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@free.fr> > > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > Cc: Peter Korsgaard <jacmet@uclibc.org> > > Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com> > > Cc: Arnout Vandecappelle <arnout@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). OK, will see to it. > > 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 :-) OK, fair enough. Will see to it. > > + 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. Hmmm.. As far as I can see, it is still needed, because the Config.in files in the br2-external tree may want to include other Config.in files, no? Like for example (with the variable renamed as per above): .../ext-tree/Config.in source "$BR2_EXTERNAL_DIR_FOO/package/pkg-1/Config.in" source "$BR2_EXTERNAL_DIR_FOO/package/pkg-2/Config.in" So it is stil needed to have the path to each br2-external paths from Kconfig. Unless I missed something? Regards, Yann E. MORIN.
Thomas, All, On 2016-08-27 22:10 +0200, Thomas Petazzoni spake thusly: > On Sun, 17 Jul 2016 12:34:30 +0200, 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. > > I would suggest to use the terminology "name" rather than "ID". I > think it's much easier to understand when using "name" that using "ID". > It's a bit longer, but not much, and will make things clearer. I'm OK with that. I will change. Cordialement, Yann E. MORIN.
Hello, On Sat, 27 Aug 2016 23:59:51 +0200, Yann E. MORIN wrote: > > > + 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. > > Hmmm.. As far as I can see, it is still needed, because the Config.in > files in the br2-external tree may want to include other Config.in > files, no? > > Like for example (with the variable renamed as per above): > > .../ext-tree/Config.in > source "$BR2_EXTERNAL_DIR_FOO/package/pkg-1/Config.in" > source "$BR2_EXTERNAL_DIR_FOO/package/pkg-2/Config.in" > > So it is stil needed to have the path to each br2-external paths from > Kconfig. > > Unless I missed something? No, I think you're right. I don't see how we can do without this config variable, which is needed to communicate the BR2_EXTERNAL locations from the make logic down to the Kconfig logic, in order to allow Config.in files to include other Config.in files located in BR2_EXTERNAL. Thomas
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. 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) + 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)); \ + printf 'source "$$BR2_EXTERNAL_%s/Config.in"\n\n' $(BR2_EXTERNAL_ID); \ + fi; \ printf 'endmenu # User-provided options\n'; \ fi >$@
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@free.fr> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Peter Korsgaard <jacmet@uclibc.org> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com> Cc: Arnout Vandecappelle <arnout@mind.be> --- Makefile | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)