kconfig/cmake: Improve reconfiguration behavior #22043

ulfalizer · 2020-01-20T12:54:03Z

There are some issues with the behavior when rerunning CMake in an
already initialized build directory:

The check for assignments to promptless symbols in configuration
fragments isn't run when reconfiguring, because it only runs if
zephyr/.config doesn't exist
As outlined in
The contents of .config should not be able to prevent users from running menuconfig. #9573, you can
get into situations where zephyr/.config is invalid (e.g. due to
being outdated), but menuconfig/guiconfig can't be run to fix it
If kconfig.py fails while merging fragments during reconfiguration,
it will ignore the fragments during the next reconfiguration and use
the existing zephyr/.config instead, because the fragment checksum
is calculated and saved before running kconfig.py

(Footnote: The input configuration file(s) to kconfig.py can be either a
list of configuration fragments, when merging fragments, or just
zephyr/.config, if the merged configuration is up-to-date. The output
configuration file is always zephyr/.config.)

To fix the first two issues, explicitly tell kconfig.py when it's
dealing with handwritten configuration input (fragments), via a new
--handwritten-input-configs flag. This is more robust than checking
whether zephyr/.config exists, which was the old logic.

When dealing with handwritten input, there should be no assignments to
promptless symbols. Assignments to promptless symbols is expected in
zephyr/.config however, because it doubles as configuration output.

When running menuconfig/guiconfig, the input configuration is
zephyr/.config rather than configuration fragments, so this change also
makes sure that menuconfig can always be run as long as zephyr/.config
exists and is up-to-date.

To fix the last issue, only write the checksum for the configuration
fragments if kconfig.py succeeds (which means it wrote a
zephyr/.config).

Also improve naming a bit, add help texts for the command-line
parameters to kconfig.py, and simplify write_kconfig_filenames() by
moving logic into it.

Partial fix for
#9573, without the
part in #issuecomment-469701831. Can still run into issues when e.g.
when CMake files can't make sense of settings.

carlescufi · 2020-01-20T17:27:43Z

@SebastianBoe can you review this PR?

SebastianBoe · 2020-01-21T08:49:06Z

Will review soon.

SebastianBoe · 2020-01-21T14:02:12Z

This solves part of #9573, but not all of it (as I read 9573).

So we need to remove the "fixes #9573" from the description.

Steps to reproduce:

Build hello world with qemu_cortex_m3.

Run menuconfig and enable ADC.

Run menuconfig again to try to change another configuration, but see it fail because .config is corrupt.

sebo@sebo-nix:~/ncs/zephyr/samples/hello_world/outdir$ ninja menuconfig
ninja menuconfig
[0/1] Re-running CMake...
-- Zephyr version: 2.1.99
-- Selected BOARD qemu_cortex_m3
-- Found west: /home/sebo/.local/bin/west (found suitable version "0.6.99", minimum required is "0.6.0")
-- Loading /home/sebo/ncs/zephyr/boards/arm/qemu_cortex_m3/qemu_cortex_m3.dts as base
Devicetree configuration written to /home/sebo/ncs/zephyr/samples/hello_world/outdir/zephyr/include/generated/devicetree.conf
Parsing /home/sebo/ncs/zephyr/Kconfig
Loaded configuration '/home/sebo/ncs/zephyr/samples/hello_world/outdir/zephyr/.config'
No change to '/home/sebo/ncs/zephyr/samples/hello_world/outdir/zephyr/.config'
-- Cache files will be written to: /home/sebo/.cache/zephyr
-- Configuring done
CMake Error at ../../cmake/extensions.cmake:372 (add_library):
  No SOURCES given to target: drivers__adc
Call Stack (most recent call first):
  ../../cmake/extensions.cmake:349 (zephyr_library_named)
  ../../drivers/adc/CMakeLists.txt:3 (zephyr_library)


-- Build files have been written to: /home/sebo/ncs/zephyr/samples/hello_world/outdir
FAILED: build.ninja 
/home/sebo/bin/cmake/cmake-3.13.1-Linux-x86_64/bin/cmake -S/home/sebo/ncs/zephyr/samples/hello_world -B/home/sebo/ncs/zephyr/samples/hello_world/outdir
ninja: error: rebuilding 'build.ninja': subcommand failed

SebastianBoe · 2020-01-21T14:17:45Z

I'd like to test this. Could you give steps to reproduce:

kconfig.py fails while merging fragments during reconfiguration

ulfalizer · 2020-01-21T14:56:33Z

Run menuconfig again to try to change another configuration, but see it fail because .config is corrupt.

That one shouldn't be a problem on the kconfig.py end at least. Any idea what causes it?

The description for #9573 is about int/hex symbols without defaults, which is more of a Kconfig issue. That's fixed at least.

I'd like to test this. Could you give steps to reproduce:

kconfig.py fails while merging fragments during reconfiguration

