Skip to content
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

Makefile.dep: make stdio_uart a feature required by ethos #12739

Closed
wants to merge 2 commits into from
Closed

Makefile.dep: make stdio_uart a feature required by ethos #12739

wants to merge 2 commits into from

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 18, 2019

Contribution description

An alternative proposal to #12724. Without touching any boards, with stdio_uart being the deciding factor if the application builds or not.

Testing procedure

Take any of the applications changed in this PR and try to build them for one of the boards previously blacklisted. It still fails:

$ BOARD=ruuvitag make -C tests/gnrc_ipv6_ext
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext'
There are unsatisfied feature requirements: stdio_uart
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext/../../Makefile.include:811: *** You can let the build continue on expected errors by setting CONTINUE_ON_EXPECTED_ERRORS=1 to the command line.  Stop.
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext'

Issues/PRs references

Alternative to #12724

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 18, 2019
@miri64 miri64 added this to the Release 2020.01 milestone Nov 18, 2019
@miri64 miri64 requested a review from aabadie November 18, 2019 13:31
@miri64 miri64 mentioned this pull request Nov 18, 2019
Merged
aabadie
aabadie requested changes Nov 18, 2019
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

IMHO, this is worse than the initial proposal in #12724

Makefile.dep
@@ -404,7 +404,7 @@ ifneq (,$(filter newlib,$(USEMODULE)))
USEMODULE += newlib_syscalls_default
endif
ifeq (,$(filter stdio_rtt stdio_cdc_acm,$(USEMODULE)))
USEMODULE += stdio_uart
FEATURES_PROVIDED += stdio_uart
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is forbidden.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where and why?

Copy link
Contributor

@aabadie aabadie Nov 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Features should only be provided in Makefile.features files. Makefile.dep files are here to do the resolution of dependencies given the provided features. If we start mixing the concepts we will end up in more mess.

This PR doesn't work with make info-boards-supported, so Murdock will fail in any case:

make -C tests/gnrc_ipv6_ext_frag info-boards-supported --no-print-directory | tr " " "\n" | grep thingy52
thingy52

Normally the result should be empty if the board is excluded.

@miri64
Copy link
Member Author

miri64 commented Nov 18, 2019

BTW, with this proposal:

CFLAGS="-DUSB_CONFIG_VID=0x1209 -DUSB_CONFIG_PID=0x0001" BOARD=arduino-zero USEMODULE=stdio_cdc_acm make -C tests/gnrc_ipv6_ext -j16
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext'
There are unsatisfied feature requirements: stdio_uart
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext/../../Makefile.include:811: *** You can let the build continue on expected errors by setting CONTINUE_ON_EXPECTED_ERRORS=1 to the command line.  Stop.
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gnrc_ipv6_ext'

With #12724 it passes (and pulls in 2 stdio implementations... not sure how and why this works).

fjmolinas
fjmolinas requested changes Nov 18, 2019
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

I'm against this. stdio_uart IMO is not a FEATURE, if we disagree on what a FEATURE is maybe we can have the discussion in #12473.

What you are doing is not declarative, which is also something @cladmi worked a lot to get rid of.

@miri64
Copy link
Member Author

miri64 commented Nov 18, 2019

I agree with you. This PR is not much better than #12473, so I'm closing it.

@miri64 miri64 closed this Nov 18, 2019
@miri64 miri64 deleted the stdio_uart/enh/make-feature branch November 18, 2019 14:42
@miri64 miri64 added the State: invalid State: The issue / PR is not valid label Nov 18, 2019
@RIOT-OS RIOT-OS locked as resolved and limited conversation to collaborators Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aabadie aabadie aabadie requested changes

@fjmolinas fjmolinas fjmolinas requested changes

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 State: invalid State: The issue / PR is not valid Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.01
Development

Successfully merging this pull request may close these issues.
3 participants
@miri64 @aabadie @fjmolinas