Fix python variant of merge_config #5417

nashif · 2017-12-17T14:15:16Z

Do not use comment indicating variable is not set, this confuses the merge
script.

Fixes #5374

carlescufi · 2017-12-17T14:42:27Z

Should we fix the parsing of these "not set" lines for the future, in case someone mistakenly adds one?

nashif · 2017-12-17T21:17:00Z

Should we fix the parsing of these "not set" lines for the future, in case someone mistakenly adds one?

I am still not sure if this was actually valid but we can do that, it will require rewriting the python script though.

mbolivar · 2017-12-17T22:30:16Z

I think the "is not set" syntax is valid kconfig fragment syntax, even though it looks like a comment. Are you sure about this?

nashif · 2017-12-18T04:47:48Z

I think the "is not set" syntax is valid kconfig fragment syntax, even though it looks like a comment. Are you sure about this?

it is valid for kconfig in general, was not sure if it is supported as fragment. We have been using direct assignment instead with the exception of the few files changed in the PR.
We do not need to support this I guess, this will require doing it in a fashion similar to how it is done in the shell script thus rewriting the python script.

SebastianBoe · 2017-12-18T08:21:22Z

I believe we should support this "as fragment", because otherwise it wouldn't be valid
for a user to do

cp build/zephyr/.config prj.conf

which is the recommended way to save a GUI-edited .conf file isn't it?

dbkinder · 2017-12-18T14:30:43Z

In the title, veriant should be variant.

carlescufi · 2017-12-18T14:39:06Z

Is “is not set” used only for variables that default to “n”, and never for ones that have been explicitly ser to “n” but default to “y”? Maybe we could simply ignore lines that begin with “#”?

nashif · 2017-12-18T15:18:27Z

I now have a version that mimics the original shell script and does exactly the same, will upload after more testing

nashif · 2017-12-18T20:01:05Z

updated, still WIP, just uploaded to get through sanitycheck

nashif · 2017-12-19T12:49:16Z

ok, I think i have it now, please review

SebastianBoe · 2017-12-19T13:29:16Z

scripts/kconfig/merge_config.py

+    else:
+        with open('%s/.config' % args.output_dir, 'w') as file:
+            file.write(initfile_content)
+        with open('%s/.config.test' % args.output_dir, 'w') as file:


Is this debug code that was included by mistake?

Changed the python variant to support fragments with comments indicating an option is not being set. Signed-off-by: Anas Nashif <[email protected]>

SebastianBoe · 2017-12-19T15:56:09Z

There is something off with the redundancy-detection logic. I have a bunch of fixes/refactorings on top of your work so I'll do a PR to your branch that fixes the fact that the redundancy detector uses substring to detect redundancies.

prev=list(filter(lambda x: cfg in x, initfile_content.split("\n")))

sebo@mach:~/zephyr/samples/net/irc_bot$ /usr/bin/python3 /home/sebo/zephyr/scripts/kconfig/merge_config.py -m -O /home/sebo/zephyr/samples/net/irc_bot/b/zephyr /home/sebo/zephyr/boards/riscv32/zedboard_pulpino/zedboard_pulpino_defconfig /home/sebo/zephyr/samples/net/irc_bot/prj.conf
-- Using /home/sebo/zephyr/boards/riscv32/zedboard_pulpino/zedboard_pulpino_defconfig as base
-- Merging /home/sebo/zephyr/samples/net/irc_bot/prj.conf
-- Value of CONFIG_RISCV is redefined by fragment /home/sebo/zephyr/samples/net/irc_bot/prj.conf
-- Previous value: CONFIG_RISCV32=y
-- New value: CONFIG_RISCV=y
sebo@mach:~/zephyr/samples/net/irc_bot$ git diff
WARNING: terminal is not fully functional
-  (press RETURN)
diff --git a/samples/net/irc_bot/prj.conf b/samples/net/irc_bot/prj.conf
index bc88318..0e73d90 100644
--- a/samples/net/irc_bot/prj.conf
+++ b/samples/net/irc_bot/prj.conf
@@ -1,5 +1,7 @@
 CONFIG_INIT_STACKS=y
 
