kconfig: introduce migration test in CI by leandrolanzieri · Pull Request #14626 · RIOT-OS/RIOT

leandrolanzieri · 2020-07-28T07:24:05Z

Contribution description

This PR splits some initial commits from #14055:

The Murdock script is modified to introduce a test that can be used during the current phase of the migration. The test consists on building certain board/app pairs using Kconfig module modelling to get the list of modules to compile. Then, the binary hash is compared against the hash of the application built normally.
Some initial modules have been modelled in Kconfig, just the bare minimum to get examples/hello-world to build for samr21-xpro (this means that, for instance, I did not include the whole STDIO modelling like in [PoC] Module dependency modelling using Kconfig #14055).

The idea is to build our way application by application, which will get easier as we model more modules. This test will try to ensure also some synchronization with the current makefile-based dependency resolution system.

The proposed way to do the migration, in order to not interfere with the existing Kconfig files, is to model modules using the MOD_ prefix, and guard all new symbols with the TEST_KCONFIG symbol (which is set when the environmental variable TEST_KCONFIG is set). Also, so we don't raise issues with the current kconfig_features test, new features (that only exist in Kconfig) have the HAVE_ prefix for now, which can be changed in the future.

The TEST_KCONFIG variable will serve test purposes during migration, it switches from the makefile-based dependency resolution and Kconfig.

Question: Some modules like sys and board have been modelled as non-visible, should they have a prompt to be configurable?

Testing procedure

Some things to pay special attention to:
- Modelling on peripheral drivers
- The way common configuration files are merged together (via KCONFIG_ADD_CONFIG variable) prior to Kconfig running
- The test itself
Green CI (check the output for the build of examples/hello-world for samr21-xpro:gnu, you should see the output of this test)
Alternatively one could run something like /bin/sh -c ". ./.murdock; JOBS=4 compile examples/hello-world samr21-xpro:gnu"

Issues/PRs references

Part of #14055

cgundogan

A few minor comments and questions so far.

.murdock

drivers/periph_common/Kconfig.flashpage

drivers/periph_common/Kconfig.gpio

drivers/periph_common/Kconfig.i2c

drivers/periph_common/Kconfig.spi

makefiles/kconfig.mk

leandrolanzieri · 2020-08-04T08:25:18Z

Rebased to fix conflicts

leandrolanzieri · 2020-08-04T09:32:14Z

And again

makefiles/vars.inc.mk

cgundogan

To me the changes look sane. @kaspar030 @fjmolinas any comments?

fjmolinas · 2020-08-06T07:52:03Z

To me the changes look sane. @kaspar030 @fjmolinas any comments?

The change makes sense to me as well, should it be given a run through murdock?

fjmolinas · 2020-08-11T10:21:58Z

Looks like all is green, @cgundogan can @leandrolanzieri squash (and rebase?)?

cgundogan · 2020-08-11T10:46:15Z

@cgundogan can @leandrolanzieri squash (and rebase?)?

sure, please go ahead @leandrolanzieri

leandrolanzieri · 2020-08-11T11:17:26Z

Squashed and rebased. @fjmolinas @cgundogan do you think it would make sense to temporary introduce a commit that makes the test fail just to be sure that it works? I was thinking of some pseudomodule that would still compile in both versions but would make the binary check fail. Ideas?

cgundogan · 2020-08-11T11:57:03Z

@fjmolinas @cgundogan do you think it would make sense to temporary introduce a commit that makes the test fail just to be sure that it works?

yes, sounds like a great idea (:

leandrolanzieri · 2020-08-12T09:38:01Z

I just added a dummy pseudomodule in the application. Now the binary hashes should not match, and the application compile test should fail.

leandrolanzieri · 2020-08-12T10:06:02Z

It seems to have triggered the expected error.

cgundogan · 2020-08-12T10:11:57Z

It seems to have triggered the expected error.

Yup, looks good. I think it's safe now to remove the commit and get this PR merged.

This models cortexm_common and cortexm_common_periph modules.

Select it from cortexm_common module as it is always needed.

This switch allows to test the module dependency modelling during the Kconfig migration. When set, it will use the symbols prefixed with CONFIG_MOD_ defined by Kconfig as the list of modules to compile.

This holds a list of .config files to be merged. For more details see kconfig.mk.

The configuration file is included by samd21 so it is merged when using Kconfig.

This adds a list of board/application pairs which should be tested. The test consists on comparing the binaries generated using dependency resolution in Makefile and in Kconfig.

leandrolanzieri · 2020-08-12T10:23:07Z

Commit removed

cgundogan · 2020-08-13T12:10:53Z

Murdock repeatedly built this PR with success. Here we goooo.

aabadie · 2020-08-22T11:03:51Z

boards/Kconfig

    help
        Name of the currently selected board.
+
+config MOD_BOARD


In a previous PR, I already commented that the config for modules should be prefixed with a more explicit name MODULE instead of MOD. Too bad this was merged without taking it into account.

Oh sorry, I thought that as there was no response to my reply on the original comment this was OK. I think it's good to have the MODULE prefix but I did not want to mix them up just now, as those currently come from the USEMODULE list. Once migration is done we will get rid of the generated module list (Kconfig.dep) and then we can rename the MOD symbols.

there was no response to my reply on the original comment this was OK

Just forgot about that sorry. If unsure better ask for a confirmation before.

I think it's good to have the MODULE prefix but I did not want to mix them up just now, as those currently come from the USEMODULE list

This I don't get. Why not do things right from the start and use the MODULE prefix ?

Once migration is done we will get rid of the generated module list (Kconfig.dep) and then we can rename the MOD symbols.

I don't buy this strategy. There are too many modules and I fear that it might become harder to change once the migration is complete. I would prefer to change that as early as possible and before it's too late.

This I don't get. Why not do things right from the start and use the MODULE prefix ?

If symbols are renamed on what has been migrated so far, one gets the following warning:

warning: default on the choice symbol MODULE_STDIO_UART (defined at bin/samr21-xpro/generated/Kconfig.dep:76, RIOT/sys/Kconfig.stdio:23) will have no effect, as defaults do not affect choice symbols

This is because Kconfig.dep sets the default of MODULE_STDIO_UART to y but in the module modelling this symbol is inside a choice.

We could do it the other way around, changing the prefix for the automatic generated symbols in Kconfig.dep to MOD_ and use MODULE_ for the proper ones? This would entail doing some renaming on the Kconfigs so far (changing the prefix on the dependencies of the menuconfigs), but the actual module symbols would need no renaming afterwards.

We could do it the other way around, changing the prefix for the automatic generated symbols in Kconfig.dep to MOD_ and use MODULE_ for the proper ones?

Is it possible to prefix with _ ? for the internally generated symbols. Something like _MODULE_<module> ?

I disagree about the need to accelerate now because I think we are going into the wrong naming direction. If we improve this as early as possible, there will be less renaming work after.
So a good naming consensus should be taken before moving forward. The term MOD_ is very misleading for me since it could mean a modified thing. In the case of a board, this is even more worse.
It's also unclear to me how the internal of these symbols for module work at the moment. It would be great if that could be explained somewhere (maybe it's already the case, I haven't check, and I'll be happy if someone could give me some links).

It's also unclear to me how the internal of these symbols for module work at the moment. It would be great if that could be explained somewhere

If you mean the current automatically generated symbols:

Documentation on the steps in the integration of Kconfig in the build process (step 1): http://doc.riot-os.org/kconfig-in-riot.html#kconfig-steps-build-process

Appendix A on how these symbols should be used to have optionally configurable modules: http://doc.riot-os.org/kconfig-in-riot.html#kconfig-appendix-a

Documentation in the Makefile that generates the symbols:

RIOT/makefiles/kconfig.mk

Lines 118 to 127 in 7a3c371

# Build a Kconfig file defining all used modules and packages. This is done by

# defining symbols like 'MODULE_<MODULE_NAME>' or PKG_<PACKAGE_NAME> which

# default to 'y'. Then, every module and package Kconfig menu will depend on

# that symbol being set to show its options.

# Do nothing when testing Kconfig module dependency modelling.

$(KCONFIG_GENERATED_DEPENDENCIES): FORCE | $(GENERATED_DIR)

$(Q)printf "%s " $(USEMODULE_W_PREFIX) $(USEPKG_W_PREFIX) \

| awk 'BEGIN {RS=" "}{ gsub("-", "_", $$0); \

printf "config %s\n\tbool\n\tdefault y\n", toupper($$0)}' \

