Skip to content

makefiles/kconfig: include out.config only when running Kconfig #16064

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 23, 2021
Merged

makefiles/kconfig: include out.config only when running Kconfig #16064

merged 1 commit into from
Feb 23, 2021

Conversation

leandrolanzieri
Copy link
Contributor

@leandrolanzieri leandrolanzieri commented Feb 22, 2021 •
edited
Loading

Contribution description

This modifies the Kconfig makefile to only source the out.config file into the build system when Kconfig has to run. There is no need to include it otherwise.

Testing procedure

We need to make sure this change keeps Kconfig working across different workflows. I tested the following:

  1. make menuconfig > make all should load out.config
  2. env TEST_KCONFIG=1 make should load out.config, using dependency modelling
  3. make (on an application without Kconfig files) should not load out.config
  4. make menuconfig > make clean all should not load out.config (because of the clean)
  5. Successive configurations via .config files or menuconfig should always load out.config until clean is executed

To check if there are Kconfig symbols loaded to the build system, one can add something like the following to an application makefile, and run the workflows described above (and more!) on native:

diff --git a/examples/hello-world/Makefile b/examples/hello-world/Makefile
index 258d8e9baf..e7e7583bc1 100644
--- a/examples/hello-world/Makefile
+++ b/examples/hello-world/Makefile
@@ -16,3 +16,9 @@ DEVELHELP ?= 1
 QUIET ?= 1
 
 include $(RIOTBASE)/Makefile.include
+
+ifndef CONFIG_HAS_ARCH_32BIT
+  $(info ->> NOT using Kconfig)
+else
+  $(info ->> using Kconfig)
+endif

Issues/PRs references

Addresses the problem described in #16009 (comment)

@leandrolanzieri leandrolanzieri added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system Area: Kconfig Area: Kconfig integration labels Feb 22, 2021
@MrKevinWeiss
Copy link
Contributor

Is there any way we can have the clean happen before menuconfig?

I find myself doing a lot of:

make menuconfig
TEST_KCONFIG=1 make menuconfig
# Oops error
make clean
TEST_KCONFIG=1 make menuconfig

It would be nicer if I could just go

make clean menuconfig
TEST_KCONFIG=1 make clean menuconfig

Just a thought though.

@MrKevinWeiss
Copy link
Contributor

or have a different target clean-menuconfig or something

@fjmolinas
Copy link
Contributor

@MrKevinWeiss does this solve your issue;

diff --git a/makefiles/kconfig.mk b/makefiles/kconfig.mk
index 96f9b57459..d9f523f827 100644
--- a/makefiles/kconfig.mk
+++ b/makefiles/kconfig.mk
@@ -115,7 +115,7 @@ USEPKG_W_PREFIX = $(addprefix USEPKG_,$(USEPKG))
 # Opens the menuconfig interface for configuration of modules using the Kconfig
 # system. It will try to update the autoconf.h, which will update if needed
 # (i.e. out.config changed).
-menuconfig: $(MENUCONFIG) $(KCONFIG_OUT_CONFIG)
+menuconfig: $(MENUCONFIG) $(KCONFIG_OUT_CONFIG) | $(CLEAN)
        $(Q)KCONFIG_CONFIG=$(KCONFIG_OUT_CONFIG) $(MENUCONFIG) $(KCONFIG)
        $(MAKE) $(KCONFIG_GENERATED_AUTOCONF_HEADER_C)

@fjmolinas
Copy link
Contributor

From local tests this seems to work correctly for me, @leandrolanzieri should some of this be tested in docker as well? (BUILD_IN_DOCKER)

@MrKevinWeiss
Copy link
Contributor

@MrKevinWeiss does this solve your issue;

I think the clean needs to happen before the menuconfig, with this I need to run make clean menuconfig twice to clear the error when switching from TEST_KCONFIG=0 to TEST_KCONFIG=1

@MrKevinWeiss
Copy link
Contributor

Maybe this isn't part of this PR though.

@leandrolanzieri
Copy link
Contributor Author

@MrKevinWeiss does this solve your issue;

diff --git a/makefiles/kconfig.mk b/makefiles/kconfig.mk
index 96f9b57459..d9f523f827 100644
--- a/makefiles/kconfig.mk
+++ b/makefiles/kconfig.mk
@@ -115,7 +115,7 @@ USEPKG_W_PREFIX = $(addprefix USEPKG_,$(USEPKG))
 # Opens the menuconfig interface for configuration of modules using the Kconfig
 # system. It will try to update the autoconf.h, which will update if needed
 # (i.e. out.config changed).
-menuconfig: $(MENUCONFIG) $(KCONFIG_OUT_CONFIG)
+menuconfig: $(MENUCONFIG) $(KCONFIG_OUT_CONFIG) | $(CLEAN)
        $(Q)KCONFIG_CONFIG=$(KCONFIG_OUT_CONFIG) $(MENUCONFIG) $(KCONFIG)
        $(MAKE) $(KCONFIG_GENERATED_AUTOCONF_HEADER_C)

I think there might be a problem with this, regarding the generation of out.config that should happen before, but I haven't test it yet.

@leandrolanzieri
Copy link
Contributor Author

From local tests this seems to work correctly for me, @leandrolanzieri should some of this be tested in docker as well? (BUILD_IN_DOCKER)

Not sure if that would change something, but it would be good just to be sure

@fjmolinas
Copy link
Contributor

Works as expected for me.

@fjmolinas
Copy link
Contributor

@MrKevinWeiss other then the clean comment is this one OK for you?

@MrKevinWeiss
Copy link
Contributor

Yup! A very clean solution 😆

@MrKevinWeiss MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 23, 2021
fjmolinas
fjmolinas approved these changes Feb 23, 2021
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.

ACK!

@fjmolinas fjmolinas merged commit ddc5dea into RIOT-OS:master Feb 23, 2021
@leandrolanzieri
Copy link
Contributor Author

Thanks for reviewing !

@leandrolanzieri leandrolanzieri deleted the pr/makefiles/kconfig_include_config_only_when_running branch February 23, 2021 17:59
@MrKevinWeiss MrKevinWeiss mentioned this pull request Feb 25, 2021
Merged
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
@leandrolanzieri leandrolanzieri added the Release notes: ignored Set on PRs that have been considered for inclusion in the current release's notes but were minor. label Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

Assignees
No one assigned
Labels
Area: build system Area: Build system 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 Release notes: ignored Set on PRs that have been considered for inclusion in the current release's notes but were minor. 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
@leandrolanzieri @MrKevinWeiss @fjmolinas @kaspar030