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.

Sign up for GitHub

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

Jump to bottom

parsing: Add support for wildcards and globbing #34

Closed
wants to merge 1 commit into from
Closed

parsing: Add support for wildcards and globbing #34

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 42 additions & 27 deletions kconfiglib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,7 @@
service, or open a ticket on the GitHub page.
"""
import errno
import glob
import os
import platform
import re
Expand Down Expand Up @@ -997,34 +998,45 @@ def __repr__(self):
# File reading
#

def _open(self, filename):
def _resolve(self, filename, globbing):
"""
First tries to open 'filename', then '$srctree/filename' if $srctree
First tries with 'filename', then '$srctree/filename' if $srctree
was set when the configuration was loaded.
"""
if os.path.isfile(filename):
return [filename]

if not os.path.isabs(filename) and self.srctree is not None:
filename = os.path.join(self.srctree, filename)
Copy link
Owner

Choose a reason for hiding this comment

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

This reorganization causes some selftests to fail, because paths registered for items now include $srctree. Before, they only included the filename as passed.

fail: expected <menu node for symbol MULTI_DEF, deps y, has next, Kconfiglib/tests/Klocation_included:3> to have the location tests/Klocation_included:3, had the location Kconfiglib/tests/Klocation_included:3
fail: expected <menu node for choice CHOICE, deps y, has next, Kconfiglib/tests/Klocation_included:5> to have the location tests/Klocation_included:5, had the location Kconfiglib/tests/Klocation_included:5
fail: expected <menu node for menu, prompt "menu" (visibility y), deps y, 'visible if' deps y, has next, Kconfiglib/tests/Klocation_included:10> to have the location tests/Klocation_included:10, had the location Kconfiglib/tests/Klocation_included:10
fail: expected <menu node for comment, prompt "comment" (visibility y), deps y, has next, Kconfiglib/tests/Klocation_included:15> to have the location tests/Klocation_included:15, had the location Kconfiglib/tests/Klocation_included:15

Might be fine to just update the tests to expect $srctree to be included. Has the pro that the paths returned reflect the actual paths, and the con (or pro, depending on the case) that scripts might give different results out-of-tree.

This patch would do it in that case:

--- a/testsuite.py
+++ b/testsuite.py
@@ -762,22 +762,23 @@ g
     os.environ.pop("EXPANDED_FROM_ENV", None)
     os.environ.pop("srctree", None)
 
-    verify_locations(c.syms["SINGLE_DEF"].nodes, "tests/Klocation:4")
+    verify_locations(c.syms["SINGLE_DEF"].nodes,
+                     "Kconfiglib/tests/Klocation:4")
 
     verify_locations(c.syms["MULTI_DEF"].nodes,
-      "tests/Klocation:6",
-      "tests/Klocation:16",
-      "tests/Klocation_included:3",
-      "tests/Klocation:32")
+      "Kconfiglib/tests/Klocation:6",
+      "Kconfiglib/tests/Klocation:16",
+      "Kconfiglib/tests/Klocation_included:3",
+      "Kconfiglib/tests/Klocation:32")
 
     verify_locations(c.named_choices["CHOICE"].nodes,
-                     "tests/Klocation_included:5")
+                     "Kconfiglib/tests/Klocation_included:5")
 
     verify_locations([c.syms["MENU_HOOK"].nodes[0].next],
-                     "tests/Klocation_included:10")
+                     "Kconfiglib/tests/Klocation_included:10")
 
     verify_locations([c.syms["COMMENT_HOOK"].nodes[0].next],
-                     "tests/Klocation_included:15")
+                     "Kconfiglib/tests/Klocation_included:15")

The code that opens the top-level Kconfig file would have to be tweaked as well (but see the comment re. the behavior of glob() below):

--- a/kconfiglib.py
+++ b/kconfiglib.py
@@ -628,7 +628,7 @@ class Kconfig(object):
         self._filestack = []
 
         # The current parsing location
-        self._filename = filename
+        self._filename = self._resolve(filename, False)[0]
         self._linenr = 0
 
         self._file = self._open(filename)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry about the test failures. Is there doc about how to run the tests myself so I can fix this?

Copy link
Owner

Choose a reason for hiding this comment

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

Check out https://github.com/ulfalizer/Kconfiglib#test-suite. With a Linux kernel tree in linux, you can run these commands:

$ cd linux
$ git clone https://github.com/ulfalizer/Kconfiglib.git
$ git am Kconfiglib/makefile.patch
$ pypy Kconfiglib/testsuite.py speedy

(pypy is faster than CPython for running the test suite.)


if os.path.isfile(filename):
return [filename]

if globbing:
# Try globbing
return glob.glob(filename)
Copy link
Owner

@ulfalizer ulfalizer Jan 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It seems glob() returns an empty list if no file is found (including when no glob metacharacters are used). This would cause an IndexError in filename = self._resolve(filename, False)[0] in _open(), instead of a more helpful error. I think the empty result case would have to be checked separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I haven't understood fully: how could it cause an IndexError if I'm calling self._resolve with globbing=False? No globbing would be attempted in that case and an exception would be raised instead?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah... sorry, I read it too quickly. Yeah, should be safe in that case.

I was thinking of the source "nonexistent" case. I'm guessing the source would actually be skipped there now, since glob("nonexistent") returns an empty list. It would be better if it still gave an error, to avoid hard-to-diagnose issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is intentional. In the Zephyr kernel there are some instances of wildcards that match nothing at all. I was raising an error earlier but had to remove it to be able to process Zephyr's Kconfig tree. I think it's acceptable, given that "no match" is somehow admittable as a result of a wildcard match.

Copy link
Owner

Choose a reason for hiding this comment

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

It's fine for source with wildcards, but when it's just a plain filename, I'd still want it to give an error. Otherwise, source statements in other projects that reference nonexistent files would start to fail silently.

Just out of curiosity, do you use a dynamic tree, or is it the same set of files being globbed up every time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it's a reasonable suggestion. Let me check and get back to you.

Copy link
Owner

@ulfalizer ulfalizer Jan 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If you go for a separate keyword, it won't matter much if the behavior is a bit wonky at first either, because it won't have any great effect on how the vanilla Kconfig logic works, and could be improved over time. Gets a bit more iffy when core behavior might change.

Could ignore $srctree entirely until when (if) you need it, for example. It'd be more of a pure extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about a global flag that defaults to False when initializing Kconfig? Something like conf=Kconfig(..., globbing=True). This would keep the current behaviour untouched and would simplify our transition to Kconfiglib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be transparent for current and future users of Kconfiglib who don't care about globbing, but it would clutter the constructor API a tiny bit when inspecting it. If you don't think this is acceptable then we can also change our Kconfig files.

Copy link
Owner

Choose a reason for hiding this comment

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

Think I would prefer if the Kconfig files were changed. A flag would need to be supported in the future if people start depending on it, possibly becoming deprecated luggage at some point if it's no longer needed. I try to avoid stuff like that if possible. Plus the globbing, $srctree, etc. interactions are pretty tricky to follow already, and would get trickier.

Copy link
Owner

Choose a reason for hiding this comment

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

One thing that's a bit inconsistent is that if $srctree is set, then globbing will only be tried in $srctree. Normally, $srctree is only checked if no file is found in the working directory.

Mostly just a comment. Globbing and $srctree interactions get tricky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought about that too. But then again if one has $srctree set incorrectly I think it's expected that Kconfig will fail?

Copy link
Owner

Choose a reason for hiding this comment

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

I think the motivation for trying the working directory first (I got the logic from the C implementation) is that it allows you to selectively override files when you do out-of-tree builds. The expected behavior when you combine that with wildcards isn't obvious though, so I'm fine with it working differently.


raise IOError(
"Could not find '{}'. Perhaps the $srctree "
"environment variable (which was {}) is set incorrectly. Note "
"that the current value of $srctree is saved when the Kconfig "
"instance is created (for consistency and to cleanly "
"separate instances)."
.format(filename,
"unset" if self.srctree is None else
'"{}"'.format(self.srctree)))

def _open(self, filename):
"""
Normalize the filename based on $srctree
"""
filename = self._resolve(filename, False)[0]
try:
return open(filename)
except IOError as e:
if not os.path.isabs(filename) and self.srctree is not None:
filename = os.path.join(self.srctree, filename)
try:
return open(filename)
except IOError as e2:
# This is needed for Python 3, because e2 is deleted after
# the try block:
#
# https://docs.python.org/3/reference/compound_stmts.html#the-try-statement
e = e2

raise IOError(
"Could not open '{}' ({}: {}). Perhaps the $srctree "
"environment variable (which was {}) is set incorrectly. Note "
"that the current value of $srctree is saved when the Kconfig "
"instance is created (for consistency and to cleanly "
"separate instances)."
.format(filename, errno.errorcode[e.errno], e.strerror,
"unset" if self.srctree is None else
'"{}"'.format(self.srctree)))
"Could not open '{}' ({}: {})."
.format(filename, errno.errorcode[e.errno], e.strerror))

def _enter_file(self, filename):
"""
Expand Down Expand Up @@ -1456,12 +1468,15 @@ def _parse_block(self, end_token, parent, visible_if_deps, prev_node):
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())
f = self._resolve(f, True)
for s in f:
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
Expand Down