+CONFIG_RISCV=y
+
 CONFIG_NET_LOG_ENABLED=y
 CONFIG_SYS_LOG_NET_LEVEL=4
 CONFIG_NET_IPV4=y
sebo@mach:~/zephyr/samples/net/irc_bot$ git log -1
WARNING: terminal is not fully functional
-  (press RETURN)
commit 7cbac50536880a15beb530f75242a3739ae9b978
Author: Anas Nashif <[email protected]>
Date:   Sun Dec 17 08:23:11 2017 -0500

    kconfig: use merge_config.py for merging
    
    Changed the python variant to support fragments with comments indicating
    an option is not being set.
    
    Signed-off-by: Anas Nashif <[email protected]>

nashif · 2017-12-19T21:59:02Z

prev=list(filter(lambda x: cfg in x, initfile_content.split("\n")))

maybe

prev=list(filter(lambda x: cfg in x.split(), initfile_content.split("\n")))

Rewrite of merge_config.py, several unrelated changes including; Do not attribute meaning to lines that look like this: Only to lines that look like this: Removed the unimplemented 'runmake' and 'alltarget' arguments. Invoke the python interpreter instead of the script directly to avoid reliance on bash's shebang feature. Use exact line match instead of substring to detect redundant config's. And more ... Signed-off-by: Sebastian Bøe <[email protected]>

SebastianBoe · 2017-12-20T12:44:48Z

@nashif : Various fixes based on your work can be found here: nashif#2

rewrite merge_config

mbolivar · 2017-12-20T20:53:02Z

@nashif : Various fixes based on your work can be found here: nashif#2

I've got a few changes I'd like to propose to the combined efforts so far; putting my review together now

lpereira · 2017-12-20T22:42:17Z

As a nitpick: it's often better to use a list comprehension instead of filter(), specially if the result is going to be converted to a list afterwards: prev=list(filter(lambda x: cfg in x.split(), initfile_content.split("\n"))) can be written as prev = [line for line in initfile_content.split("\n") if cfg in line.split()].

mbolivar

I think it's almost there, but still has some issues. Here is an update which hopefully fixes the issues I've pointed out in this review:

https://github.com/mbolivar/zephyr/tree/merge_config-py

mbolivar · 2017-12-20T23:44:29Z

scripts/kconfig/merge_config.py

+    # '# CONFIG_FOO is not set' should be treated by merge_config as a
+    # state like any other
+
+    is_not_set_pattern = re.compile('^# (CONFIG_[a-zA-Z0-9_]*) is not set.*')


I like this change a lot.

mbolivar · 2017-12-20T23:51:56Z

scripts/kconfig/merge_config.py

+    # In other words [(config_name, config_line), ... ]
+    #
+    # Note that "# CONFIG_ARM is not set" is not the same as a
+    # comment, it has meaning (for now).


I really think the confusing "is not set" syntax is the "correct" syntax within kconfig.

Some data on "# CONFIG_FOO is not set" versus "CONFIG_FOO=n":

465 defconfig files in Linux v4.14 use "# CONFIG_FOO is not set" syntax. defconfig files are not full .config files, but they do determine the default settings, so they are fragments that need to be merged properly.

In contrast, only 1 defconfig (for the unicore arch, so not exactly common) and only 38 files total in 4.14 use CONFIG_FOO=n, and almost all of those are in the rcutorture selftests. So that seems to be the "wrong" syntax for config fragments, since it isn't really used anywhere, even though people say CONFIG_FOO=n informally.

All of the following also seem to say that "is not set" is valid:

https://github.com/torvalds/linux/blob/master/scripts/diffconfig#L39 -

https://github.com/torvalds/linux/blob/master/scripts/config#L166 -

