scripts: kconfig: Replace conf with Python #5569

carlescufi · 2018-01-05T12:13:15Z

carlescufi · 2018-01-05T12:15:11Z

This completely replaces conf and merge_config with a single kconfig.py Python script.

carlescufi · 2018-01-05T12:18:08Z

cmake/kconfig.cmake

This is a leftover from debugging, to check that the kconfig/ folder, which is no longer being generated, is not required. Can be removed.

+1 to removing it.

carlescufi · 2018-01-05T12:18:32Z

scripts/kconfig/kconfig.py

We can replace the header of the .config file if we want to

carlescufi · 2018-01-05T12:18:46Z

scripts/kconfig/kconfig.py

We can replace the header of the autoconf.h file if we want to

carlescufi · 2018-01-05T12:19:05Z

scripts/kconfig/kconfiglib.py

This is equivalent to an MIT license: https://en.wikipedia.org/wiki/ISC_license

nashif · 2018-01-05T12:20:47Z

you probably want to mention in the kconfiglib commit that it will replace the current version in doc. So it is not something new that need to be approved.

SebastianBoe · 2018-01-05T12:22:24Z

When building hello_world with BOARD=nrf52_pca10040

sebo@mach:~/zephyr/samples/hello_world$ cb
-- Found PythonInterp: /usr/bin/python3 (found suitable version "3.5.2", minimum required is "3.4")
-- Selected BOARD nrf52_pca10040
Zephyr version: 1.10.99
warning: COVERAGE was assigned the value "y" but got the value "n" -- check dependencies
-- Generating zephyr/include/generated/generated_dts_board.h
-- The C compiler identification is GNU 6.2.0

Can we improve the UX here, make it more obvious that the warning is coming from Kconfig for example?

SebastianBoe · 2018-01-05T12:25:52Z

I liked that the old system listed which files were used to merge configs,
I don't think we should remove that.

Using /home/sebo/zephyr/boards/arm/nrf52_pca10040/nrf52_pca10040_defconfig as base
Merging /home/sebo/zephyr/samples/hello_world/prj.conf
Merging /home/sebo/zephyr/tests/include/test.config

carlescufi · 2018-01-05T12:26:03Z

Note there's no support for menuconfig or any other graphical configuration system yet. I want to concentrate on the sanitycheck first.

SebastianBoe · 2018-01-05T12:26:37Z

Ah, ok, please mark as WIP/RFC.
EDIT: Or, I guess this is merge-able? The GUI tools can be used alongside this patch can't they?

codecov-io · 2018-01-05T12:35:17Z

Codecov Report

Merging #5569 into master will increase coverage by 1.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5569      +/-   ##
==========================================
+ Coverage   52.51%   53.55%   +1.04%     
==========================================
  Files         430      428       -2     
  Lines       41190    41189       -1     
  Branches     7916     7855      -61     
==========================================
+ Hits        21631    22060     +429     
+ Misses      19024    19018       -6     
+ Partials      535      111     -424

Impacted Files	Coverage Δ
include/misc/slist.h	`73.13% <0%> (-25.42%)`	⬇️
kernel/include/nano_internal.h	`75% <0%> (-25%)`	⬇️
kernel/include/kernel_structs.h	`72.72% <0%> (-20.13%)`	⬇️
kernel/include/ksched.h	`82.5% <0%> (-2.66%)`	⬇️
kernel/poll.c	`90.36% <0%> (-2.41%)`	⬇️
include/misc/dlist.h	`86.2% <0%> (-1.73%)`	⬇️
kernel/mempool.c	`91.17% <0%> (-0.59%)`	⬇️
kernel/include/kernel_offsets.h	`0% <0%> (ø)`	⬆️
drivers/sensor/tmp007/tmp007.c	`0% <0%> (ø)`	⬆️
include/crypto/cipher.h	`0% <0%> (ø)`	⬆️
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4694ffe...13547f8. Read the comment docs.

SebastianBoe · 2018-01-05T12:41:03Z