cd samples/hello_world; mkdir b; cd b; cmake -GNinja -DBOARD=frdm_kw41z .. (any board will do)
Put some nonsense into samples/hello_world/prj.conf, like fsdifhsdiufh
Reconfigure with cmake -GNinja -DBOARD=frdm_kw41z ... Fails, but still saves checksum.
Reconfigure with cmake -GNinja -DBOARD=frdm_kw41z .. again. Now succeeds, because checksum is up-to-date.

The fix here is to prevent the checksum from being saved in step 3.

SebastianBoe · 2020-01-21T15:05:51Z

That one shouldn't be a problem on the kconfig.py end at least. Any idea what causes it?

When the .config is corrupt (by corrupt I mean that when CMake runs with it it does not complete, but instead crashes at some point. This could either be because the Kconfig sources are incorrect and generating invalid configurations, or because the build scripts are incorrect and making wrong assumptions about what kind of Kconfig it could be receiving) it will not be possible to run ninja menuconfig because the build fails before the new ninja build system is generated.

The description for #9573 is about int/hex symbols without defaults, which is more of a Kconfig issue. That's fixed at least.

I interpret 9573 about being about the title, and the description just being one example of how 9573 can occur. With this interpretation we can't close it. See also

#9573 (comment)

ulfalizer · 2020-01-21T16:02:18Z

Hadn't noticed the amendment. Updates the commit description now.

Rest fine?

SebastianBoe · 2020-01-22T13:01:35Z

scripts/kconfig/kconfig.py

-                            for path in paths}):
+    with open(kconfig_list_path, 'w') as out:
+        for path in sorted({os.path.realpath(os.path.join(kconf.srctree, path))
+                            for path in kconf.kconfig_filenames}):


For future reference, the review goes faster when changes are separated from refactorings in different commits.

Because then when looking at refactoring patches I can trivially verify that the changes have no semantic effects.

And when looking at actual changes I won't have to sift through the noise of unrelated refactoring.

Yeah, did some drive-by-ing and got lazy. Will split better next time.

SebastianBoe · 2020-01-22T13:04:10Z

scripts/kconfig/kconfig.py

+                             "on them, like no promptless symbols being "
+                             "assigned")
+    parser.add_argument("kconfig",
+                        help="Top-level Kconfig file")


When kconfig_root is used, the variable name is self-explanatory and no help text is needed.

I prefer this name over kconfig, which could be referring to anything.

kconfig_root to me sounds like it refers to $srctree (the root directory that source works relative to).

I could call it kconfig_file though. Should be clear enough. Looks like the C tools do:

$ scripts/kconfig/conf --help Usage: scripts/kconfig/conf [-s] [option] <kconfig-file> ...

I forgot about srctree, yes, kconfig_file will have to do.

SebastianBoe · 2020-01-22T13:06:15Z

scripts/kconfig/kconfig.py

+    parser.add_argument("kconfig",
+                        help="Top-level Kconfig file")
+    parser.add_argument("config_out",
+                        help="Output configuration file")


Configuration file is too ambiguous, let's either use the term "Kconfig fragment" or "fragment" to clarify.

Using _out and _in suffixes makes sense.

The output configuration file is a not a fragment, so probably shouldn't have "fragment" in it.

ah, right, I consider the

A=B syntax to be

of the filetype "Kconfig fragment",

but I guess technically fragment also implies that it's just a subset of options.

Then I don't know how to make this clearer, so this will have to do.

SebastianBoe · 2020-01-22T13:14:44Z

scripts/kconfig/kconfig.py

+                        help="Output file for list of parsed Kconfig files")
+    parser.add_argument("configs_in",
+                        nargs="+",
+                        help="Input configuration files")


We need to use the term fragment to be less ambiguous.

Updated the help text.

SebastianBoe · 2020-01-22T13:15:40Z

scripts/kconfig/kconfig.py