https://stackoverflow.com/questions/41885015/what-exactly-does-linux-kernels-make-defconfig-do

So I think that the Zephyr habit of writing CONFIG_FOO=n directly in the .conf fragments should probably be deprecated.

Don't get me wrong; I think this is an extremely annoying and confusing "feature" of Kconfig. It's even worse that CONFIG_FOO=n usage happens sometimes, even in upstream Linux. But I think it's important to preserve behavior in the Python re-implementations as long as Zephyr still uses Kconfig.

I disagree. IMHO "# is not set" should eventually be deprecated in favour of "=n" for obvious reasons.

Zephyr usability trumps Linux kernel Kconfig compatibility.

EDIT: #5443

I disagree. IMHO "# is not set" should eventually be deprecated in favour of "=n" for obvious reasons.

I can see why you say so.

There are many more people who are familiar with the Linux style than the Zephyr style, though, and I think it's "obvious" that we want to obey principle of least surprise, especially since Zephyr is a Linux Foundation project.

@nashif, what's your decision?

If I may weigh in, I think that both formats should be supported and should do exactly the same:

CONFIG_BT=n # CONFIG_BT is not set

Should both turn Bluetooth OFF if present in the .conf or defconfig files

Maybe I've missed something, but does conf treat those 2 differently in the .config file?

To get this merged, I suggest we retain KBuild compatibilty for now, and then we can discuss the long-term Kconfig syntax at a later point.

mbolivar · 2017-12-20T23:56:02Z

scripts/kconfig/merge_config.py

+            print("-- Merging {}".format(fragment_path))
+
+            for config_name, line in fragment_conf:
+                for (i, (prev_config_name, prev_line)) in enumerate(list(conf)):


In the worst case, if there are m config fragments with n CONFIG_ lines each, this runtime is O(m * n^2). It could be O(m * n) by keeping a hash of the configs you've seen before and handling the "seen it before" test with a membership test. This can be done by using an OrderedDict from CONFIG_ to line instead of list of (CONFIG_, line) tuples.

mbolivar · 2017-12-20T23:59:14Z

scripts/kconfig/merge_config.py

+    # state like any other
+
+    is_not_set_pattern = re.compile('^# (CONFIG_[a-zA-Z0-9_]*) is not set.*')
+    pattern            = re.compile('^(CONFIG_[a-zA-Z0-9_]*)[= ].*')


This has issues:

It matches strings like "CONFIG_FOO did not actually receive a value".

If the base file sets "# CONFIG_FOO is not set", and some other fragment sets "CONFIG_FOO=n", that gets treated like an override, even though it's redundant (and CONFIG_FOO=n should be changed to "is not set").

I think two changes are needed:

this function should probably patch things up for now by converting "=n" to "is not set", then the .conf fragments should be changed to "is not set" syntax. The "patch-up" can then be removed if necessary.

if the line doesn't match "CONFIG_FOO=n" exactly, the pattern should be '^(CONFIG_[a-zA-Z0-9_]*)\s*=.*' instead

"It matches strings like "CONFIG_FOO did not actually receive a value"

That is a minor problem because that is invalid syntax, but it would be great for
usability if you were able to fix this!

If the base file sets "# CONFIG_FOO is not set", and some other fragment sets "CONFIG_FOO=n", that gets treated like an override, even though it's redundant (and CONFIG_FOO=n should be changed to "is not set").

I don't see how this affects the user, but if you can find a clean fix then why not :)

That is a minor problem because that is invalid syntax, but it would be great for
usability if you were able to fix this!

Glad you agree it's OK to fix. It's fixed in my patch.

I don't see how this affects the user, but if you can find a clean fix then why not :)

This script prints warnings on overrides. Warnings affect users.

mbolivar · 2017-12-21T00:08:09Z

scripts/kconfig/merge_config.py

+                        is_redefine  = line != prev_line
+                        if not args.quiet:
+                            if is_redefine:
+                                print("-- Value of {} is redefined by fragment {}".format(config_name, fragment_path))