| $(LAZYSPONGE) $(LAZYSPONGE_FLAGS) $@

If you mean the new symbols used in migration:

There is an extensive documentation in the PoC that still can't be merged until the migration is finished a93c011#diff-bd4a0704958a0d6c5cd69178d5de8f69 It tries to clarify the usage of symbols for modules and features, and mention good practices.

@aabadie would you agree with the following prefix changes?

Current prefix New prefix Symbol description

MODULE_ USEMODULE_ Symbols generated from USEMODULE variable. Needed only during migration.

KCONFIG_MODULE_ KCONFIG_USEMODULE_ Symbols used in menuconfigs to allow optional configuration of used modules. Depend on USEMODULE_ symbols.

PKG_ USEPKG_ Symbols generated from USEPKG variable. Needed only during migration.

KCONFIG_PKG_ KCONFIG_USEPKG_ Symbols used in menuconfigs to allow optional configuration of used packages. Depend onUSEPKG_ symbols.

MOD_ MODULE_ Symbols that model modules in Kconfig.

PKG_ PACKAGE_ Symbols that model packages in Kconfig.

would you agree with the following prefix changes?

Yes!

Please see #14904

leandrolanzieri added Area: CI Area: Continuous Integration of RIOT components Area: Kconfig Area: Kconfig integration labels Jul 28, 2020

leandrolanzieri requested review from aabadie, cgundogan, fjmolinas, kaspar030 and maribu July 28, 2020 07:24

leandrolanzieri requested review from DipSwitch, MrKevinWeiss, benpicco, biboc, dylad, haukepetersen, jia200x, keestux and thomaseichinger as code owners July 28, 2020 07:24

cgundogan requested changes Jul 28, 2020

View reviewed changes

leandrolanzieri added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Jul 29, 2020

leandrolanzieri force-pushed the pr/kconfig/test_modules_kconfig branch from 0b88863 to 5a780cd Compare August 4, 2020 08:23

leandrolanzieri force-pushed the pr/kconfig/test_modules_kconfig branch from 5a780cd to 4d38b71 Compare August 4, 2020 09:31

cgundogan reviewed Aug 4, 2020

View reviewed changes

makefiles/vars.inc.mk Outdated Show resolved Hide resolved

cgundogan approved these changes Aug 4, 2020

View reviewed changes

leandrolanzieri force-pushed the pr/kconfig/test_modules_kconfig branch from 41f96ce to 6a2516d Compare August 6, 2020 07:39

cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 6, 2020

leandrolanzieri force-pushed the pr/kconfig/test_modules_kconfig branch from 6a2516d to 443ef00 Compare August 11, 2020 11:15

leandrolanzieri and others added 14 commits August 12, 2020 12:22

core: Add modules to Kconfig

f5e57fb

kconfig: Add default modules' symbols

4ad2180

cpu/cortexm_common: Model cortexm_common modules

2d53003

This models cortexm_common and cortexm_common_periph modules.

cpu/sam0_common: add sam0_common_periph module to Kconfig

0ddd288

cpu: Add 'periph' module to Kconfig

b98527e

Select it from cortexm_common module as it is always needed.

drivers/periph_common: Add modules to Kconfig

14499e3

sys: add newlib modules to Kconfig

67b2519

sys/stdio: add module to Kconfig

4f2494f

sys/pm_layered: Add module to Kconfig

e763879

Makefile.include: introduce TEST_KCONFIG switch

0145418

This switch allows to test the module dependency modelling during the Kconfig migration. When set, it will use the symbols prefixed with CONFIG_MOD_ defined by Kconfig as the list of modules to compile.

