Skip to content
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.

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

Glob #40

Closed
wants to merge 2 commits into from
Closed

Glob #40

wants to merge 2 commits into from

Conversation

SebastianBoe
Copy link
Contributor

Hi @ulfalizer , before I start testing and cleaning up this patch, I would like to request comments on whether the glob extension could become an opt-in feature in Kconfiglib.

The motivation from our side is to avoid forking.

carlescufi and others added 2 commits March 9, 2018 12:10
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.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>
@SebastianBoe
Copy link
Contributor Author

If this could be supported upstream, then we just need to figure out how it is enabled.

E.g.;

a new source statement, like rsource, (gsource, globbed_source?)

or a config constructor argument like 'warn' (like the current patch).

NB: rsource has not been patched to support this, but I assume that would be desirable if this is accepted.

@ulfalizer
Copy link
Owner

Hello,

Would you still be fine with a separate keyword (e.g. gsource, to keep it consistent with rsource)?

One handy thing about gsource is that it would double as an include-if-exists function when given a plain filename. source and gsource could then be used like include and -include in make. That's one reason that I'd prefer to have both available. The other reason is that I'm afraid that adding flags for different "flavors" of Kconfig will lead to complexity down the road.

Having rgsource would make sense too (a relative globbing source), but I think gsource would be enough for now.

If you're fine with that approach, I'll implement it for you. I want to keep the "common" paths like source as readable and straightforward as possible, so I prefer a more separated-out approach.

@SebastianBoe
Copy link
Contributor Author

I'm pretty sure I can sell using gsource to the Zephyr project. It makes sense. Globbed sourcing has different semantics from source, so it should have a different keyword as well.

I'll post an RFC this week.

@ulfalizer
Copy link
Owner

ulfalizer commented Mar 12, 2018

Do you think this version would be fine?

Currently, you always get filenames without the $srctree prefix in e.g. MenuNode.filename. It can be argued whether that's a feature or not, but this keeps it consistent at least.

diff --git a/kconfiglib.py b/kconfiglib.py
index 85a0054..ec26517 100644
--- a/kconfiglib.py
+++ b/kconfiglib.py
@@ -354,6 +354,7 @@ Send bug reports, suggestions, and questions to ulfalizer a.t Google's email
 service, or open a ticket on the GitHub page.
 """
 import errno
+import glob
 import os
 import platform
 import re
@@ -1839,6 +1840,34 @@ class Kconfig(object):
                                               prev_node)
                 self._leave_file()
 
+            elif t0 == _T_GSOURCE:
+                # Temporarily cd into $srctree if it is set before globbing.
+                # This setup makes the globbed filenames appear without a
+                # $srctree prefix, which is consistent with how 'source' and
+                # 'rsource' work. The normal $srctree logic will find the files
+                # later.
+                prev_dir = os.getcwd()
+                try:
+                    if self.srctree is not None:
+                        os.chdir(self.srctree)
+
+                    # Sort the glob results to ensure a consistent ordering of
+                    # Kconfig symbols, which indirectly ensures a consistent
+                    # ordering in e.g. .config files
+                    filenames = sorted(glob.glob(
+                        self._expand_syms(self._expect_str_and_eol())))
+
+                finally:
+                    os.chdir(prev_dir)
+
+                for filename in filenames:
+                    self._enter_file(filename)
+                    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
                 # node and return it.
@@ -4699,6 +4728,7 @@ STR_TO_TRI = {
     _T_EQUAL,
     _T_GREATER,
     _T_GREATER_EQUAL,
+    _T_GSOURCE,
     _T_HELP,
     _T_HEX,
     _T_IF,
@@ -4725,7 +4755,7 @@ STR_TO_TRI = {
     _T_TRISTATE,
     _T_UNEQUAL,
     _T_VISIBLE,
-) = range(45)
+) = range(46)
 
 # Public integers representing expression types
 #
@@ -4759,6 +4789,7 @@ _get_keyword = {
     "endif":          _T_ENDIF,
     "endmenu":        _T_ENDMENU,
     "env":            _T_ENV,
+    "gsource":        _T_GSOURCE,
     "help":           _T_HELP,
     "hex":            _T_HEX,
     "if":             _T_IF,
@@ -4792,6 +4823,7 @@ _STRING_LEX = frozenset((
     _T_BOOL,
     _T_CHOICE,
     _T_COMMENT,
+    _T_GSOURCE,
     _T_HEX,
     _T_INT,
     _T_MAINMENU,

@ulfalizer
Copy link
Owner

Guess some people might wince at the chdir() shenanigans. Might not get so bad with os.path.relpath() either...

@ulfalizer
Copy link
Owner

Here's a Mark II without the chdir(). Seems a bit neater.

diff --git a/kconfiglib.py b/kconfiglib.py
index 85a0054..24220d5 100644
--- a/kconfiglib.py
+++ b/kconfiglib.py
@@ -354,6 +354,7 @@ Send bug reports, suggestions, and questions to ulfalizer a.t Google's email
 service, or open a ticket on the GitHub page.
 """
 import errno