I think it'd be an improvement to print these to stderr

ulfalizer · 2017-12-29T16:46:05Z

@carlescufi
That matches my reading of merge_config.sh from the kernel tree. After all the config fragment files have been merged into $TMP_FILE, it runs the following command to fill out the generated .config with all the symbol values that haven't been specified. Those could come from defaults specified in a Kconfig file, for example.

make KCONFIG_ALLCONFIG=$TMP_FILE $OUTPUT_ARG $ALLTARGET

$ALLTARGET will be either alldefconfig or allnoconfig, and those require parsing the Kconfig files. Checking dependencies to make sure that the generated .config is well-formed also requires looking at them.

It's very possible that I've misunderstood some nuance of how it is supposed to work, so don't be afraid to correct me if I have. :)

carlescufi · 2017-12-29T16:53:15Z

@ulfalizer The thing is that Zephyr uses it slightly differently than the kernel. Right now we invoke merge_config.sh like so:

merge_config.sh -m -q -O <dest path for .config> fragment1 fragment2 ...

Note the -m which disables the invocation of make. And then we run conf.exe separately passing the Kconfig tree path and the .config as parameters.

So if I understand you correctly your script does both the merge_config.sh and the actual conf run in one go, whereas we do it in 2 steps today. Our .config after running merge_config.sh is just a merge of all the fragments with a very small subset of default values, whereas with yours it would actually be the final .config used for building if I understand correctly.

EDIT: What I mean is that if I add this in Kconfig:

config FOZ
        int
        default 123

then the resulting merged contains CONFIG_FOZ=123, whereas in our run of merge_config.sh CONFIG_FOZ would not be present at all in merged.

ulfalizer · 2017-12-29T17:03:33Z

@carlescufi
Yep, right now it's generating the complete .config right away. That's probably the simplest approach if you can get away with it.

I actually did an earlier version that functions more like the Zephyr version before I looked closer at merge_config.sh though. Replacing the kconf.write_config(sys.argv[2]) will the following gives that version:

with open(sys.argv[2], "w") as merged_config:
    node = kconf.top_node

    while True:
        if isinstance(node.item, Symbol) and node.item.user_value is not None:
            if node.item.visibility:
                merged_config.write(node.item.config_string)
            else:
                print("warning: an assignment to {0} appeared in one of "
                      "the config fragments but couldn't be respected in "
                      "the merged configuration, due to {0} not being "
                      "visible".format(node.item.name))

        # Iterative tree walk using parent pointers

        if node.list:
            node = node.list
        elif node.next:
            node = node.next
        else:
            while node.parent:
                node = node.parent
                if node.next:
                    node = node.next
                    break
            else:
                break

The iterative tree walk could be replaced with something simpler to read like in examples/print_tree.py. It was just handy to copy it out of the write_config() implementation.

That spammy warning could be replaced with something more informative like in the first version too.

carlescufi · 2017-12-29T17:08:14Z

@ulfalizer thanks very much for all the suggestions.
@mbolivar @nashif @SebastianBoe: Actually and given all the input here, I would be in favor of using the merge_config.py that @ulfalizer has posted here and call it something like kconfig.py instead, since it does everything in one go.

ulfalizer · 2017-12-29T17:08:14Z

That version still looks at the Kconfig tree though. Kconfiglib unfortunately isn't well-suited to processing .config files in isolation.

carlescufi · 2017-12-29T17:09:23Z

@ulfalizer yes, and that's why I think the better solution is to consolidate the Kconfig parsing and the fragment merging in your first version instead of keeping it separate.

ulfalizer · 2017-12-29T17:10:54Z

@carlescufi Yeah, I think that might lead to less head-scratching from tricky dependency stuff in the end too.

carlescufi · 2017-12-29T17:11:38Z

