diff mbox

[02/16,v3] core: commonalise the bundled and br2-external %_defconfig rules

Message ID e302f10d06b7406978bbc888b903cb917cb0e754.1468750623.git.yann.morin.1998@free.fr
State Accepted
Headers show

Commit Message

Yann E. MORIN July 17, 2016, 10:34 a.m. UTC
The code for both cases is exactly the same, and only differs in the
location where defconfig files are looked for.

We use an intermediate macro to generate the corresponding rules,
because directly generating the rules is ugly and needs lots of escaping
and double-dollar-ing for the $(eval ...) and $(foreach ...) calls to
play nicely together.

Furthermore, that will be tremendously useful when we support multiple
br2-external trees.

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>

---
Changes v2 -> v3:
  - use lower-case macro  (Arnout)
  - move comment in the generated rule  (Arnout)
---
 Makefile | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

Romain Naour July 30, 2016, 8:48 p.m. UTC | #1
Hi Yann, All,

Le 17/07/2016 à 12:34, Yann E. MORIN a écrit :
> The code for both cases is exactly the same, and only differs in the
> location where defconfig files are looked for.
> 
> We use an intermediate macro to generate the corresponding rules,
> because directly generating the rules is ugly and needs lots of escaping
> and double-dollar-ing for the $(eval ...) and $(foreach ...) calls to
> play nicely together.
> 
> Furthermore, that will be tremendously useful when we support multiple
> br2-external trees.

This patch is similar to the one sent by Sam Bobroff [1]. I reviewed it during
the summer camp but it was not applied since it was in a series we rejected.

Reviewed-by: Romain Naour <romain.naour@gmail.com>

Best regards,
Romain

[1] http://lists.busybox.net/pipermail/buildroot/2016-June/165382.html

> 
> 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>
> 
> ---
> Changes v2 -> v3:
>   - use lower-case macro  (Arnout)
>   - move comment in the generated rule  (Arnout)
> ---
>  Makefile | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index af2d982..7c0dcb1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -847,14 +847,13 @@ olddefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile
>  defconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile
>  	@$(COMMON_CONFIG_ENV) $< --defconfig$(if $(DEFCONFIG),=$(DEFCONFIG)) $(CONFIG_CONFIG_IN)
>  
> -# Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig
> -%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(TOPDIR)/configs/%_defconfig outputmakefile
> -	@$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(TOPDIR)/configs/$@ \
> -		$< --defconfig=$(TOPDIR)/configs/$@ $(CONFIG_CONFIG_IN)
> -
> -%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(BR2_EXTERNAL)/configs/%_defconfig outputmakefile
> -	@$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(BR2_EXTERNAL)/configs/$@ \
> -		$< --defconfig=$(BR2_EXTERNAL)/configs/$@ $(CONFIG_CONFIG_IN)
> +define percent_defconfig
> +%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(1)/configs/%_defconfig outputmakefile
> +	# Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig
> +	@$$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(1)/configs/$$@ \
> +		$$< --defconfig=$(1)/configs/$$@ $$(CONFIG_CONFIG_IN)
> +endef
> +$(eval $(foreach d,$(TOPDIR) $(BR2_EXTERNAL),$(call percent_defconfig,$(d))$(sep)))
>  
>  savedefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile
>  	@$(COMMON_CONFIG_ENV) $< \
>
Thomas Petazzoni Aug. 27, 2016, 2:12 p.m. UTC | #2
Hello,

On Sun, 17 Jul 2016 12:34:22 +0200, Yann E. MORIN wrote:

> +define percent_defconfig
> +%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(1)/configs/%_defconfig outputmakefile
> +	# Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig

This comment in here was a bit annoying, because it appears on stdout
when you run "make <foo>_defconfig". So I've moved it outside of the
target definition instead, in order to preserve the existing behavior
in terms of what appears on stdout when loading a defconfig.

Applied to next with this change.

Thanks,

Thomas
Yann E. MORIN Aug. 27, 2016, 2:16 p.m. UTC | #3
Thomas, All,

On 2016-08-27 16:12 +0200, Thomas Petazzoni spake thusly:
> On Sun, 17 Jul 2016 12:34:22 +0200, Yann E. MORIN wrote:
> 
> > +define percent_defconfig
> > +%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(1)/configs/%_defconfig outputmakefile
> > +	# Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig
> 
> This comment in here was a bit annoying, because it appears on stdout
> when you run "make <foo>_defconfig". So I've moved it outside of the
> target definition instead, in order to preserve the existing behavior
> in terms of what appears on stdout when loading a defconfig.

Well, that's how I did it first, but Arnout contended that the comment
should be moved like I did (or at least that's what I understood from
his review).

I'm happy you moved it. ;-)

Thanks!

Regards,
Yann E. MORIN.
Arnout Vandecappelle Aug. 27, 2016, 5:10 p.m. UTC | #4
On 27-08-16 16:16, Yann E. MORIN wrote:
> Thomas, All,
> 
> On 2016-08-27 16:12 +0200, Thomas Petazzoni spake thusly:
>> On Sun, 17 Jul 2016 12:34:22 +0200, Yann E. MORIN wrote:
>>
>>> +define percent_defconfig
>>> +%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(1)/configs/%_defconfig outputmakefile
>>> +	# Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig
>>
>> This comment in here was a bit annoying, because it appears on stdout
>> when you run "make <foo>_defconfig". So I've moved it outside of the
>> target definition instead, in order to preserve the existing behavior
>> in terms of what appears on stdout when loading a defconfig.
> 
> Well, that's how I did it first, but Arnout contended that the comment
> should be moved like I did (or at least that's what I understood from
> his review).

 With "inside the define" I meant exactly what Thomas did: inside the define
(not inside the rule).

 Regards,
 Arnout

> 
> I'm happy you moved it. ;-)
> 
> Thanks!
> 
> Regards,
> Yann E. MORIN.
>
diff mbox

Patch

diff --git a/Makefile b/Makefile
index af2d982..7c0dcb1 100644
--- a/Makefile
+++ b/Makefile
@@ -847,14 +847,13 @@  olddefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile
 defconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile
 	@$(COMMON_CONFIG_ENV) $< --defconfig$(if $(DEFCONFIG),=$(DEFCONFIG)) $(CONFIG_CONFIG_IN)
 
-# Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig
-%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(TOPDIR)/configs/%_defconfig outputmakefile
-	@$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(TOPDIR)/configs/$@ \
-		$< --defconfig=$(TOPDIR)/configs/$@ $(CONFIG_CONFIG_IN)
-
-%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(BR2_EXTERNAL)/configs/%_defconfig outputmakefile
-	@$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(BR2_EXTERNAL)/configs/$@ \
-		$< --defconfig=$(BR2_EXTERNAL)/configs/$@ $(CONFIG_CONFIG_IN)
+define percent_defconfig
+%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(1)/configs/%_defconfig outputmakefile
+	# Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig
+	@$$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(1)/configs/$$@ \
+		$$< --defconfig=$(1)/configs/$$@ $$(CONFIG_CONFIG_IN)
+endef
+$(eval $(foreach d,$(TOPDIR) $(BR2_EXTERNAL),$(call percent_defconfig,$(d))$(sep)))
 
 savedefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile
 	@$(COMMON_CONFIG_ENV) $< \