makefiles: introduce KCONFIG_ADD_CONFIG variable

180ee6e

This holds a list of .config files to be merged. For more details see kconfig.mk.

cpu/sam0_common: Add default Kconfig configuration

f4e651e

The configuration file is included by samd21 so it is merged when using Kconfig.

cpu/cortexm_common: add default Kconfig configuration

7dc6639

murdock: add test for Kconfig dependency modelling

8f1fd3a

This adds a list of board/application pairs which should be tested. The test consists on comparing the binaries generated using dependency resolution in Makefile and in Kconfig.

leandrolanzieri force-pushed the pr/kconfig/test_modules_kconfig branch from 0a810c5 to 8f1fd3a Compare August 12, 2020 10:22

cgundogan merged commit f64511d into RIOT-OS:master Aug 13, 2020

leandrolanzieri deleted the pr/kconfig/test_modules_kconfig branch August 13, 2020 12:11

aabadie reviewed Aug 22, 2020

View reviewed changes

leandrolanzieri mentioned this pull request Aug 24, 2020

Kconfig: add modules and configurations to run tests/periph_* #14777

Merged

leandrolanzieri mentioned this pull request Aug 31, 2020

treewide: modify Kconfig symbol prefixes #14904

Merged

aabadie mentioned this pull request May 23, 2023

[Tracking]: modeling dependencies in Kconfig #16875

Closed

	# Build a Kconfig file defining all used modules and packages. This is done by
	# defining symbols like 'MODULE_<MODULE_NAME>' or PKG_<PACKAGE_NAME> which
	# default to 'y'. Then, every module and package Kconfig menu will depend on
	# that symbol being set to show its options.
	# Do nothing when testing Kconfig module dependency modelling.
	$(KCONFIG_GENERATED_DEPENDENCIES): FORCE \| $(GENERATED_DIR)
	$(Q)printf "%s " $(USEMODULE_W_PREFIX) $(USEPKG_W_PREFIX) \
	\| awk 'BEGIN {RS=" "}{ gsub("-", "_", $$0); \
	printf "config %s\n\tbool\n\tdefault y\n", toupper($$0)}' \
	\| $(LAZYSPONGE) $(LAZYSPONGE_FLAGS) $@

Current prefix	New prefix	Symbol description
`MODULE_`	`USEMODULE_`	Symbols generated from `USEMODULE` variable. Needed only during migration.
`KCONFIG_MODULE_`	`KCONFIG_USEMODULE_`	Symbols used in `menuconfig`s to allow optional configuration of used modules. Depend on `USEMODULE_` symbols.
`PKG_`	`USEPKG_`	Symbols generated from `USEPKG` variable. Needed only during migration.
`KCONFIG_PKG_`	`KCONFIG_USEPKG_`	Symbols used in `menuconfig`s to allow optional configuration of used packages. Depend on`USEPKG_` symbols.
`MOD_`	`MODULE_`	Symbols that model modules in Kconfig.
`PKG_`	`PACKAGE_`	Symbols that model packages in Kconfig.

Conversation

leandrolanzieri commented Jul 28, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

cgundogan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leandrolanzieri commented Aug 4, 2020

Uh oh!

leandrolanzieri commented Aug 4, 2020

Uh oh!

Uh oh!

cgundogan left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Aug 6, 2020

Uh oh!

fjmolinas commented Aug 11, 2020

Uh oh!

cgundogan commented Aug 11, 2020

Uh oh!

leandrolanzieri commented Aug 11, 2020

Uh oh!

cgundogan commented Aug 11, 2020

Uh oh!

leandrolanzieri commented Aug 12, 2020

Uh oh!

leandrolanzieri commented Aug 12, 2020

Uh oh!

cgundogan commented Aug 12, 2020

Uh oh!

leandrolanzieri commented Aug 12, 2020

Uh oh!

cgundogan commented Aug 13, 2020

Uh oh!

aabadie Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

leandrolanzieri Aug 24, 2020 •

edited

Loading

aabadie Aug 24, 2020 •

edited

Loading

aabadie Aug 28, 2020 •

edited

Loading