-    parser.add_argument("conf_fragments", nargs='+')
+    parser.add_argument("--handwritten-input-configs",
+                        action="store_true",
+                        help="Assume the input configuration files are "


s/configuration files/input fragments/

Updated the help text.

SebastianBoe

Minor changes requested.

The improvement itself is sane.

There are some issues with the behavior when rerunning CMake in an already initialized build directory: 1. The check for assignments to promptless symbols in configuration fragments isn't run when reconfiguring, because it only runs if zephyr/.config doesn't exist 2. As outlined in zephyrproject-rtos#9573, you can get into situations where zephyr/.config is invalid (e.g. due to being outdated), but menuconfig/guiconfig can't be run to fix it 3. If kconfig.py fails while merging fragments during reconfiguration, it will ignore the fragments during the next reconfiguration and use the existing zephyr/.config instead, because the fragment checksum is calculated and saved before running kconfig.py (Footnote: The input configuration file(s) to kconfig.py can be either a list of configuration fragments, when merging fragments, or just zephyr/.config, if the merged configuration is up-to-date. The output configuration file is always zephyr/.config.) To fix the first two issues, explicitly tell kconfig.py when it's dealing with handwritten configuration input (fragments), via a new --handwritten-input-configs flag. This is more robust than checking whether zephyr/.config exists, which was the old logic. When dealing with handwritten input, there should be no assignments to promptless symbols. Assignments to promptless symbols is expected in zephyr/.config however, because it doubles as configuration output. When running menuconfig/guiconfig, the input configuration is zephyr/.config rather than configuration fragments, so this change also makes sure that menuconfig can always be run as long as zephyr/.config exists and is up-to-date. To fix the last issue, only write the checksum for the configuration fragments if kconfig.py succeeds (which means it wrote a zephyr/.config). Also improve naming a bit, add help texts for the command-line parameters to kconfig.py, and simplify write_kconfig_filenames() by moving logic into it. Partial fix for zephyrproject-rtos#9573, without the part in #issuecomment-469701831. Can still run into issues when e.g. when CMake files can't make sense of settings. Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>

ulfalizer · 2020-01-22T14:05:28Z

Nits fixed.

ulfalizer requested review from SebastianBoe, hakonfam and cvinayak January 20, 2020 12:54

ulfalizer requested a review from nashif as a code owner January 20, 2020 12:54

ulfalizer requested a review from carlescufi January 20, 2020 12:54

ulfalizer force-pushed the reconf-improvements branch from 1a527ac to 0887f29 Compare January 20, 2020 12:55

zephyrbot added area: Build System area: Kconfig labels Jan 20, 2020

ulfalizer added the bug The issue is a bug, or the PR is fixing a bug label Jan 20, 2020

ulfalizer mentioned this pull request Jan 20, 2020

kconfig: Show unsatisfied deps. when assignments don't take #22052

Merged

ulfalizer force-pushed the reconf-improvements branch from 0887f29 to 47a9e4a Compare January 20, 2020 18:37

ulfalizer force-pushed the reconf-improvements branch from 47a9e4a to 02c1cd4 Compare January 21, 2020 15:57

ulfalizer force-pushed the reconf-improvements branch from 02c1cd4 to 3868a96 Compare January 21, 2020 17:04

SebastianBoe reviewed Jan 22, 2020

View reviewed changes

SebastianBoe requested changes Jan 22, 2020

View reviewed changes

ulfalizer force-pushed the reconf-improvements branch from 3868a96 to 1209796 Compare January 22, 2020 14:04

SebastianBoe approved these changes Jan 22, 2020

View reviewed changes

carlescufi approved these changes Jan 22, 2020

View reviewed changes

carlescufi merged commit 45050dd into zephyrproject-rtos:master Jan 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kconfig/cmake: Improve reconfiguration behavior #22043

kconfig/cmake: Improve reconfiguration behavior #22043

ulfalizer commented Jan 20, 2020 •

edited

Loading

carlescufi commented Jan 20, 2020

SebastianBoe commented Jan 21, 2020

SebastianBoe commented Jan 21, 2020 •

edited

Loading

SebastianBoe commented Jan 21, 2020 •

edited

Loading

ulfalizer commented Jan 21, 2020

SebastianBoe commented Jan 21, 2020

ulfalizer commented Jan 21, 2020

SebastianBoe Jan 22, 2020 •

edited

Loading

SebastianBoe Jan 22, 2020

ulfalizer Jan 22, 2020

SebastianBoe Jan 22, 2020

ulfalizer Jan 22, 2020

SebastianBoe Jan 22, 2020

ulfalizer Jan 22, 2020

SebastianBoe Jan 22, 2020

ulfalizer Jan 22, 2020

SebastianBoe Jan 22, 2020

SebastianBoe Jan 22, 2020

ulfalizer Jan 22, 2020

SebastianBoe Jan 22, 2020

ulfalizer Jan 22, 2020

SebastianBoe left a comment

ulfalizer commented Jan 22, 2020

kconfig/cmake: Improve reconfiguration behavior #22043

kconfig/cmake: Improve reconfiguration behavior #22043

Conversation

ulfalizer commented Jan 20, 2020 • edited Loading

carlescufi commented Jan 20, 2020

SebastianBoe commented Jan 21, 2020

SebastianBoe commented Jan 21, 2020 • edited Loading

SebastianBoe commented Jan 21, 2020 • edited Loading

ulfalizer commented Jan 21, 2020

SebastianBoe commented Jan 21, 2020

ulfalizer commented Jan 21, 2020

SebastianBoe Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe left a comment

Choose a reason for hiding this comment

ulfalizer commented Jan 22, 2020

ulfalizer commented Jan 20, 2020 •

edited

Loading

SebastianBoe commented Jan 21, 2020 •

edited

Loading

SebastianBoe commented Jan 21, 2020 •

edited

Loading

SebastianBoe Jan 22, 2020 •

edited

Loading