@ulfalizer since both you and @nashif are here, I think there was a change that we required to Kconfiglib to be able to use it properly, which is "glob" support as shown here: https://github.com/zephyrproject-rtos/zephyr/blob/master/boards/Kconfig#L19. Is this something we could consider sending a PR for?

ulfalizer · 2017-12-29T17:13:13Z

@carlescufi
I'd be happy to merge it. Don't think it should interfere with vanilla Kconfig parsing.

A loop in the _T_SOURCE handling ought to do it.

carlescufi · 2017-12-29T17:16:20Z

@ulfalizer well thanks very much for the offer! I will let @nashif confirm since he's the one that has been looking into this switch from C to Python in detail.

nashif · 2017-12-29T17:18:22Z

A loop in the _T_SOURCE handling ought to do it.

already have a patch for this:

commit 35c4ef1f65d37c4e0cd0c6fea9fa9138a9418eb0
Author: Anas Nashif <[email protected]>
Date:   Tue Dec 12 10:46:23 2017 -0500

    zephyr: support globbing

    Signed-off-by: Anas Nashif <[email protected]>

diff --git a/kconfiglib.py b/kconfiglib.py
index 4d763a3..254c8b4 100644
--- a/kconfiglib.py
+++ b/kconfiglib.py
@@ -368,6 +368,7 @@ import os
 import platform
 import re
 import sys
+import glob

 # File layout:
 #
@@ -1453,12 +1454,15 @@ class Kconfig(object):
                 prev_node.next = prev_node = node

             elif t0 == _T_SOURCE:
-                self._enter_file(self._expand_syms(self._next_token()))
-                prev_node = self._parse_block(None,            # end_token
-                                              parent,
-                                              visible_if_deps,
-                                              prev_node)
-                self._leave_file()
+                f = self._expand_syms(self._next_token())
+                g = glob.glob(f)
+                for s in g:
+                    self._enter_file(s)
+                    prev_node = self._parse_block(None,            # end_token
+                                                  parent,
+                                                  visible_if_deps,
+                                                  prev_node)
+                    self._leave_file()

             elif t0 == end_token:
                 # We have reached the end of the block. Terminate the final

ulfalizer · 2017-12-29T17:26:07Z

@nashif
Yeah, that's the reasonable approach. One thing that gets a bit icky is that globbing also ought to be done relative to $srctree if set, so that out-of-tree configuration works. Do you ever do those?

Small nitpick: The imports are sorted alphabetically.

If you submit a PR, we could pick up there.

ulfalizer · 2017-12-29T17:32:01Z

Unless there's a huge number of Kconfig files or some dynamic stuff going on with the tree, it seems simpler to just list them all explicitly though. Easier to selectively disable individual Kconfig files for debugging then too.

nashif · 2017-12-29T17:33:06Z

Yeah, that's the reasonable approach. One thing that gets a bit icky is that globbing also ought to be done relative to $srctree if set, so that out-of-tree configuration works. Do you ever do those?

Small nitpick: The imports are sorted alphabetically.

If you submit a PR, we could pick up there.

will do after some more testing, thanks.

ulfalizer · 2017-12-29T18:11:33Z

@nashif
If you need some tests, something like this in testsuite.py should do, if Kglobbed1-3 are globbed up in various ways at the end of tests/Klocation (so that nothing else moves and breaks the existing tests) and each contain a single symbol.

+    # Test 'source' globbing
+
+    verify_locations(c.syms["GLOBBED1"].nodes, "tests/Kglobbed1:1")
+    verify_locations(c.syms["GLOBBED2"].nodes, "tests/Kglobbed2:1")
+    verify_locations(c.syms["GLOBBED3"].nodes, "tests/sub/Kglobbed3:1")
+
 
     print("Testing visibility")

Functions similarly to scripts/kconfig/merge_config.sh from the kernel. Came up in zephyrproject-rtos/zephyr#5417.

carlescufi · 2018-01-03T10:47:17Z