+import glob
 import os
 import platform
 import re
@@ -1839,6 +1840,33 @@ class Kconfig(object):
                                               prev_node)
                 self._leave_file()
 
+            elif t0 == _T_GSOURCE:
+                # If $srctree is set, glob relative to it
+                pattern = self._expand_syms(self._expect_str_and_eol())
+                if self.srctree is not None:
+                    pattern = os.path.join(self.srctree, pattern)
+
+                # Sort the glob results to ensure a consistent ordering of
+                # Kconfig symbols, which indirectly ensures a consistent
+                # ordering in e.g. .config files
+                for filename in sorted(glob.glob(pattern)):
+                    if self.srctree is not None:
+                        # Strip the $srctree prefix from the filename and let
+                        # the normal $srctree logic find the file. This makes
+                        # the globbed filenames appear without a $srctree
+                        # prefix in MenuNode.filename, which is consistent with
+                        # how 'source' and 'rsource' work. We get the same
+                        # behavior as if the files had been 'source'd one by
+                        # one.
+                        filename = os.path.relpath(filename, self.srctree)
+
+                    self._enter_file(filename)
+                    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
                 # node and return it.
@@ -4699,6 +4727,7 @@ STR_TO_TRI = {
     _T_EQUAL,
     _T_GREATER,
     _T_GREATER_EQUAL,
+    _T_GSOURCE,
     _T_HELP,
     _T_HEX,
     _T_IF,
@@ -4725,7 +4754,7 @@ STR_TO_TRI = {
     _T_TRISTATE,
     _T_UNEQUAL,
     _T_VISIBLE,
-) = range(45)
+) = range(46)
 
 # Public integers representing expression types
 #
