makefiles: cleanup DEFAULT_MODULE, introduce DEFAULT_MODULE_DELAYED #13785

fjmolinas · 2020-04-01T14:12:39Z

Contribution description

This PR attempts to cleanup, limits and documents the use of DEFAULT_MODULE and DISABLE_MODULE. It is the result of findings in #13651, as well as as findings and discussion while reviewing #13349.

(a) This PR includes DEFAULT_MODULEs only once to avoid undefined behavior where DEFAULT_MODULES added during the first dependency resolution iteration would be converted to USEMODULE and others wouldn't. If a BOARD or CPU needs a DEFAULT_MODULE added before dependency resolution it may add it to a $(RIOTCPU)/$(CPU)/defaultmodules.inc.mk file. DEFAULT_MODULEs added here can have dependencies, as they are resolved before dependency resolution (although there is currently no use case for this). This also means they can only be disabled in a declarative way (no ifneq) by an APPLICATION, CPU or BOARD.

~~This means that current handling of stdio_% is not allowed, but we keep it as is until another solution is proposed.~~ Now removed

The case where a DEFAULT_MODULE needs conditional disabling will be unsupported, and with no intentions to support through this mechanism.

(b) Any DEFAULT_MODULE added in a Makefile.dep will only be added at the end of dependency resolution process. They MAY not have any dependencies as they will never be converted during resolution. This Modules MAY be DISABLED at any time, since they are only resolved once at the end of dependency resolution.

This PR also introduces a new DEFAULT_MODULE_DELAYED. This variable allows making a difference between the two cases mentioned above. DEFAULT_MODULE is used for (a) and the new DEFAULT_MODULE_DELAYED for (b).

Adding the new variable allows to document the expected properties of that module. It would make it easier to add static checks verifying allowed behaviors easier. But if deemed unnecessary the last two commits adding and using DEFAULT_MODULE_DELAYED could be removed. I could also split that change as well as the auto_init/periph_init change.

Here is a debrief of the changes commit by commit.

d2d50eb
converts DEFAULT_MODULE to USEMODULE only before and after dependency resolution. This avoid undefined behaviour where depending on order on inclusion some modules would be added as USEMODULE during dependency resolution and others wouldn't.
db858fa enables adding DEFAULT_MODULE at BOARD and CPU level, this wants to get ready for when DEFAULT_MODULE addition is limited
991cecf moves auto_init and periph_init module disabling. Removes application wildcard disable.
36f61d2: adds a mew DEFAULT_MODULE_DELAYED variable and all required modifications to support it.
0f356be replaces all DEFAULT_MODULE not in defaultmodules.in.mk by DEFAULT_MODULE_DELAYED

TODO (as follow-ups):

entries in build-system-basics
add static checks
remove stdio_% special handling

~~To not conflict with #13349 this PR is based on it, although the changes and objectives are orthogonal.~~

Therefore this PR is reviewed better commit by commit, to not mix the changes in #13349. ~~I'll rebase as soon as possible to remove those.~~ rebased

Testing procedure

green murdock
save_all_dependencies should be ran but there will be many differences because of variable change. I would suggest removing 1ea4fc4 to reduce noise. There should not be any difference with regard to USEMODULEs, differences should show up only in DEFAULT_MODULE and DEFAULT_MODULE_DELAYED
feedback on naming for DEFAULT_MODULE_DELAYED
all periph_init_% and auto_init_% modules are still present and can be removed:

BOARD=nucleo-l152re make -C tests/xtimer_usleep/ info-debug-variable-USEMODULE --no-print-directory

auto_init_xtimer board boards_common_nucleo core core_init core_msg core_panic cortexm_common cortexm_common_periph cpu div isrpipe newlib newlib_nano newlib_syscalls_default periph periph_common periph_gpio periph_init_gpio periph_init_pm periph_init_timer periph_init_uart periph_pm periph_timer periph_uart pm_layered stdin stdio_uart stdio_uart_rx stm32_common stm32_common_periph sys test_utils_interactive_sync tsrb xtimer

DISABLE_MODULE="periph_init_gpio auto_init" BOARD=nucleo-l152re make -C tests/xtimer_usleep/ info-debug-variable-USEMODULE --no-print-directory

board boards_common_nucleo core core_init core_msg core_panic cortexm_common cortexm_common_periph cpu div isrpipe newlib newlib_nano newlib_syscalls_default periph periph_common periph_gpio periph_init_pm periph_init_timer periph_init_uart periph_pm periph_timer periph_uart pm_layered stdin stdio_uart stdio_uart_rx stm32_common stm32_common_periph sys test_utils_interactive_sync tsrb xtimer

Issues/PRs references

Addresses part of #13651, will be finished when TODO list is completed.

maribu · 2020-04-02T09:45:21Z

Could you rebase?

Before DEFAULT_MODULEs where recursiverly added to USEMODULE. This led to undefined behaviour where DEFAULT_MODULEs added in the first iteration through MAKEFILE.dep would end up in USEMODULE but if conditionally added during the recursion they would not be converted to USEMODULE. They are now added through simple expansion so that they are only added once before the parsing of Makefile.dep.

Allow for BOARDs and CPUs to declare DEFAULT_MODULEs that might have dependencies and therefore require being handled before parsing Makefile.dep. DEFAULT_MODULEs declared in %/defaultmodules.inc.mk can only be disabled at application level. In other cases the behaviour is undefined.

Now that auto_init and periph_init modules are added only once, they can be silently disabled in Makefile.dep, with no risk of them being added early to USEMODULE.

