Makefile.dep: disable stdio_% modules before they are included#13650
Makefile.dep: disable stdio_% modules before they are included#13650miri64 merged 1 commit intoRIOT-OS:masterfrom
Conversation
miri64
left a comment
There was a problem hiding this comment.
Tested with some boards. Is the following of any concern?
$ USEMODULE=stdio_null BOARD=feather-m0 make -C examples/hello-world/ info-modules
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/examples/hello-world'
auto_init
---> auto_init_usbus
board
boards_common_samd21-arduino-bootloader
core
core_init
core_msg
core_panic
cortexm_common
cortexm_common_periph
cpu
newlib
newlib_nano
newlib_syscalls_default
periph
periph_common
periph_gpio
periph_init
periph_init_common
periph_init_gpio
periph_init_init
periph_init_pm
periph_pm
pm_layered
sam0_common_periph
stdio_null
sys
usb_board_reset
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/examples/hello-world'
|
Other than my change request, I'm fine with the overall concept of this PR. |
|
Just an idea I head, when working on #13649: Might it make sense to have a dedicated module list for the STDIO module(s), e.g. |
| ifneq (,$(filter newlib,$(USEMODULE))) | ||
| ifeq (,$(filter $(STDIO_MODULES),$(USEMODULE))) | ||
| USEMODULE += stdio_uart | ||
| endif | ||
| endif |
There was a problem hiding this comment.
Adapted from Makefile.dep, I know, but does this dependency even make sense? Why enforce a STDIO when newlib is there? AFAIK newlib should just work fine without STDIO, or am I mistaken?
There was a problem hiding this comment.
Adapted from Makefile.dep, I know, but does this dependency even make sense? Why enforce a STDIO when newlib is there? AFAIK newlib should just work fine without STDIO, or am I mistaken?
This might just be a way of saying if newlib is included then default stdio is stdio_uart.
There was a problem hiding this comment.
Yea, but basically this is why stdio_null is necessary, right? Why does there need to be a stdio at all?
There was a problem hiding this comment.
Yea, but basically this is why stdio_null is necessary, right? Why does there need to be a stdio at all?
It doesn't but most users expect to see something if they do make term so it makes sense to usually include it.
Maybe more can be done towards cleaning this up by actually using DEFAULT_MODULE here, but I would rather leave it for later.
There was a problem hiding this comment.
Yeahyeah, this was just a thought, not actually requesting a solution here.
Yes it is but thats an issue with #12304, where there is no way of properly disabling |
Then let's go with the current approach. Please squash! |
Move all stdio dependencies to its own makefile
5890f45 to
9536a80
Compare
Can you elaborate? would these be a list of supported |
My thinking was that |
|
The idea is mostly to keep the semantics of |
I understand, maybe we can discus that in #13651? What you propose could be a good way of separating special cases of |
|
Thanks for the review! |
|
I've updated #13469 for the things discussed here (under "More considerations") |
Contribution description
This PR moves all
stdio_%dependency resolution to is own file to cleanup inclusion order and better isolate the dependencies. Maybe #13469 will lead to a parent module and the dependencies can be moved to thatMakefile.dep?This PR also fixes disabling of
stdiomodules when this is done not by usingDISABLE_MODULEdirectly but by the use of a conflictingUSEMODULE.Before this PR because of recursive declaration, on the first pass through
Makefile.depstdio_rttwould be added and then removed fromUSEMODULEthis would lead to having inUSEMODULEthe dependencies ofstdio_rttbut not the module itself.By disabling right away we make sure its dependencies are note added.
Testing procedure
testing procedure in Makefile.dep: DEFAULT_MODULE += stdio_rtt will always load dependencies - even if other stdio is selected #13460
If the new Makefile is not liked I can keep the same changes in
Makefile.depI haven't changed the order in which
stdiomodules are included so there should be no change here, a green Murdock should be enough.Issues/PRs references
Fixes #13460
Related to #13469