Skip to content

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

Closed
miri64 wants to merge 2 commits intoRIOT-OS:masterfrom
miri64:stdio_uart/enh/make-feature
Closed

Makefile.dep: make stdio_uart a feature required by ethos#12739
miri64 wants to merge 2 commits intoRIOT-OS:masterfrom
miri64:stdio_uart/enh/make-feature

Conversation

@miri64
Copy link
Copy Markdown
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
Copy Markdown
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
endif
ifeq (,$(filter stdio_rtt stdio_cdc_acm,$(USEMODULE)))
USEMODULE += stdio_uart
FEATURES_PROVIDED += stdio_uart
Copy link
Copy Markdown
Contributor

@aabadie aabadie Nov 18, 2019

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
Copy Markdown
Member Author

@miri64 miri64 Nov 18, 2019

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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