Skip to content

Makefile.dep: disable stdio_% modules before they are included #13650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 18, 2020
Merged

Makefile.dep: disable stdio_% modules before they are included #13650

merged 1 commit into from
Mar 18, 2020

Conversation

fjmolinas
Copy link
Contributor

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 that Makefile.dep?

This PR also fixes disabling of stdio modules when this is done not by using DISABLE_MODULE directly but by the use of a conflicting USEMODULE.

Before this PR because of recursive declaration, on the first pass through Makefile.dep stdio_rtt would be added and then removed from USEMODULE this would lead to having in USEMODULE the dependencies of stdio_rtt but not the module itself.

By disabling right away we make sure its dependencies are note added.

Testing procedure

Issues/PRs references

Fixes #13460
Related to #13469

@fjmolinas fjmolinas added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Mar 18, 2020
@fjmolinas fjmolinas requested review from miri64 and aabadie March 18, 2020 12:37
fjmolinas
fjmolinas commented Mar 18, 2020
View reviewed changes
@fjmolinas fjmolinas mentioned this pull request Mar 18, 2020
Closed
miri64
miri64 requested changes Mar 18, 2020
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

@miri64
Copy link
Member

miri64 commented Mar 18, 2020

Other than my change request, I'm fine with the overall concept of this PR.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Mar 18, 2020
@miri64
Copy link
Member

miri64 commented Mar 18, 2020

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. STDIO_MODULES += stdio_rtt?

miri64
miri64 reviewed Mar 18, 2020
View reviewed changes
makefiles/stdio.inc.mk
Comment on lines +24 to +28
ifneq (,$(filter newlib,$(USEMODULE)))
ifeq (,$(filter $(STDIO_MODULES),$(USEMODULE)))
USEMODULE += stdio_uart
endif
endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, but basically this is why stdio_null is necessary, right? Why does there need to be a stdio at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeahyeah, this was just a thought, not actually requesting a solution here.

@fjmolinas
Copy link
Contributor Author

Tested with some boards. Is the following of any concern?

Yes it is but thats an issue with #12304, where there is no way of properly disabling arduino-bootloader and boards/common/samd21-arduino-bootloader/Makefile.dep is always included for those boards. So the modules you point out are not included by stdio_cdc_acm

@miri64
Copy link
Member

miri64 commented Mar 18, 2020

Tested with some boards. Is the following of any concern?

Yes it is but thats an issue with #12304, where there is no way of properly disabling arduino-bootloader and boards/common/samd21-arduino-bootloader/Makefile.dep is always included for those boards. So the modules you point out are not included by stdio_cdc_acm

Then let's go with the current approach. Please squash!

miri64
miri64 approved these changes Mar 18, 2020
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK!

@fjmolinas
Makefile.dep: use stdio.inc.mk
9536a80 
Move all stdio dependencies to its own makefile
@fjmolinas fjmolinas force-pushed the pr_fix_stdio_disabling branch from 5890f45 to 9536a80 Compare March 18, 2020 14:58
@fjmolinas
Copy link
Contributor Author

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. STDIO_MODULES += stdio_rtt?

Can you elaborate? would these be a list of supported stdio?

@miri64
Copy link
Member

miri64 commented Mar 18, 2020

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. STDIO_MODULES += stdio_rtt?

Can you elaborate? would these be a list of supported stdio?

My thinking was that STDIO_MODULES would semantically work like DEFAULT_MODULES just be a distinct list. The reason I picked modules instead of module (singular) is to be prepared for #13469 already ;-).

@miri64
Copy link
Member

miri64 commented Mar 18, 2020

The idea is mostly to keep the semantics of DEFAULT_MODULES clean (which, when the build system itself starts disabling defaults by itself starts me to wonder: is it really a default then?)

@fjmolinas
Copy link
Contributor Author

The idea is mostly to keep the semantics of DEFAULT_MODULES clean (which, when the build system itself starts disabling defaults by itself starts me to wonder: is it really a default then?)

I understand, maybe we can discus that in #13651? What you propose could be a good way of separating special cases of DEFAULT_MODULEs (whether we use that name or not). But at the same time we could say the same thing about all USEMODULEs somehow.

@miri64 miri64 merged commit 8df7a46 into RIOT-OS:master Mar 18, 2020
@fjmolinas
Copy link
Contributor Author

Thanks for the review!

@fjmolinas fjmolinas deleted the pr_fix_stdio_disabling branch March 18, 2020 15:17
@miri64 miri64 mentioned this pull request Mar 18, 2020
Closed
@miri64
Copy link
Member

miri64 commented Mar 18, 2020

I've updated #13469 for the things discussed here (under "More considerations")

@miri64 miri64 mentioned this pull request Mar 18, 2020
Closed
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 19, 2020
@fjmolinas fjmolinas mentioned this pull request Mar 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

@aabadie aabadie Awaiting requested review from aabadie

Assignees
No one assigned
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.

Makefile.dep: DEFAULT_MODULE += stdio_rtt will always load dependencies - even if other stdio is selected
3 participants
@fjmolinas @miri64 @leandrolanzieri