@@ -4759,6 +4788,7 @@ _get_keyword = {
     "endif":          _T_ENDIF,
     "endmenu":        _T_ENDMENU,
     "env":            _T_ENV,
+    "gsource":        _T_GSOURCE,
     "help":           _T_HELP,
     "hex":            _T_HEX,
     "if":             _T_IF,
@@ -4792,6 +4822,7 @@ _STRING_LEX = frozenset((
     _T_BOOL,
     _T_CHOICE,
     _T_COMMENT,
+    _T_GSOURCE,
     _T_HEX,
     _T_INT,
     _T_MAINMENU,

@ulfalizer
Copy link
Owner

Hmm... think that version is broken for absolute paths with $srctree set though. chdir() version might not be so bad. Keeps the paths untouched and avoids surprises.

@carlescufi
Copy link
Contributor

@ulfalizer @SebastianBoe this version worked fine with absolute paths and $srctree

@ulfalizer
Copy link
Owner

ulfalizer commented Mar 12, 2018

@carlescufi
There's subtle differences in behavior I think, e.g. when the file exists in the working directory but not in $srctree. I think that version would also add $srctree as a prefix to MenuNode.filename, which is inconsistent.

Here's what I had in mind:

When $srctree is set, source searches the working directory before $srctree (I suspect no one ever uses that, but it's what the C tools do, and it was easy to implement). Defining what the equivalent behavior for a globbing source should be gets tricky, so I decided to keep it simple and restrict gsource to only search $srctree when set.

Something like

gsource "foo*"

would then search $srctree and turn into the equivalent of e.g.

source "foo1"
source "foo2"
source "foo3"

You could then still have foo1, foo2, etc. files in the working directory if you wanted to "override" those files. The only thing you'd lose would be the ability to have globbed files exist in the working directory but not in $srctree. I suspect no one would need that, and the only-search-$srctree (but allow overriding) behavior is simpler to understand and gives simpler code.

It's splitting hairs anyway, as I might be the only one who cares about those corner cases. :P

@ulfalizer
Copy link
Owner

ulfalizer commented Mar 12, 2018

Heh... thinking about it, the only reason the C tools do that is probably so that people can have a .config in the working directory when doing out-of-tree builds. They just happen to use the same function for loading .config and Kconfig files, which accidentally gives that overriding behavior for Kconfig files. The intent was probably to always source a file in $srctree. Bit hairy.

ulfalizer added a commit that referenced this pull request Mar 13, 2018
'gsource' works like 'source', but takes a glob pattern and sources all
matching files. Works as a no-op if no files match, and hence doubles as
an include-if-exists function, similar to '-include' in 'make'.

Add a 'grsource' statement as well, mirroring 'rsource'.

Came up in #40.
@ulfalizer
Copy link
Owner

ulfalizer commented Mar 13, 2018

I pushed a fixed version of the second version: daac69d. I implemented grsource as well (I kept mistyping rgsource, so that was probably a sign).

Tell me if you have any objections. It could be tweaked still, as it's not in any release.

SebastianBoe added a commit to SebastianBoe/zephyr that referenced this pull request Mar 14, 2018
This is an RFC for changing the Kconfig dialect that Zephyr is using.

Zephyr has copied and modified the Linux kernel's configuration
language into it's own tree. One modification that Zephyr has done is
changed the semantics of the 'source' keyword. Instead of the 'source'
argument being treated as a path it is treated as a glob pattern[0].

This is a backwards-incompatible change with the kernel's
configuration language, not just an extention of what 'source'
accepts. Meaning, running our Kconfig processor on the Linux kernel's
sources can produce different results.

Specifically, if there is a typo in the 'source' path the linux kernel
will trigger an error, but in Zephyr this is silently treated as a
pattern that had no matches.

Also, when glob special characters are used the two languages will
match on different source files.

The Linux kernel's literal 'source' keyword is not present in Zephyr,
but it is useful, and often desired behaviour. E.g. the intended and
expected behaviour of:

source "Kconfig.zophyr"

would be to trigger an error due to a typo in the file name. But in
the Zephyr dialect this is treated as a valid glob pattern that
happened to not have any matches.

Another issue with changing the semantics of 'source' is that Kconfig
code and Kconfig infrastructure can no longer be re-used between the
Linux kernel and Zephyr as they are working with different languages.

To resolve these issues this RFC proposes to revert 'source' back to
the original Linux kernel semantics and introduce the 'gsource'
keyword to cover the use-case of wildcard matches.

In this solution the linux kernel's language will be a subset of the
Zephyr language and if a user wanted to he could write portable
Kconfig if he limited himself to this subset.

Script to reword wildcard matches to 'gsource':
https://gist.github.com/SebastianBoe/8d36b5b1af0491e5abc8fd598ded976c

RFC to introduce 'gsource':
ulfalizer/Kconfiglib#40

[0] https://en.wikipedia.org/wiki/Glob_(programming)

Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>
@ulfalizer
Copy link
Owner

Closing for now.

@ulfalizer ulfalizer closed this Mar 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants