Makefile.dep: Fix duplicate modules from USEMODULE

[leandrolanzieri]

Contribution description

With the changes introduced in #13089 the list of USEMODULEs shows multiple times auto_init_% modules.

There are multiple reasons for this:

1. The logic that adds the auto_init_% DEFAULT_MODULES to USEMODULE is placed at the end of Makefile.dep, which means that it gets executed multiple times (due to recursive inclusion of Makefile.dep).
2. DEFAULT_MODULES is not sorted (unlike USEMODULE) so it holds duplicates.
3. In Makefile.include USEMODULE is extended with the not-disabled DEFAULT_MODULEs, but the assignment is not simply expanded, so it only gets expended after a first iteration of Makefile.dep. This means that the DEFAULT_MODULE list may already have new elements.

Printing USEMODULE and filtering duplicates, for example, for examples/gcoap returns:

make info-debug-variable-USEMODULE | tr " " "\n" | sort | uniq -d

On master:

auto_init_gnrc_ipv6
auto_init_gnrc_ipv6_nib
auto_init_gnrc_pktbuf
auto_init_gnrc_udp
auto_init_random
auto_init_xtimer

With PR:

# empty

Testing procedure

• Check with make info-debug-variable-USEMODULE that modules are not longer duplicated, and the list of not duplicated is the same.

Issues/PRs references

#13089

[cgundogan]

@leandrolanzieri nice catch!

I don't want to dogmatize this incident, but here again we have a poster child of how untamable our current solution to dependency resolution is ... and that we are missing proper regression tests for the build system itself.

Anything that adds complexity to the enormity in Makefile.dep (c'mon, recursive inclusion of the same Makefile?) feels like a step backwards. It requires a good grasp of large parts of the build system to see overall implications of any change in Makefile.dep. I probably would've also missed to see this issue back in #13089 (since murdock signaled positively) and just would've merged it.
A replacement for Makefile.dep (such as with current efforts using Kconfig) is at least in my opinion very much welcome.

[leandrolanzieri]

@aabadie and @kaspar030, you reviewed the original PR, could you take a second look at this?

[fjmolinas]

Thanks for the catch @leandrolanzieri, sorry for not checking this earlier I was on vacation.

Regarding the changes alltough there is not harm in having duplicates, it is a nice cleanup!

The simple expansion is important catch, so thanks for the fix. I tested locally and it still works. The build issue are also present in master for me for all msp430:

/tmp/dwq.0.2075977587139033/7b8eb60c6d548c6521e399f152bbeb16/cpu/msp430_common/include/stdlib.h:30:20: fatal error: malloc.h: No such file or directory

Has something happened with msp430 since I've been away??

[cgundogan]

@fjmolinas sounds like a toolchain problem to me. @kaspar030 was there a docker image change and do we need to restart some workers?

[kaspar030]

@kaspar030 was there a docker image change and do we need to restart some workers?

The msp320-elf tolchain has been added, but there should be no change to the existing 4.x toolchain.

[kaspar030]

cpu/msp430_common/include/stdlib.h:30:20: fatal error: malloc.h: No such file or directory

IIRC the old msp430 needs oneway_malloc, which is a default module. Maybe some change there makes that not be included anymore.

[fjmolinas]

IIRC the old msp430 needs oneway_malloc, which is a default module. Maybe some change there makes that not be included anymore.

Hmm I don't understand why if I do:

BOARD=chronos make -C tests/xtimer_usleep info-debug-variable-USEMODULE --no-print-directory
auto_init board chronos-drivers core core_init core_msg core_panic cpu div msp430_common msp430_common_periph msp430_malloc periph periph_common periph_pm periph_timer stdin sys test_utils_interactive_sync xtimer auto_init_xtimer

there is no oneway_malloc

but if I do:

BOARD=ruuvitag make -C tests/xtimer_usleep info-debug-variable-USEMODULE --no-print-directory
auto_init board boards_common_nrf52xxdk core core_init core_msg core_panic cortexm_common cortexm_common_periph cortexm_fpu cpu cpu_common div newlib newlib_nano newlib_syscalls_default nrf5x_common_periph periph periph_common periph_gpio periph_pm periph_timer stdin stdio_rtt sys test_utils_interactive_sync xtimer auto_init_xtimer

there is stdio_rtt

[fjmolinas]

The change also breaks tha handling of stdio_rtt, but nrf52dk gets away with because of this hack:

RIOT/boards/common/nrf52/Makefile.include

Lines 9 to 12 in 2ebf1d6

	# The following configuration is dependencies specific
	# but they are resolved later
	# Hack to know now if 'nordic_softdevice_ble' is used
	include $(RIOTBOARD)/$(BOARD)/Makefile.dep

which includes Makefile.dep before Makefile.include, which is what we want long term anyway. We could replicate this hack for msp430 while the parsing order is not fixed.

[cgundogan]

We could replicate this hack for msp430 while the parsing order is not fixed.

I guess the problem will vanish with #12457, but waiting for that PR to be merged may probably take a little bit more time.

which is what we want long term anyway.

we want a hack to persist long-term? :) I would not object with switching the include order. Are there other benefits to having Makefile.dep included before Makefile.include?

[fjmolinas]

we want a hack to persist long-term? :) I would not object with switching the include order. Are there other benefits to having Makefile.dep included before Makefile.include?

Not the hack hahaha, just the re-ordering, see #9913 for a more detaile road-map and explanation. Once all requirements are fulfilled, the ordering can be inverted and both hacks removed.

[fjmolinas]

@leandrolanzieri your last fixup re-introduces the behavior that make includes order matter, but that in this case make the DEFAULT_MODULE handling for msp430 work. Since the objective of the PR is fixing a different issue, I think that is the way to go. But I liked having an explicit behavior better.

[fjmolinas]

The current solution fixes the stated issue for me:

make -C examples/gnrc_networking info-debug-variable-USEMODULE --no-print-directory | tr " " "\n" | sort | uniq -d
# empty

ACK, please squash.

[leandrolanzieri]

@leandrolanzieri your last fixup re-introduces the behavior that make includes order matter, but that in this case make the DEFAULT_MODULE handling for msp430 work. Since the objective of the PR is fixing a different issue, I think that is the way to go. But I liked having an explicit behavior better.

This is not my favourite solution either. But we can't have the simply expanded assignment before the first iteration of the Makefile.deps, as a module could be added to DEFAULT_MODULE in any of them. We could fix it with the 'hack' for the msp430, but what if a driver or some other module adds something to DEFAULT_MODULE?

[leandrolanzieri]

@fjmolinas squashed

[fjmolinas]

This is not my favourite solution either. But we can't have the simply expanded assignment before the first iteration of the Makefile.deps, as a module could be added to DEFAULT_MODULE in any of them. We could fix it with the 'hack' for the msp430, but what if a driver or some other module adds something to DEFAULT_MODULE?

IMO DEFAULT_MODULE is not very well defined, and since the handling is wrong and unclear it leads to improper use. IMO DEFAULT_MODULE not declared in default.inc.mk should not trigger dependency resolutions, and If they don't then they can be converted to MODULES whenever. So for examples oneway_malloc is OK since id doesn't trigger dependency resolutions, stdio is wrong (I know I suggested its use as such before understanding DEFAULT_MODULES issue).

See #13089 (comment)_ also