Message ID | e302f10d06b7406978bbc888b903cb917cb0e754.1468750623.git.yann.morin.1998@free.fr |
---|---|
State | Accepted |
Headers | show |
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) $< \ >
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
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.
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 --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) $< \
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(-)