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

Proposal: support relative path in 'source' statement #38

Closed
romavis opened this issue Feb 25, 2018 · 4 comments
Closed

Proposal: support relative path in 'source' statement #38

romavis opened this issue Feb 25, 2018 · 4 comments

Comments

@romavis
Copy link
Contributor

romavis commented Feb 25, 2018

Hi,

I'm working on a project where build configuration system is implemented entirely in Python with Kconfiglib at its center. That way, we don't have to use any of Linux kernel Kconfig/Kbuild code, which has certain benefits. In our situation it's tempting to extend Kconfiglib with new features beyond what's offered by Linux kernel, and one such extension that may be useful for someone else is the support for relative paths in source directive.

Imagine the following directory structure:

Project
|
+--Kconfig
+--src
   +--Kconfig
   |
   +--SubSystem1
   |  +--Kconfig
   |  |
   |  +--ModuleA
   |  |  +--Kconfig
   |  |
   |  +--ModuleB
   |     +--Kconfig   
   |
   +--SubSystem2
      +--Kconfig
      |
      +--ModuleC
      |  +--Kconfig
      |
      +--ModuleD
         +--Kconfig

Here, Kconfig file in SubSystem1 sources Kconfig file from ModuleA, and to do this one writes:

source "src/SubSystem1/ModuleA/Kconfig"

Complete path from project root needs to be specified, which is error-prone and creates difficulties when for some reason one wants to rename/move SubSystem1 directory, as paths should be modified in all Kconfig files under affected directory. Sometimes Kconfig files are nested more deeply than in my example, so it becomes even worse.

The proposal is to add support for relative paths in source directive, so that path relative to directory of currently processed Kconfig file can be specified instead of path relative to project root:

source "ModuleA/Kconfig"

Obviously, this mode of source directive interpretation is not compatible with Linux kernel tools. I see two ways how this can be enabled without breaking existing code:

  1. Add optional argument to Kconfig constructor:

    def __init__(self, filename="Kconfig", warn=True, source_uses_relative_paths=False):
    

    This doesn't change Kconfig language, but in many other aspects it's ugly:

    • If such Kconfig infrastructure is by mistake used with Linux kernel tools, cryptic error messages would be rised about non-existing files. Or, even worse, if relative path specified in Kconfig file nested deep inside project directory tree exists in project root, then Linux tools may interpret configuration incorrectly without issuing warnings of any kind..
    • The support for new constructor argument needs to be implemented in all tools based on Kconfiglib that you want to use in your configuration system
    • It's implicit: you can't immediately tell by looking into Kconfig files which kind of paths is used
  2. Add new Kconfig language statement, for example rsource:

    rsource "ModuleA/Kconfig"
    

    Pros:

    • The new statement explicitly tells user that relative path is used instead of standard behavior
    • Linux kernel tools would fail with clean syntax error about unknown token if used by mistake to parse your Kconfig files, so there should be no silent bugs
    • source and rsource can be intermixed in Kconfig files if needed

    Cons:

    • It's possible to have a conflict in future if Linux kernel devs introduce rsource statement in kernel tools. Not sure about right way to prevent this...

I personally prefer the rsource variant (sure, new directive name is subject to discussion).
If you find this feature useful, please let me know and I will prepare a pull request.

Thanks,
Roman

@ulfalizer
Copy link
Owner

Hello,

I'd be happy to take this in. I also prefer the rsource approach, as it's more flexible and keeps the extension nicely separated.

I doubt rsource will ever appear in the kernel tools, but we could make the following deal: If rsource appears upstream with different behavior, then I will implement that instead, and maybe rename the extension if it still makes sense. :)

@ulfalizer
Copy link
Owner

I hacked up a version of it. Feel free to use it in the pull request if it seems fine. I could add some documentation and tests for it after that.

The Zephyr project is modding the plain source to add globbing in a separate Kconfiglib fork. Maybe it would save them some pain to keep _T_RSOURCE separated like this for now, even though there's a little bit of code duplication with _T_SOURCE. I could always clean it up a bit later if it makes sense.

diff --git a/kconfiglib.py b/kconfiglib.py
index a05a079..16ec669 100644
--- a/kconfiglib.py
+++ b/kconfiglib.py
@@ -1572,6 +1572,18 @@ class Kconfig(object):
                                               prev_node)
                 self._leave_file()
 
+            elif t0 == _T_RSOURCE:
+                self._enter_file(
+                    os.path.join(
+                        os.path.dirname(self._filename),
+                        self._expand_syms(self._expect_str_and_eol())))
+
+                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.
@@ -4450,13 +4462,14 @@ STR_TO_TRI = {
     _T_OR,
     _T_PROMPT,
     _T_RANGE,
+    _T_RSOURCE,
     _T_SELECT,
     _T_SOURCE,
     _T_STRING,
     _T_TRISTATE,
     _T_UNEQUAL,
     _T_VISIBLE,
-) = range(44)
+) = range(45)
 
 # Keyword to token map, with the get() method assigned directly as a small
 # optimization
@@ -4490,6 +4503,7 @@ _get_keyword = {
     "optional":       _T_OPTIONAL,
     "prompt":         _T_PROMPT,
     "range":          _T_RANGE,
+    "rsource":        _T_RSOURCE,
     "select":         _T_SELECT,
     "source":         _T_SOURCE,
     "string":         _T_STRING,
@@ -4508,6 +4522,7 @@ _STRING_LEX = frozenset((
     _T_MAINMENU,
     _T_MENU,
     _T_PROMPT,
+    _T_RSOURCE,
     _T_SOURCE,
     _T_STRING,
     _T_TRISTATE,

@ulfalizer
Copy link
Owner

ulfalizer commented Feb 27, 2018

The propsed feature was integrated by aea0232, from pull request #39.

@ulfalizer
Copy link
Owner

I pushed out a new 3.2.0 release that includes the new feature to PyPI as well.

Thanks!

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

No branches or pull requests

2 participants