Skip to content

make: handle disabled default modules before dependencies#10744

Merged
cgundogan merged 1 commit intoRIOT-OS:masterfrom
kaspar030:handle_disabled_default_modules_before_deps
Jan 13, 2019
Merged

make: handle disabled default modules before dependencies#10744
cgundogan merged 1 commit intoRIOT-OS:masterfrom
kaspar030:handle_disabled_default_modules_before_deps

Conversation

@kaspar030
Copy link
Contributor

Contribution description

Currently, default modules can be disabled by adding the name to DISABLE_MODULE.
Unfortunately, this gets parsed after resolving the dependencies.

This makes it impossible to disable e.g., stdio_uart and implicitly periph/uart. The dependency parsing would select periph/uart (as it is dependend on stdio_uart), but later stdio_uart would be dropped, leaking the dependency.

This PR moves DISABLE_MODULE before dependency resolution.

Testing procedure

Correct compilation of all applications should suffice.

Issues/PRs references

#10741

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation 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 labels Jan 10, 2019
@kaspar030 kaspar030 requested a review from cladmi January 10, 2019 11:22
@basilfx
Copy link
Member

basilfx commented Jan 10, 2019

Maybe I'm using/understanding it wrong, but it isn't working for me.

I have a board that includes stdio_rtt in its Makefile.dep. Now I want to compile it with stdio_rtt disabled (and default to stdio_uart since it's the default).

I run DISABLE_MODULE=stdio_rtt BOARD=basilfx-taster make -j8 and I get this error:

DISABLE_MODULE="stdio_uart" BOARD=basilfx-taster make -j8
Required modules were disabled using DISABLE_MODULE: stdio_uart

EXPECT ERRORS!

examples/gnrc_knx_taster/../../Makefile.include:737: examples/gnrc_knx_taster/bin/basilfx-taster/stdio_uart.a
examples/gnrc_knx_taster/../../Makefile.include:739: *** BASELIBS value changed.  Stop.

@kaspar030
Copy link
Contributor Author

kaspar030 commented Jan 11, 2019

Maybe I'm using/understanding it wrong, but it isn't working for me.

Unfortunately, despite it's naming, DISABLE_MODULE only works for DEFAULT_MODULES.

This PR is the first, there are more to come that together will allow disabling stdio_uart...

Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

Sensible change. ACK

@cgundogan cgundogan added 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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 13, 2019
@cgundogan cgundogan merged commit a032070 into RIOT-OS:master Jan 13, 2019
@kaspar030 kaspar030 deleted the handle_disabled_default_modules_before_deps branch January 13, 2019 20:45
@cladmi
Copy link
Contributor

cladmi commented Jan 14, 2019

The only definitions of DEFAULT_MODULE were in makefiles/defaultmodules.inc.mk and one in cpu/msp430_common/Makefile.include for some reason, so has no bad impact now when looking at which modules are inside.

However, I do not really understood the issue that it will solve. The values in DEFAULT_MODULE are rather generic mechanisms that are included in a normal firmware, and modules you are supposed to find there.

I also mix a bit the order with depends on is dependent on so cannot really see the goal right now.

If A depends on B, you disable B, but still request A. I would maybe expect it to fail, not silently add B because it is a dependency. I do not think that you should able to say add and remove something at the same time.
Even more right now when it is for generic things like there is in the DEFAULT_MODULE list.

But we may indeed be missing a mechanism for your need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants