[Buildroot] [PATCH v3] Makefile: fix use of many br2-external trees

[Arnout Vandecappelle]

On 21/09/2022 20:13, Thomas Petazzoni wrote:
> On Tue, 20 Sep 2022 21:46:45 +0200
> "Yann E. MORIN" <yann.morin.1998 at 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