Introduce DEFAULT_MODULE_DELAYED for modules that do not have dependencies and therefore can be resolved after Makefile.dep. These modules can be disabled with DISABLE_MODULE during dependency resolution as well as before.

Use DEFAULT_MODULE_DELAYED and remove second inclusion of DEFAULT_MODULEs

fjmolinas · 2020-04-03T07:40:48Z

Rebased, the PR is no longer waiting. Maybe it makes sense to split the first three commits out of this one?

fjmolinas · 2020-04-03T08:09:44Z

These two are causing problems.

ifneq (,$(filter auto_init_gnrc_netif,$(USEMODULE)))
  USEMODULE += gnrc_netif_init_devs
endif

ifneq (,$(filter auto_init_saul,$(USEMODULE)))
  USEMODULE += saul_init_devs
endif

I'll need to figure those out..

maribu · 2020-04-03T08:37:22Z

I was thinking if instead of naming it DEFAULT_MODULE_DELAYED as USEMODULE_OPTIONAL would be more consistent with with FEATURES_OPTIONAL?

And I think that those should be part of the recursion to allow them to have dependencies themselves. I guess if we forbid this, it is only a matter of time someone can present a use case for an optional module which has dependencies on its own. But in order to prevent this from introducing subtle issues depending on the position in Makefile.dep, those optional modules could be added in an "outer recursion". The "inner recursion" would run until USEMODULE, USEPKG and FEATURES_USED converge. And the outer recursion adds the optional modules (if any), and if any optional modules are pulled in, it recurses again (which again calls the "inner recursion").

I'm not sure how much overhead this adds. But that approach seems to be rock-solid. If the overhead of this would be too much, I think it would be worth the overhead.

fjmolinas · 2020-04-06T07:23:46Z

I'm not sure how much overhead this adds. But that approach seems to be rock-solid. If the overhead of this would be too much, I think it would be worth the overhead.

I think that would work as long as there are no negative dependencies. None of these modules could DISABLE another one. If constructions as the auto_init ones are to be had then it would need to be changed to:

ifneq(, $( filter foo auto_init, $(USEMODULE))
  DEFAULT_MODULE += auto_init_foo
endif

And DISABLE_MODULE may only be defined in APPLICATION, BOARD or CPU defaultmodules.inc.mk in a declarative way:

# THIS NO
ifneq(, $( filter bar, $(USEMODULE))
  DISABLE_MODULE += foo
endif

# THIS YES
DISABLE_MODULE += foo

stale · 2020-10-09T12:45:28Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

fjmolinas requested review from maribu and leandrolanzieri April 1, 2020 14:12

fjmolinas requested review from aabadie, bergzand, DipSwitch, gschorcht, haukepetersen, jia200x, kaspar030, MichelRottleuthner, miri64, PeterKietzmann, smlng and thomaseichinger as code owners April 1, 2020 14:12

This was referenced Apr 1, 2020

build system: Restructure dependency resolution #13349

Merged

*/Makefile.dep: remove usage of DEFAULT_MODULE += stdio% #13791

Merged

fjmolinas added 5 commits April 3, 2020 09:30

makefiles/defaultmodules.inc.mk: move auto_init_% periph_init_% removal

991cecf

Now that auto_init and periph_init modules are added only once, they can be silently disabled in Makefile.dep, with no risk of them being added early to USEMODULE.

makefiles/vars.inc.mk: introduce DEFAULT_MODULE_DELAYED

36a9011

Introduce DEFAULT_MODULE_DELAYED for modules that do not have dependencies and therefore can be resolved after Makefile.dep. These modules can be disabled with DISABLE_MODULE during dependency resolution as well as before.

%/Makefile.dep: use DEFAULT_MODULE_DELAYED

39e5ca2

Use DEFAULT_MODULE_DELAYED and remove second inclusion of DEFAULT_MODULEs

fjmolinas force-pushed the pr_default_modules branch from 0f356be to 39e5ca2 Compare April 3, 2020 07:36

fjmolinas removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 3, 2020

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Oct 9, 2020

stale bot closed this Nov 9, 2020

fjmolinas deleted the pr_default_modules branch June 9, 2022 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

makefiles: cleanup DEFAULT_MODULE, introduce DEFAULT_MODULE_DELAYED #13785

makefiles: cleanup DEFAULT_MODULE, introduce DEFAULT_MODULE_DELAYED #13785

Uh oh!

fjmolinas commented Apr 1, 2020 •

edited

Loading

Uh oh!

maribu commented Apr 2, 2020

Uh oh!

fjmolinas commented Apr 3, 2020

Uh oh!

fjmolinas commented Apr 3, 2020

Uh oh!

maribu commented Apr 3, 2020

Uh oh!

fjmolinas commented Apr 6, 2020

Uh oh!

stale bot commented Oct 9, 2020

Uh oh!

Uh oh!

makefiles: cleanup DEFAULT_MODULE, introduce DEFAULT_MODULE_DELAYED #13785

makefiles: cleanup DEFAULT_MODULE, introduce DEFAULT_MODULE_DELAYED #13785

Uh oh!

Conversation

fjmolinas commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu commented Apr 2, 2020

Uh oh!

fjmolinas commented Apr 3, 2020

Uh oh!

fjmolinas commented Apr 3, 2020

Uh oh!

maribu commented Apr 3, 2020

Uh oh!

fjmolinas commented Apr 6, 2020

Uh oh!

stale bot commented Oct 9, 2020

Uh oh!

Uh oh!

fjmolinas commented Apr 1, 2020 •

edited

Loading