@ulfalizer I've posted a PR with globbing support including $srctree in your GitHub repo. However when running your proposed script here, and although the configuration is parsed correctly, the .defconfig and .conf files seem not to apply, and the architecture is set to the default x86 even though I'm providing a defconfig that sets it to ARM. Any ideas?
More specifically I am providing this and this files to your script while at the same time pointing to this Kconfig

EDIT: Scratch that, I am dumb, during my tests I had overwritten the defconfig :| I blame the flu

carlescufi · 2018-01-03T10:57:33Z

@ulfalizer how hard would it be to write a kconfigdiff with your library? something that is able to load 2 .config files and compare them regardless of the order the variables appear? This would be useful actually to make sure the transition from conf to Kconfiglib has not introduced changes.

codecov-io · 2018-01-03T11:18:31Z

Codecov Report

❗ No coverage uploaded for pull request base (master@16fbf25). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #5417   +/-   ##
=========================================
  Coverage          ?   53.16%           
=========================================
  Files             ?      425           
  Lines             ?    35175           
  Branches          ?        0           
=========================================
  Hits              ?    18702           
  Misses            ?    16473           
  Partials          ?        0

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 16fbf25...f7c10fd. Read the comment docs.

carlescufi · 2018-01-03T15:58:23Z

@ulfalizer So after playing around a bit more with your script with globbing added, I see a few differences between the .config generated by conf and the one generated by Kconfiglib. Those revolve mainly around items that default to a value with an if statement such as:
https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/bluetooth/host/Kconfig#L29

In this case, and for my particular application, BT_RX_BUF_COUNT should default to 3 because BT_RECV_IS_RX_THREAD is enabled, but in the .config generated by Kconfiglib I get 10 instead.

Another example is here:
https://github.com/zephyrproject-rtos/zephyr/blob/master/kernel/Kconfig#L119

Since ARM and CPU_HAS_FPU are both =y in my .config, the idle thread stack size should be set to 320, but instead I find it at 256.

Do you know why that would be the case?

ulfalizer · 2018-01-03T20:42:37Z

@carlescufi
Do you think your conf utility might be out of sync with the kernel's?

Normally, conf walks through the defaults in order, picking the first one with a satisfied condition. A missing condition, like for default 10, is always satisfied, so any default after it shouldn't even be considered. The defaults would need to be swapped.

Here's the code from the C implementation (in scripts/kconfig/symbol.c), where visible is the condition:

static struct property *sym_get_default_prop(struct symbol *sym)
{
	struct property *prop;

	for_all_defaults(sym, prop) {
		prop->visible.tri = expr_calc_value(prop->visible.expr);
		if (prop->visible.tri != no)
			return prop;
	}
	return NULL;
}

Here's the corresponding Kconfiglib code (which also includes some logic from sym_calc_value() in the C implementation):

for default, cond in self.defaults:
    cond_val = expr_value(cond)
    if cond_val:
        val = min(expr_value(default), cond_val)
        self._write_to_conf = True
        break

I suspect some things would fail in Kconfiglib's compatibility test suite too if the behavior didn't match here.

Here's two test files you could use (with e.g. $ scripts/kconfig/conf default) to see this behavior:

default

config FOO
	bool "foo"

config INT
	int "int"
	default 10
	default 12 if FOO

.config

CONFIG_FOO=y

ulfalizer · 2018-01-03T20:49:49Z

@carlescufi
One way to compare two .config files would be to create two Kconfig instances, load the two .config files into them, and then go through all symbols in both of them and compare the ones that have a Symbol.user_value that is not None (a similar thing is going on in the merge_config.py example too, so it might be good reference).

The Symbol.user_value field holds the value that was assigned to the symbol in the .config file. This value might be different from Symbol.str_/tri_value, because Kconfig always respects dependencies to avoid creating invalid configurations.

carlescufi · 2018-01-03T20:51:42Z

@ulfalizer

Do you think your conf utility might be out of sync with the kernel's?

Yes, that is possible, although I am surprised about such a fundamental difference

Normally, conf walks through the defaults in order, picking the first one with a satisfied condition

So what you are saying is that in Linux's Kconfig files, the default with no condition comes always as the last one after all the ones with conditions? That's definitely not the case with Zephyr.

Here's two test files you could use (with e.g. $ scripts/kconfig/conf default) to see this behavior:

I assume here Linux's conf and Kconfiglib would default INT to 10, but you expect Zephyr's conf to default it to 12, is that correct?

ulfalizer · 2018-01-03T21:01:55Z

@carlescufi
Yep. INT defaults to 10 with conf/mconf with the Linux Kconfig tools.

Someone changing the behavior there seems a bit arbitrary, but it's the only explanation I can think of. How heavily modified (and old) is the Zephyr conf?

Here's the kind of stuff you see in the Linux Kconfig files:

config PGTABLE_LEVELS
	int
	default 5 if X86_5LEVEL
	default 4 if X86_64
	default 3 if X86_PAE
	default 2

mbolivar · 2018-01-04T21:44:47Z

Happy new year all :).

@mbolivar @nashif @SebastianBoe: Actually and given all the input here, I would be in favor of using the merge_config.py that @ulfalizer has posted here and call it something like kconfig.py instead, since it does everything in one go.

+1 to not reinventing the wheel.

carlescufi · 2018-01-04T21:49:38Z

@mbolivar I have Zephyr building with Kconfiglib, but unfortunately not all the changes can go upstream to @ulfalizer's repo due to differences between Linux's Kconfig and ours. autoconf generation can though, and it's currently in PR form: ulfalizer/Kconfiglib#35

mbolivar · 2018-01-04T22:15:05Z

@mbolivar I have Zephyr building with Kconfiglib, but unfortunately not all the changes can go upstream to @ulfalizer's repo due to differences between Linux's Kconfig and ours.

@carlescufi saw the discussion about default ordering -- that's what you're referring to, right? Are you saying that we should fix/keep our own merge_config.py and incompatible default order in our Kconfigs?

I assumed the way forward was to fix the Zephyr Kconfigs so they are compatible with Linux, so we can use kconfiglib and the "standard" conf. Sorry if I misunderstood.

Happy to pick up my changes to merge_config.py if we're going to keep our incompatibilities for the time being.

carlescufi · 2018-01-05T10:07:15Z

@mbolivar happy new year!

@carlescufi saw the discussion about default ordering -- that's what you're referring to, right? Are you
saying that we should fix/keep our own merge_config.py and incompatible default order in our
Kconfigs?

There are 2 different items that make our Kconfig files incompatible with vanilla Linux Kconfig:

Globbing: We require support for globbing when sourcing Kconfig files
Prefer later defaults: Zephyr "prefers" the later defaults instead of matching the first one

I plan to do this in 2 steps: First leave the Kconfig files unchanged but use a modified version of Kconfiglib to reproduce our current behavior. This is to ensure that Kconfiglib indeed respects everything else about Kconfig and to avoid changes in Kconfig files at the same time that we replace conf. Then in a second step I suggest we remove the globbing (it's silly anyway IMHO) and that we go back to standard default matching.

Happy to pick up my changes to merge_config.py if we're going to keep our incompatibilities for the
time being.

There's no more need for a merge_config.py. As soon as I post my PR you'll see that there's a single kconfig.py that does both merge_config and conf in one go, as well as writing the autoconf.h.

carlescufi · 2018-01-05T15:38:16Z

I think we should close this PR now that #5569 is posted.

nashif requested review from mike-scott and SebastianBoe as code owners December 17, 2017 14:15

nashif added this to the v1.11.0 milestone Dec 17, 2017

nashif changed the title ~~Fix python veriant of merge_config~~ Fix python variant of merge_config Dec 18, 2017

nashif force-pushed the merge_Config_fix branch from 8d525ab to eba4958 Compare December 18, 2017 20:00

nashif force-pushed the merge_Config_fix branch 2 times, most recently from ffc38af to 0d02c09 Compare December 19, 2017 03:27

SebastianBoe requested changes Dec 19, 2017

View reviewed changes

kconfig: use merge_config.py for merging

7cbac50

Changed the python variant to support fragments with comments indicating an option is not being set. Signed-off-by: Anas Nashif <[email protected]>

nashif force-pushed the merge_Config_fix branch from 0d02c09 to 7cbac50 Compare December 19, 2017 13:39

Merge pull request #2 from SebastianBoe/merge_config_rework

f7c10fd

rewrite merge_config

lpereira closed this Dec 20, 2017

lpereira reopened this Dec 20, 2017

mbolivar requested changes Dec 21, 2017

View reviewed changes

ulfalizer added a commit to ulfalizer/Kconfiglib that referenced this pull request Dec 30, 2017

Add merge_config.py example

efcd2bd

Functions similarly to scripts/kconfig/merge_config.sh from the kernel. Came up in zephyrproject-rtos/zephyr#5417.

nashif closed this Jan 5, 2018

Fix python variant of merge_config #5417

Fix python variant of merge_config #5417

Conversation

nashif commented Dec 17, 2017

carlescufi commented Dec 17, 2017

nashif commented Dec 17, 2017

mbolivar commented Dec 17, 2017

nashif commented Dec 18, 2017

SebastianBoe commented Dec 18, 2017

dbkinder commented Dec 18, 2017

carlescufi commented Dec 18, 2017

nashif commented Dec 18, 2017

nashif commented Dec 18, 2017

nashif commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe commented Dec 19, 2017

nashif commented Dec 19, 2017

SebastianBoe commented Dec 20, 2017 • edited Loading

mbolivar commented Dec 20, 2017

lpereira commented Dec 20, 2017

mbolivar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe Dec 21, 2017 • 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

ulfalizer commented Dec 29, 2017

carlescufi commented Dec 29, 2017 • edited Loading

ulfalizer commented Dec 29, 2017

carlescufi commented Dec 29, 2017

ulfalizer commented Dec 29, 2017

carlescufi commented Dec 29, 2017

ulfalizer commented Dec 29, 2017

carlescufi commented Dec 29, 2017

ulfalizer commented Dec 29, 2017 • edited Loading

carlescufi commented Dec 29, 2017

nashif commented Dec 29, 2017

ulfalizer commented Dec 29, 2017 • edited Loading

ulfalizer commented Dec 29, 2017

nashif commented Dec 29, 2017

ulfalizer commented Dec 29, 2017 • edited Loading

carlescufi commented Jan 3, 2018 • edited Loading

carlescufi commented Jan 3, 2018

codecov-io commented Jan 3, 2018 • edited Loading

Codecov Report

carlescufi commented Jan 3, 2018 • edited Loading

ulfalizer commented Jan 3, 2018

ulfalizer commented Jan 3, 2018

carlescufi commented Jan 3, 2018

ulfalizer commented Jan 3, 2018

mbolivar commented Jan 4, 2018

carlescufi commented Jan 4, 2018

mbolivar commented Jan 4, 2018

carlescufi commented Jan 5, 2018

carlescufi commented Jan 5, 2018

SebastianBoe commented Dec 20, 2017 •

edited

Loading

SebastianBoe Dec 21, 2017 •

edited

Loading

carlescufi commented Dec 29, 2017 •

edited

Loading

ulfalizer commented Dec 29, 2017 •

edited

Loading

ulfalizer commented Dec 29, 2017 •

edited

Loading

ulfalizer commented Dec 29, 2017 •

edited

Loading

carlescufi commented Jan 3, 2018 •

edited

Loading

codecov-io commented Jan 3, 2018 •

edited

Loading

carlescufi commented Jan 3, 2018 •

edited

Loading