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.

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

build system: Improve failure mode for FEAUTRES_REQUIRED_ANY #20408

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 21, 2024

Contribution description

Failing to provide any of the required features can provide a message such as:

There are unsatisfied feature requirements: periph_uart|periph_lpuart

This can be confusing and may hide the actual. E.g. above message was generated when using SPI on the msb-430 and stdio_uart. However, the MSB-430 board does provide periph_uart, so this looks like a bug in the feature resolution. This changes the failure mode of FEATURES_REQUIRED_ANY to just pick the first of the alternatives given if none of the alternative is usable, which gives in the example the following message instead:

The following features may conflict: periph_spi periph_uart
Rationale: Both SPI and UART are provided by the same USART
           peripheral

The output is less surprising and can provide non-obvious reasons why FEATURES_REQUIRED_ANY failed to pick a feature. The downside is that the alternatives are no longer visible. However, that output likely was so confusing this might be for the best.

Testing procedure

The build system unit tests have been updated to match the new failure mode.

Issues/PRs references

Needed for #20366, as the current failure mode of requiring periph_uart|periph_lpuart when neither periph_uart nor periph_lpuart is usable will trigger the test for requiring unknown features.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: build system Area: Build system labels Feb 21, 2024
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Feb 21, 2024
@maribu maribu added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Feb 21, 2024
@riot-ci
Copy link

riot-ci commented Feb 21, 2024

Murdock results

✔️ PASSED

d296523 build system: Improve failure mode for FEAUTRES_REQUIRED_ANY

Success Failures Total Runtime
9997 0 9997 09m:31s

Artifacts

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Soft-ACK as I'm not very familiar with RIOT's make system, but looks sensible to me. Just two comment nits.

makefiles/features_check.inc.mk Outdated Show resolved Hide resolved
makefiles/features_check.inc.mk Outdated Show resolved Hide resolved
Failing to provide any of the required features can provide a message
such as:

    There are unsatisfied feature requirements: periph_uart|periph_lpuart

This can be confusing and may hide the actual. E.g. above message
was generated when using SPI on the `msb-430` and `stdio_uart`. However,
the MSB-430 board *does* provide `periph_uart`, so this looks like a bug
in the feature resolution. This changes the failure mode of
`FEATURES_REQUIRED_ANY` to just pick the first of the alternatives
given if none of the alternative is usable, which gives in the example
the following message instead:

    The following features may conflict: periph_spi periph_uart
    Rationale: Both SPI and UART are provided by the same USART
               peripheral

The output is less surprising and can provide non-obvious reasons
why `FEATURES_REQUIRED_ANY` failed to pick a feature. The downside is
that the alternatives are no longer visible. However, that output
likely was so confusing this might be for the best.

Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
@maribu maribu force-pushed the makefiles/features_check.inc.mk branch from 01389ba to d296523 Compare February 22, 2024 13:03
@maribu
Copy link
Member Author

maribu commented Feb 22, 2024

Thanks. I applied the suggestions and squashed.

@maribu maribu added this pull request to the merge queue Feb 22, 2024
Merged via the queue into RIOT-OS:master with commit 7ddae48 Feb 22, 2024
26 checks passed
@maribu maribu deleted the makefiles/features_check.inc.mk branch March 1, 2024 17:38
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
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 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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants