Skip to content

build system: error on conflicting features #21013

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 2 commits into from
Nov 22, 2024
Merged

build system: error on conflicting features #21013

merged 2 commits into from
Nov 22, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 20, 2024

Contribution description

When the build system is aware of an invalid configuration - such as multiple C libs being used or a single piece of hardware being used in two conflicting roles - this now is treated as an error rather than a warning.

Because of this, the app tests/build_system/warn_conflict can no longer be build and is obsolete. It is removed in the second commit.

Note

The static tests in dist/tools/feature_resolution/check.sh already covers testing the build system feature resolution, including feature conflicts.

Testing procedure

master

$ FEATURES_REQUIRED='periph_rtt periph_rtc' BOARD=nucleo-f103rb make -C examples/default
make: Entering directory '/home/marian.buschsieweke@ml-pa.loc/Repos/software/RIOT/master/examples/default'
The following features may conflict: periph_rtc periph_rtt
Rationale: On the STM32F1, the RTC and RTT map to the same hardware peripheral. Only one standard C library can be used. Only one GPIO IRQ implementation can be used. Package tinyUSB is not yet compatible with periph_usbdev.

EXPECT undesired behaviour!
[...]

==> builds successfully a defunct app

This PR

$ FEATURES_REQUIRED='periph_rtt periph_rtc' BOARD=nucleo-f103rb make -C examples/default 
make: Entering directory '/home/marian.buschsieweke@ml-pa.loc/Repos/software/RIOT/master/examples/default'
The following features conflict: periph_rtc periph_rtt
Rationale: On the STM32F1, the RTC and RTT map to the same hardware peripheral. Only one standard C library can be used. Only one GPIO IRQ implementation can be used. Package tinyUSB is not yet compatible with periph_usbdev.
/home/marian.buschsieweke@ml-pa.loc/Repos/software/RIOT/master/examples/default/../../Makefile.include:1006: *** 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/marian.buschsieweke@ml-pa.loc/Repos/software/RIOT/master/examples/default'

==> does not build a defunct app

Issues/PRs references

None

@maribu
build system: error on conflicting features
f166e74 
When the build system is aware of an invalid configuration - such as
multiple C libs being used or a single piece of hardware being used in
two conflicting roles - this now is treated as an error rather than
a warning.
@maribu
tests/build_system: drop warn_conflict app
d8304fa 
This is no longer a warning but an error.

Also: This is already covered by the static tests via
`dist/tools/feature_resolution/check.sh`, which uses a lot less CPU
time in the CI.
@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 Nov 20, 2024
@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework labels Nov 20, 2024
@riot-ci
Copy link

riot-ci commented Nov 20, 2024

Murdock results

✔️ PASSED

d8304fa tests/build_system: drop warn_conflict app

Success Failures Total Runtime
10249 0 10249 15m:41s

Artifacts

@Enoch247
Copy link
Contributor

Have you done any digging through git logs or old PRs to verify that conflict resolution has always been a warning and not an error. I'd like to make sure that we aren't reverting a feature that was needed for forgotten reasons.

@maribu
Copy link
Member Author

maribu commented Nov 21, 2024

It seems to be added ~10 years ago as a warning in f9a79ee and unchanged ever since.

Enoch247
Enoch247 approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@Enoch247 Enoch247 left a comment

Choose a reason for hiding this comment

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

I tested, and the change works as described.

If a user really wishes to ignore the conflict, they can still bypass this error by setting CONTINUE_ON_EXPECTED_ERRORS=1.

Shall I merge?

@Enoch247 Enoch247 enabled auto-merge November 22, 2024 13:46
@Enoch247 Enoch247 added this pull request to the merge queue Nov 22, 2024
Merged via the queue into RIOT-OS:master with commit d9593c7 Nov 22, 2024
29 checks passed
@maribu maribu deleted the build_system/feature-conflict-is-error branch November 22, 2024 21:59
@maribu
Copy link
Member Author

maribu commented Nov 22, 2024

Thx :)

@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Enoch247 Enoch247 Enoch247 approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: doc Area: Documentation Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2025.01
Development

Successfully merging this pull request may close these issues.
4 participants
@maribu @riot-ci @Enoch247 @MrKevinWeiss