Skip to content

Kconfig: make app.config and app.config.test mutually exclusive #16092

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 1 commit into from
Feb 26, 2021
Merged

Kconfig: make app.config and app.config.test mutually exclusive #16092

merged 1 commit into from
Feb 26, 2021

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Feb 25, 2021 •
edited
Loading

Contribution description

When an application contains an app.config for configuration using Kconfig without dependency resolution and an app.config.test for configuration using Kconfig with dependency resolution, the app.config file is still loaded, setting legacy symbols, and this is causing errors with Kconfig (see e.g. #15951 (comment)). This makes app.config and app.config.test mutually exclusive and their selection as application base configuration based on the setting of TEST_KCONFIG=1.

Testing procedure

I cherry-picked this PR on top of #15951 and try to build the test there with and without TEST_KCONFIG=1 (currently causing an easy to solve merge conflict

$ git remote add miri64 git@github.com:miri64/RIOT.git
$ git fetch miri64
$ git checkout miri64/congure/feat/initial
$ git cherry-pick miri64/kconfig/fix/app.config-mutual-exclusive
$ TEST_KCONFIG=1 make -C tests/congure_test -j
$ TEST_KCONFIG=1 make -C tests/congure_test -j clean   # otherwise one get's errors
$ make -C tests/congure_test -j
<!--
Details steps to test your contribution:
- which test/example to compile for which board and is there a 'test' command
- how to know that it was not working/available in master
- the expected success test output
-->


### Issues/PRs references

<!--
Examples: Fixes #1234. See also #5678. Depends on PR #9876.

Please use keywords (e.g., fixes, resolve) with the links to the issues you
resolved, this way they will be automatically closed when your pull request
is merged. See https://help.github.com/articles/closing-issues-using-keywords/.
-->

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: Kconfig Area: Kconfig integration labels Feb 25, 2021
@miri64 miri64 mentioned this pull request Feb 25, 2021
Merged
@MrKevinWeiss
Copy link
Contributor

As long as people know that they need to double the efforts until the migration is complete that is OK with me but I will let @leandrolanzieri have the final word.

@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 25, 2021
MrKevinWeiss
MrKevinWeiss approved these changes Feb 26, 2021
View reviewed changes
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

I guess nobody opposes the idea and the implementation seems sound so ACK!

@MrKevinWeiss
Copy link
Contributor

Lets holdoff on the merge for a second. Do other app.config parameter get used with app.config.test?

@MrKevinWeiss
Copy link
Contributor

It looks like app.config only exists in some tests/gnrc_* tests/kconfig

None of which contain a app.config.test so I guess we are OK...

@MrKevinWeiss MrKevinWeiss merged commit a31ffdc into RIOT-OS:master Feb 26, 2021
@miri64 miri64 deleted the kconfig/fix/app.config-mutual-exclusive branch February 26, 2021 09:06
@miri64
Copy link
Member Author

miri64 commented Feb 26, 2021

Thanks for the review!

@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrKevinWeiss MrKevinWeiss MrKevinWeiss approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
Area: Kconfig Area: Kconfig integration 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 2021.04
Development

Successfully merging this pull request may close these issues.
4 participants
@miri64 @MrKevinWeiss @kaspar030 @leandrolanzieri