CMake clean configure time regressed significantly :(

0.76 seconds in master
0.97 seconds now

It seems that it is doing more, which can give us a better UX, so 50ms or so performance degradation would be OK. but 200ms is quite a long time IMHO ...

EDIT: Steps to reproduce: sebo@mach:~/zephyr/samples/hello_world/c$ rm -rf * && time cmake ..

carlescufi · 2018-01-05T13:12:31Z

0.76 seconds in master
0.97 seconds now

I'm sure this is true for Linux. But how about the same on MSYS2 vs running this on a native Windows prompt with a native Python build?

SebastianBoe · 2018-01-05T13:19:20Z

I'm sure this is true for Linux. But how about the same on MSYS2 vs running this on a native Windows prompt with a native Python build?

I don't know ;) I still don't understand why msys runs so slow.

carlescufi · 2018-01-05T13:23:37Z

Additional info: I compared the .config and autconf.h files generated by conf and kconfig.py. While the 2 .config are identical, Kconfiglib adds a #define CONFIG_KERNELVERSION to autoconf.h that was not present in the version generated by conf. I doubt this is an issue but I wanna raise it here in case.

SebastianBoe · 2018-01-05T13:25:44Z

This used to work, and should continue to do so:

cmake ..
# Edit a Kconfig file, e.g. zephyr/misc/Kconfig
cmake ..

Expected result:

Kconfig outputs will reflect Kconfig inputs (Such as Kconfig
files and .config files)

Intuitively; Kconfig is a part of configuration, and configuration is done in CMake's configure stage, so
running the configure stage (cmake .. or make rebuild_cache) should result in a configuration that is synced with the sources.

SebastianBoe · 2018-01-05T13:32:50Z

About KERNELVERSION. The user is supposed to be able to assume, that all # CONFIG_FOO_BAR defines he magically sees as available in his code, should also be available in CMake build scripts.

e.g. he should be able to do

if(CONFIG_KERNELVERSION STREQUAL bla bla)

But I see that CONFIG_KERNELVERSION is not entered into the CMake namespace (because it is not in the .config file), this should be fixed.

nashif · 2018-01-05T13:58:38Z

for the kconfiglib import, it would be cleaned if you import the upstream version and have another patch with the zephyr related changes.

carlescufi · 2018-01-05T14:03:45Z

@nashif I did that initially actually. But then I thought it was better to point to my GitHub repo instead. Maybe we could keep Kconfiglib in a Zephyr-owned repo? The reason is that Zephyr patches != Kconfiglib patches, and it's a pain to maintain later.

carlescufi · 2018-01-05T14:50:21Z

@nashif @SebastianBoe Addressed comments except for the dependency problem, which @SebastianBoe is looking at.

carlescufi · 2018-01-05T15:37:11Z

@SebastianBoe

But I see that CONFIG_KERNELVERSION is not entered into the CMake namespace (because it is not in > the .config file), this should be fixed.

This is not new, conf did not store CONFIG_KERNELVERSION into the .config file either (although it did not store it in the autoconf.h neither). I think this is minor and it also seems to be the case for Linux from what I can gather. I've actually sent an email to @ulfalizer asking exactly this, to clarify.

SebastianBoe · 2018-01-05T15:49:16Z

I think this is minor and it also seems to be the case for Linux from what I can gather. I've actually sent an email to @ulfalizer asking exactly this, to clarify.

I disagree that it is minor. But it doesn't need to block the merge as far as I am concerned. We can just open a bug for it and fix it later if you want.

Some projects use wildcards when sourcing a Kconfig file. Add support for globbing the files that match the wildcards and process them one by one. Origin: https://github.com/carlescufi/Kconfiglib/tree/zephyr Signed-off-by: Carles Cufi <[email protected]>

This commit mirrors c6390d5 in Kconfiglib instead of Kconfig. Origin: https://github.com/carlescufi/Kconfiglib/tree/zephyr Signed-off-by: Carles Cufi <[email protected]>

carlescufi · 2018-01-07T14:58:48Z

I disagree that it is minor. But it doesn't need to block the merge as far as I am concerned. We can just open a bug for it and fix it later if you want.

No need. @ulfalizer has fixed it upstream and I've now updated to the latest Kconfiglib.

carlescufi · 2018-01-08T14:59:40Z

cmake/kconfig.cmake

@SebastianBoe does this look OK to you?

carlescufi · 2018-01-08T15:00:00Z

cmake/host-tools.cmake

Keep looking for conf in non-Windows builds, including MSYS2.

carlescufi · 2018-01-08T15:00:58Z

@nashif @SebastianBoe I now tested it on Windows' cmd.exe, runs fine.

SebastianBoe · 2018-01-08T15:22:17Z

cmake/kconfig.cmake

s/run/${kconfig_target}/

We have been using a fork of the Linux kernel's Kconfig system to configure the Zephyr tree. The issue is that this is a native tool written in C that is not easy to compile for Windows. This patch replaces the use of the conf executable with kconfig.py, a script that uses Kconfiglib to generate the .config and autoconf.h files required to compile Zephyr. Signed-off-by: Carles Cufi <[email protected]>

ulfalizer · 2018-01-10T02:19:04Z

Hello,

Glad it's helpful!

I'm currently looking for a job by the way. If you want me to build a good menuconfig for you or implement other more advanced Kconfig- or build-related features needed in Zephyr, then I'm available for freelance work.

I'm always happy to help if you decide to do it yourselves. Just putting it out there as an option. :)

Cheers,
Ulf

carlescufi · 2018-01-10T09:16:26Z

@ulfalizer

I'm currently looking for a job by the way. If you want me to build a good menuconfig for you or implement other more advanced Kconfig- or build-related features needed in Zephyr, then I'm available for freelance work.

Thanks for letting us know, I will have a talk internally and get back to you privately.

Kconfiglib does not support merging fragments without parsing the entire Kconfig "database". For Zephyr this means we no longer get a performance gain from splitting up the fragment-merging and the .config generation. This patch removes the seperated merge step and it's associated caching mechanism. Now we do a full Kconfig execution on every reconfiguration. Signed-off-by: Sebastian Bøe <[email protected]>

When Kconfiglib was introduced it caused a significant performance issue. This patch uses pruning to mitigate the performance issue. The pruning exploits the fact that before the Kconfig database is parsed we already know what ARCH and BOARD has been selected. So in theory we could prune away all Kconfig sources that are not related to the current ARCH or BOARD. In practice, it is only the Kconfig sources in zephyr/arch/$ARCH and zephyr/board/$ARCH/ that are easy to prune. Still, that is quite a few Kconfig sources. For qemu_x86 this patch reduced the number of parsed Kconfig source files from 632 to 272. This pruning resulted in a incremental reconfiguration (time cmake ..) speedup of 21% (0.56s to 0.46) and a clean build speedup of 4% (Using board qemu_x86 and sample hello_world). Furthermore, it should be easier to maintain ARCH's and BOARD's out-of-tree since the user now has a mechanism to redirect where Kconfig sources are found. But this has not been explored. Signed-off-by: Sebastian Bøe <[email protected]>

carlescufi · 2018-01-10T12:03:26Z

This is ready to be merged as far as @SebastianBoe and myself are concerned.

nashif · 2018-01-10T15:01:23Z

will give it another try today. AFAIK, it looks great already. Need to remove the old Kconfiglib from the tree at the same time. #5622

mbolivar · 2018-01-10T21:14:50Z

This is ready to be merged as far as @SebastianBoe and myself are concerned.

+1

The file kconfig.py was originally introduced by myself in: zephyrproject-rtos#5569 The file was a modified version of a Kconfiglib one, so it makes sense to keep the original license (ISC). Signed-off-by: Carles Cufi <[email protected]>

The file kconfig.py was originally introduced by myself in: #5569 The file was a modified version of a Kconfiglib one, so it makes sense to keep the original license (ISC). Signed-off-by: Carles Cufi <[email protected]>

The file kconfig.py was originally introduced by myself in: zephyrproject-rtos/zephyr#5569 The file was a modified version of a Kconfiglib one, so it makes sense to keep the original license (ISC). (cherry picked from commit 05467ad) Original-Signed-off-by: Carles Cufi <[email protected]> GitOrigin-RevId: 05467ad Change-Id: If40741c5d8430c075585353ee62dd08f61985772 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4812192 Tested-by: ChromeOS Prod (Robot) <[email protected]> Commit-Queue: Fabio Baltieri <[email protected]> Tested-by: Fabio Baltieri <[email protected]> Reviewed-by: Fabio Baltieri <[email protected]>

carlescufi requested review from SebastianBoe and nashif as code owners January 5, 2018 12:13

carlescufi requested a review from mbolivar January 5, 2018 12:13

carlescufi commented Jan 5, 2018

View reviewed changes

scripts/kconfig/kconfig.py Outdated

Copy link

Member Author

carlescufi Jan 5, 2018

Choose a reason for hiding this comment

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

We can replace the header of the .config file if we want to

carlescufi commented Jan 5, 2018

View reviewed changes

scripts/kconfig/kconfig.py Outdated

Copy link

Member Author

carlescufi Jan 5, 2018

Choose a reason for hiding this comment

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

We can replace the header of the autoconf.h file if we want to

carlescufi commented Jan 5, 2018

View reviewed changes

scripts/kconfig/kconfiglib.py Outdated

Copy link

Member Author

carlescufi Jan 5, 2018

Choose a reason for hiding this comment

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

This is equivalent to an MIT license: https://en.wikipedia.org/wiki/ISC_license

carlescufi changed the title ~~scripts: kconfig: Replace conf with Python~~ [RFC] scripts: kconfig: Replace conf with Python Jan 5, 2018

carlescufi force-pushed the kconfiglib branch 2 times, most recently from 50371f0 to 5660ea3 Compare January 5, 2018 14:42

carlescufi force-pushed the kconfiglib branch from 5660ea3 to 3a6a3c1 Compare January 5, 2018 15:19

carlescufi mentioned this pull request Jan 5, 2018

Fix python variant of merge_config #5417

Closed

carlescufi added 2 commits January 7, 2018 15:57

scripts: kconfig: Prefer later defaults

e1b4229

This commit mirrors c6390d5 in Kconfiglib instead of Kconfig. Origin: https://github.com/carlescufi/Kconfiglib/tree/zephyr Signed-off-by: Carles Cufi <[email protected]>

carlescufi force-pushed the kconfiglib branch from ef86a5e to 372cb4f Compare January 7, 2018 14:57

carlescufi force-pushed the kconfiglib branch from 372cb4f to bb2f61e Compare January 8, 2018 14:58

carlescufi commented Jan 8, 2018

View reviewed changes

cmake/host-tools.cmake Outdated

Copy link

Member Author

carlescufi Jan 8, 2018

Choose a reason for hiding this comment

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

Keep looking for conf in non-Windows builds, including MSYS2.

SebastianBoe reviewed Jan 8, 2018

View reviewed changes

cmake/kconfig.cmake Outdated

Copy link

Contributor

SebastianBoe Jan 8, 2018

Choose a reason for hiding this comment

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

s/run/${kconfig_target}/

carlescufi force-pushed the kconfiglib branch from bb2f61e to 0a96841 Compare January 8, 2018 15:24

This was referenced Jan 8, 2018

Provide a python based kconfig processing script, replacement for conf/mconf.. #5419

Closed

merge_config.sh can behave differently from merge_config.py #5374

Closed

SebastianBoe added 2 commits January 10, 2018 11:56

carlescufi changed the title ~~[RFC] scripts: kconfig: Replace conf with Python~~ scripts: kconfig: Replace conf with Python Jan 10, 2018

SebastianBoe approved these changes Jan 12, 2018

View reviewed changes

nashif approved these changes Jan 12, 2018

View reviewed changes

nashif merged commit 4b61bd1 into zephyrproject-rtos:master Jan 12, 2018

carlescufi mentioned this pull request Aug 24, 2023

scripts: kconfig: Add copyright and license to kconfig.py #61842

Merged

scripts: kconfig: Replace conf with Python #5569

scripts: kconfig: Replace conf with Python #5569

Uh oh!

Conversation

carlescufi commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlescufi commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nashif commented Jan 5, 2018

Uh oh!

SebastianBoe commented Jan 5, 2018

Uh oh!

SebastianBoe commented Jan 5, 2018

Uh oh!

carlescufi commented Jan 5, 2018

Uh oh!

SebastianBoe commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SebastianBoe commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlescufi commented Jan 5, 2018

Uh oh!

SebastianBoe commented Jan 5, 2018

Uh oh!

carlescufi commented Jan 5, 2018

Uh oh!

SebastianBoe commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SebastianBoe commented Jan 5, 2018

Uh oh!

nashif commented Jan 5, 2018

Uh oh!

carlescufi commented Jan 5, 2018

Uh oh!

carlescufi commented Jan 5, 2018

Uh oh!

carlescufi commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SebastianBoe commented Jan 5, 2018

Uh oh!

carlescufi commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulfalizer commented Jan 10, 2018

Uh oh!

carlescufi commented Jan 10, 2018

Uh oh!

carlescufi commented Jan 10, 2018

Uh oh!

nashif commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

carlescufi commented Jan 5, 2018 •

edited

Loading

SebastianBoe commented Jan 5, 2018 •

edited

Loading

codecov-io commented Jan 5, 2018 •

edited

Loading

SebastianBoe commented Jan 5, 2018 •

edited

Loading

SebastianBoe commented Jan 5, 2018 •

edited

Loading

carlescufi commented Jan 5, 2018 •

edited

Loading

carlescufi commented Jan 7, 2018 •

edited

Loading

nashif commented Jan 10, 2018 •

edited

Loading