| Message ID | 20220920194645.670432-1-yann.morin.1998@free.fr |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v3] Makefile: fix use of many br2-external trees | expand |
On Tue, 20 Sep 2022 21:46:45 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > The top level Makefile in buildroot has a recursive rule which causes > the appearance of a hang as the number of directories in BR2_EXTERNAL > increases. When the number of directories in BR2_EXTERNAL is small, the > recursion occurs, but make detects the recursion and determines the > target does not have to be remade. This allows make to progress. > > This is the failing rule: > > define percent_defconfig > # Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig > %_defconfig: $(BUILD_DIR)/buildroot-config/conf $(1)/configs/%_defconfig outputmakefile > @$$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(1)/configs/$$@ \ > $$< --defconfig=$(1)/configs/$$@ $$(CONFIG_CONFIG_IN) > endef > $(eval $(foreach d,$(call reverse,$(TOPDIR) $(BR2_EXTERNAL_DIRS)),$(call percent_defconfig,$(d))$(sep))) > > The rule for %defconfig is created for each directory in BR2_EXTERNAL. > When the rule is matched, the stem is 'defconfig_name'. The second > prerequisite is expanded to $(1)/configs/defconfig_name_defconfig. The > rule, and all of the other rules defined by this macro, are invoked > again, but the stem is now $(1)/configs/defconfig_name_defconfig. The > second prerequisite is now expanded to > $(1)/configs/($1)/configs/defconfig_name_defconfig. This expansion > continues until make detects the infinite recursion. > > With up to 5 br2-external trees, the time is very small, so that it is > not noticeable. But starting with 6 br2-external trees, the time is > insanely big (so much so that we did not even let it finish after it ran > for hours); see timings toward the end of the commit log. Wow, insane stuff! > One of the rationale behind this code, is that we want the defconfig > files from br2-external trees further down the list, to override > defconfig files from those earlier in the list, even overriding the > defconfig files from Buildroot itself. This is the part I would like to challenge. Why do we want to allow BR2_EXTERNAL to override defconfigs from the main tree? We do not allow this for packages, why should we allow it for defconfigs? To me, allowing the override of defconfigs is actually a bad idea: when you run "make foo_defconfig", it's no longer really clear which "foo_defconfig" is really going to be used. Yes, it's well defined, but it isn't "obvious". Thomas
Thomas, All, On 2022-09-21 20:13 +0200, Thomas Petazzoni spake thusly: > On Tue, 20 Sep 2022 21:46:45 +0200 > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: [--SNIP--] > > One of the rationale behind this code, is that we want the defconfig > > files from br2-external trees further down the list, to override > > defconfig files from those earlier in the list, even overriding the > > defconfig files from Buildroot itself. > This is the part I would like to challenge. Why do we want to allow > BR2_EXTERNAL to override defconfigs from the main tree? We do not allow > this for packages, why should we allow it for defconfigs? This patch does not change the actual behaviour: we've been allowing this for the past 6 years, we've documented it; all that patch does is actually fix using more than 5 br2-external trees. > To me, allowing the override of defconfigs is actually a bad idea: when > you run "make foo_defconfig", it's no longer really clear which > "foo_defconfig" is really going to be used. Yes, it's well defined, but > it isn't "obvious". I think initially, it was far simpler to do it that way, since we did not have a list of defconfig files, and so we (ab)used make ability to override a rule, to justify a simpler code on our side. Changing that behaviour is now easier, now that we do have a list of defconfig files, but if at all, that should be done in a follow-up patch. However, the multi br2-external support is there to cover one case: a br2-xternal tree provides basic support for a board (e.g. by a team in charge of the ardware support), and a second br2-external provides defconfig for an actual product. In this case, given a board named 'foo', it is understanble that the harware guys name their defconfig foo_defconfig, and that the product team also name their defconfig foo_defconfig. So, I think allowing this overide is not a bad thing. Regards, Yann E. MORIN.
On 21/09/2022 20:13, Thomas Petazzoni wrote: > On Tue, 20 Sep 2022 21:46:45 +0200 > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > >> The top level Makefile in buildroot has a recursive rule which causes >> the appearance of a hang as the number of directories in BR2_EXTERNAL >> increases. When the number of directories in BR2_EXTERNAL is small, the >> recursion occurs, but make detects the recursion and determines the >> target does not have to be remade. This allows make to progress. >> >> This is the failing rule: >> >> define percent_defconfig >> # Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig >> %_defconfig: $(BUILD_DIR)/buildroot-config/conf $(1)/configs/%_defconfig outputmakefile >> @$$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(1)/configs/$$@ \ >> $$< --defconfig=$(1)/configs/$$@ $$(CONFIG_CONFIG_IN) >> endef >> $(eval $(foreach d,$(call reverse,$(TOPDIR) $(BR2_EXTERNAL_DIRS)),$(call percent_defconfig,$(d))$(sep))) >> >> The rule for %defconfig is created for each directory in BR2_EXTERNAL. >> When the rule is matched, the stem is 'defconfig_name'. The second >> prerequisite is expanded to $(1)/configs/defconfig_name_defconfig. The >> rule, and all of the other rules defined by this macro, are invoked >> again, but the stem is now $(1)/configs/defconfig_name_defconfig. The >> second prerequisite is now expanded to >> $(1)/configs/($1)/configs/defconfig_name_defconfig. This expansion >> continues until make detects the infinite recursion. >> >> With up to 5 br2-external trees, the time is very small, so that it is >> not noticeable. But starting with 6 br2-external trees, the time is >> insanely big (so much so that we did not even let it finish after it ran >> for hours); see timings toward the end of the commit log. > > Wow, insane stuff! > >> One of the rationale behind this code, is that we want the defconfig >> files from br2-external trees further down the list, to override >> defconfig files from those earlier in the list, even overriding the >> defconfig files from Buildroot itself. > > This is the part I would like to challenge. Why do we want to allow > BR2_EXTERNAL to override defconfigs from the main tree? We do not allow > this for packages, why should we allow it for defconfigs? And indeed, this is exactly the reverse of what we would have now. We have two pattern rules that match with the same stem. In this case, according to 'info make': "'make' will choose the first one found in the makefile." Since we put $(TOPDIR) before the externals in the foreach loop, the internal one will be the one that gets used. However, maybe this is a simpler way to solve the issue: %_defconfig/real: $(BUILD_DIR)/buildroot-config/conf ... ... %_defconfig: %_defconfig/real Since directories are only removed from the filename when there's no / in the pattern, it will only match the wrong thing one level deep. I.e. we still have: foo_defconfig -> foo_defconfig/real foo_defconfig/real -> /path/to/buildroot/configs/foo_defconfig /path/to/buildroot/configs/foo_defconfig -> /path/to/buildroot/path/to/buildroot/configs/foo_defconfig/real but there it terminates, because that last one doesn't match %_defconfig nor %_defconfig/real (because of the / in the latter). Of course, I haven't tried this, I may be talking rubbish :-) Regards, Arnout > To me, allowing the override of defconfigs is actually a bad idea: when > you run "make foo_defconfig", it's no longer really clear which > "foo_defconfig" is really going to be used. Yes, it's well defined, but > it isn't "obvious". > > Thomas
Arnout, All, On 2022-09-21 20:32 +0200, Arnout Vandecappelle spake thusly: > On 21/09/2022 20:13, Thomas Petazzoni wrote: > >On Tue, 20 Sep 2022 21:46:45 +0200 > >"Yann E. MORIN" <yann.morin.1998@free.fr> wrote: [--SNIP--] > >>One of the rationale behind this code, is that we want the defconfig > >>files from br2-external trees further down the list, to override > >>defconfig files from those earlier in the list, even overriding the > >>defconfig files from Buildroot itself. > >This is the part I would like to challenge. Why do we want to allow > >BR2_EXTERNAL to override defconfigs from the main tree? We do not allow > >this for packages, why should we allow it for defconfigs? > And indeed, this is exactly the reverse of what we would have now. We have > two pattern rules that match with the same stem. In this case, according to > 'info make': "'make' will choose the first one found in the makefile." Since > we put $(TOPDIR) before the externals in the foreach loop, the internal one > will be the one that gets used. As discussed on IRC: except we do not put TOPDIR first; we do: $(call reverse,TOPDIR BR2_EXTERNALS) so the last external does win. I.e. that patch does not change the current behaviour; it just fixes the use of more than 5 br2-extenal trees at once. Sorry, I may not have been clear about the explanations in my commit log: the rationale part refers to the current code, not the change I did. Whoever applies it can amend as they see fit, or I can respin if needed. Oh, and by the way, I got that privately, so I'll paste it here so that patchwork catches it: Tested-by: David Lawson <david.lawson1@tx.rr.com> Regards, Yann E. MORIN.
diff --git a/Makefile b/Makefile index ec7c034ac1..9d4b1626ae 100644 --- a/Makefile +++ b/Makefile @@ -1010,13 +1010,27 @@ oldconfig syncconfig olddefconfig: $(BUILD_DIR)/buildroot-config/conf outputmake defconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile @$(COMMON_CONFIG_ENV) $< --defconfig$(if $(DEFCONFIG),=$(DEFCONFIG)) $(CONFIG_CONFIG_IN) -define percent_defconfig -# Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig -%_defconfig: $(BUILD_DIR)/buildroot-config/conf $(1)/configs/%_defconfig outputmakefile - @$$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(1)/configs/$$@ \ +ALL_DEFCONFIGS = +# $1: br2-external directory, without trailing /configs/ +# $2: defconfig name with trailing _defconfig +define defconfig_rule +ifeq ($$(filter $(2),$$(ALL_DEFCONFIGS)),) +# Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the actual defconfig +$(2): $$(BUILD_DIR)/buildroot-config/conf outputmakefile + $$(Q)$$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(1)/configs/$$@ \ $$< --defconfig=$(1)/configs/$$@ $$(CONFIG_CONFIG_IN) +ALL_DEFCONFIGS += $(2) +endif endef -$(eval $(foreach d,$(call reverse,$(TOPDIR) $(BR2_EXTERNAL_DIRS)),$(call percent_defconfig,$(d))$(sep))) +$(eval \ + $(foreach d, \ + $(call reverse,$(TOPDIR) $(BR2_EXTERNAL_DIRS)), \ + $(foreach c, \ + $(wildcard $(d)/configs/*_defconfig $(d)/configs/.*_defconfig), \ + $(call defconfig_rule,$(d),$(notdir $(c)))$(sep) \ + ) \ + ) \ +) update-defconfig: savedefconfig
The top level Makefile in buildroot has a recursive rule which causes the appearance of a hang as the number of directories in BR2_EXTERNAL increases. When the number of directories in BR2_EXTERNAL is small, the recursion occurs, but make detects the recursion and determines the target does not have to be remade. This allows make to progress. This is the failing rule: define percent_defconfig # Override the BR2_DEFCONFIG from COMMON_CONFIG_ENV with the new defconfig %_defconfig: $(BUILD_DIR)/buildroot-config/conf $(1)/configs/%_defconfig outputmakefile @$$(COMMON_CONFIG_ENV) BR2_DEFCONFIG=$(1)/configs/$$@ \ $$< --defconfig=$(1)/configs/$$@ $$(CONFIG_CONFIG_IN) endef $(eval $(foreach d,$(call reverse,$(TOPDIR) $(BR2_EXTERNAL_DIRS)),$(call percent_defconfig,$(d))$(sep))) The rule for %defconfig is created for each directory in BR2_EXTERNAL. When the rule is matched, the stem is 'defconfig_name'. The second prerequisite is expanded to $(1)/configs/defconfig_name_defconfig. The rule, and all of the other rules defined by this macro, are invoked again, but the stem is now $(1)/configs/defconfig_name_defconfig. The second prerequisite is now expanded to $(1)/configs/($1)/configs/defconfig_name_defconfig. This expansion continues until make detects the infinite recursion. With up to 5 br2-external trees, the time is very small, so that it is not noticeable. But starting with 6 br2-external trees, the time is insanely big (so much so that we did not even let it finish after it ran for hours); see timings toward the end of the commit log. One of the rationale behind this code, is that we want the defconfig files from br2-external trees further down the list, to override defconfig files from those earlier in the list, even overriding the defconfig files from Buildroot itself. We fix that by only creating explicit rules for defconfig files. To keep the promise that later defconfig files override previous ones (which we do document in our manual), we need to memorise what defconfig file we already created a rule for, and only create a rule for the first-seen-in-reverse-order (aka the last one) defconfig. Since some people appear to be bold enough (or insane enough?) to use defconfig files that start with a dot, also handle those explictly. Fixes: #14996 Reported-by: David Lawson <david.lawson1@tx.rr.com> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> ---- How to test many br2-external trees: $ for i in $(seq 1 1000); do mkdir -p br2-external-${i}/configs touch br2-external-${i}/{Config.in,external.mk} echo "name: BR_TEST_${i}" >br2-external-${i}/external.desc touch br2-external-${i}/configs/foo{,_${i}}_defconfig done $ time make V=1 \ BR2_EXTERNAL="$(for i in $(seq 1 N); do printf '%s ' "$(pwd)/foo²/br2-external-${i}"; done)" \ foo_1_defconfig Timings ('real' as reported by 'time'): N Before After 1 0.325s 0.328s 5 0.957s 0.358s 6 n/a 0.394s 10 n/a 0.432s 100 n/a 1.851s 1000 n/a 15.887s So, not only does that work for a large number of br2-external trees, it is also a little bit faster when using just a few. --- Changes v2 -> v3: - fix order of comment documenting $1 and $2 (David) - don't use immediate assignment :=, use simple assignment - add timing information and a way to reproduce Changes v1 -> v2: - keep comment - fix typo --- Makefile | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)