Repository: modm-io/lbuild

Issue:
PathOption, PathCollector needs the localpath of the config
Currently the Option objects are given no context object, which they could use to interpret the values in.
For the PathOption this means all paths are interpreted in the current working directory of the lbuild call, which means inheriting configurations that reference paths from *their* CWD doesn't work.

The same issue happens for PathCollectors, however, there you can use `localpath()/repopath()`, but this isn't done automatically (since you may also reference the generated output path).

Discussion:

Patch:
diff --git a/README.md b/README.md
index e53cf87..c1c9876 100644
--- a/README.md
+++ b/README.md
@@ -724,11 +724,20 @@ Since an empty string is not a valid path, but it can be useful to allow an
 empty string as an input value to encode a special case (like a "disable" value),
 you may set `empty_ok=True` to tell the path validation to ignore empty strings.
 
+By default, the path input is not modified and must be correctly interpreted in
+the context of the module that uses it (usually relocated to the output path).
+However, if you want to input an existing path you should set `absolute=True`,
+so that *lbuild* can relocate the *relative path* declared in the config files
+to an absolute path, which is indepented of the CWD.
+This is particularly useful if you declare paths in config files that are not
+located at the project root, like options inherited from multiple `lbuild.xml`.
+
 ```python
 option = PathOption(name="option-name",
                     description="path",
                     default="path/to/folder/or/file",
                     empty_ok=False, # is an empty path considered valid?
+                    absolute=False, # is the path relative to the config file?
                     dependencies=add_option_dependencies)
 ```
 
@@ -984,7 +993,8 @@ See [PathOption](#PathOption) for documentation.
 ```python
 collector = PathCollector(name="collector-name",
                           description="path",
-                          empty_ok=False)
+                          empty_ok=False,
+                          absolute=False)
 ```
 
 
diff --git a/lbuild/collector.py b/lbuild/collector.py
index 0112b03..a647c18 100644
--- a/lbuild/collector.py
+++ b/lbuild/collector.py
@@ -42,8 +42,9 @@ class Collector(BaseNode):
     def module(self):
         return self.parent
 
-    def add_values(self, values, module, operations=None):
+    def add_values(self, values, module, operations=None, filename=None):
         checked_values = list()
+        self._option._filename = filename
         for value in lbuild.utils.listify(values):
             self._option.value = value
             checked_values.append(self._option.value)
@@ -53,7 +54,7 @@ class Collector(BaseNode):
                 context = CollectorContext(operation.module, operation.filename)
                 self._extend_values(context, checked_values)
         else:
-            self._extend_values(CollectorContext(module.fullname), checked_values)
+            self._extend_values(CollectorContext(module), checked_values)
 
     def _extend_values(self, context, values):
         if context not in self._values:
@@ -96,8 +97,8 @@ class StringCollector(StringOption):
 
 
 class PathCollector(PathOption):
-    def __init__(self, name, description, empty_ok=False):
-        PathOption.__init__(self, name, description, empty_ok=empty_ok)
+    def __init__(self, name, description, empty_ok=False, absolute=False):
+        PathOption.__init__(self, name, description, empty_ok=empty_ok, absolute=absolute)
 
 
 class BooleanCollector(BooleanOption):
@@ -132,4 +133,4 @@ class CallableCollector(Option):
         return ["Callable"]
 
     def format_values(self):
-        return _cw("Callable")
\ No newline at end of file
+        return _cw("Callable")
diff --git a/lbuild/config.py b/lbuild/config.py
index 5c98167..a896076 100644
--- a/lbuild/config.py
+++ b/lbuild/config.py
@@ -62,7 +62,7 @@ class ConfigNode(anytree.AnyNode):
     def add_commandline_options(self, cmd_options):
         for option in cmd_options:
             parts = option.split('=')
-            self._options[parts[0]] = parts[1]
+            self._options[parts[0]] = (parts[1], os.path.join(os.getcwd(), "command-line"))
 
     def _flatten(self, config):
         for node in list(self.siblings) + [self]:
@@ -196,8 +196,8 @@ class ConfigNode(anytree.AnyNode):
         # Load repositories
         for path_node in xmltree.iterfind("repositories/repository/path"):
             repopath = path_node.text.format(cache=config._cachefolder)
-            filename = ConfigNode._rel_path(repopath, configpath)
-            config._repositories.append(filename)
+            rfilename = ConfigNode._rel_path(repopath, configpath)
+            config._repositories.append(rfilename)
 
         # Load all requested modules
         config._modules = xmltree.xpath('modules/module/text()')
@@ -206,7 +206,7 @@ class ConfigNode(anytree.AnyNode):
         for option_node in xmltree.xpath('options/option'):
             name = option_node.attrib['name']
             value = option_node.attrib.get('value', option_node.text)
-            config._options[name] = value
+            config._options[name] = (value, filename)
 
         return config
 
diff --git a/lbuild/environment.py b/lbuild/environment.py
index 0fce901..60dde72 100644
--- a/lbuild/environment.py
+++ b/lbuild/environment.py
@@ -332,7 +332,10 @@ class Environment:
 
     def add_to_collector(self, key, *values, operations=None):
         try:
-            self.collectors_available[key].add_values(values, self.__module, operations)
+            self.collectors_available[key].add_values(values,
+                                                      module=self.__module.fullname,
+                                                      filename=self.__module._filename,
+                                                      operations=operations)
         except le.LbuildOptionException as error:
             raise le.LbuildEnvironmentCollectException(self.__module, str(error))
 
diff --git a/lbuild/main.py b/lbuild/main.py
index 29e87a1..fe5ff1a 100644
--- a/lbuild/main.py
+++ b/lbuild/main.py
@@ -22,7 +22,7 @@ from lbuild.format import format_option_short_description
 
 from lbuild.api import Builder
 
-__version__ = '1.11.4'
+__version__ = '1.11.5'
 
 
 class InitAction:
diff --git a/lbuild/option.py b/lbuild/option.py
index aa46302..96a6c9f 100644
--- a/lbuild/option.py
+++ b/lbuild/option.py
@@ -8,6 +8,7 @@
 # 2-clause BSD license. See the file `LICENSE.txt` for the full license
 # governing this code.
 
+import os
 import enum
 import inspect
 import collections
@@ -34,6 +35,7 @@ class Option(BaseNode):
         self._input = None
         self._output = None
         self._default = None
+        self._filename = os.path.join(os.getcwd(), "dummy")
         self._set_default(default)
 
     def _set_default(self, default):
@@ -105,16 +107,31 @@ class StringOption(Option):
 
 class PathOption(Option):
 
-    def __init__(self, name, description, default=None, dependencies=None, empty_ok=False):
+    def __init__(self, name, description, default=None, dependencies=None, empty_ok=False, absolute=False):
         self._empty_ok = empty_ok
+        self._absolute = absolute
         Option.__init__(self, name, description, default, dependencies,
                         convert_input=self._validate_path,
-                        convert_output=lambda p: str(p).strip())
+                        convert_output=self._relocate_path)
+
+    def _relocate(self, path):
+        return self._absolute and (self._filename is not None) and (path != "")
+
+    def _relocate_path(self, path):
+        path = str(path).strip()
+        # relocate path relative to the option filepath to absolute
+        if self._relocate(path):
+            path = os.path.realpath(os.path.join(self._filepath, path))
+        return path
 
     def _validate_path(self, path):
         path = str(path).strip()
+        # call the path validator function
         if not self.validate(path, self._empty_ok):
             raise TypeError("Input must be a path!")
+        # Relocate path to be relative to the user's perspective
+        if self._relocate(path):
+            path = os.path.relpath(self._relocate_path(path), os.getcwd())
         return path
 
     @staticmethod
diff --git a/lbuild/parser.py b/lbuild/parser.py
index 778fa0b..f2712de 100644
--- a/lbuild/parser.py
+++ b/lbuild/parser.py
@@ -140,10 +140,13 @@ class Parser(BaseNode):
     def merge_repository_options(self):
         # only deal with repo options that contain one `:`
         resolver = self.option_resolver
-        for name, value in {n: v for n, v in self.config.options.items()
-                            if n.count(":") == 1}.items():
+        # print(self.config.options.items())
+        for name, (value, filename) in filter(lambda i: i[0].count(":") == 1,
+                                              self.config.options.items()):
             try:
-                resolver[name].value = value
+                option = resolver[name]
+                option._filename = filename
+                option.value = value
             except le.LbuildOptionException as error:
                 raise le.LbuildDumpConfigException(
                     "Failed to validate repository options!\n{}".format(error), self)
@@ -174,10 +177,12 @@ class Parser(BaseNode):
     def merge_module_options(self):
         # only deal with repo options that contain one `:`
         resolver = self.option_resolver
-        for name, value in {n: v for n, v in self.config.options.items()
-                            if n.count(":") > 1}.items():
+        for name, (value, filename) in filter(lambda i: i[0].count(":") > 1,
+                                              self.config.options.items()):
             try:
-                resolver[name].value = value
+                option = resolver[name]
+                option._filename = filename
+                option.value = value
             except le.LbuildOptionException as error:
                 raise le.LbuildDumpConfigException(
                     "Failed to validate module options!\n{}".format(error), self)


Test patch:
diff --git a/test/api_test.py b/test/api_test.py
index 3244565..62b863b 100644
--- a/test/api_test.py
+++ b/test/api_test.py
@@ -56,7 +56,8 @@ class ApiTest(unittest.TestCase):
         self.assertEqual(api.cwd, self._get_path("parser/api"))
         self._assert_config(api,
             filename=self._rel_path("parser/api/simple.xml"),
-            options={"repo:option": "value", "repo:module:option": "value"},
+            options={"repo:option": ("value", self._get_path("parser/api/simple.xml")),
+                     "repo:module:option": ("value", self._get_path("parser/api/simple.xml"))},
             modules=["repo:module"],
             repositories=[self._get_path("parser/api/repo.lb")],
             cachefolder=self._rel_path("parser/api/.lbuild_cache"))
@@ -66,7 +67,7 @@ class ApiTest(unittest.TestCase):
         self.assertEqual(api.cwd, self._get_path("config"))
         self._assert_config(api,
             filename=self._rel_path("config/lbuild.xml"),
-            options={":target": "hosted"},
+            options={":target": ("hosted", self._get_path("config/lbuild.xml"))},
             modules=[":module1"],
             repositories=[self._get_path("config/repo1.lb")],
             cachefolder=self._rel_path("config/.lbuild_cache"))
@@ -76,7 +77,7 @@ class ApiTest(unittest.TestCase):
         self.assertEqual(api.cwd, self._get_path("parser/api"))
         self._assert_config(api,
             filename=self._rel_path("parser/api/project.xml"),
-            options={"repo:option": "value"},
+            options={"repo:option": ("value", self._get_path("parser/api/project.xml"))},
             modules=["repo:module"],
             repositories=[self._get_path("parser/api/repo.lb")],
             cachefolder=self._rel_path("parser/api/.lbuild_cache"))
diff --git a/test/collector_test.py b/test/collector_test.py
index 8b469c2..8e9dc46 100644
--- a/test/collector_test.py
+++ b/test/collector_test.py
@@ -56,11 +56,11 @@ class CollectorTest(unittest.TestCase):
     def test_should_access_values(self):
         collector = Collector(NumericCollector("test", "long description"))
 
-        collector.add_values(1, self.module1)
-        collector.add_values(2, self.module1)
+        collector.add_values(1, self.module1.fullname)
+        collector.add_values(2, self.module1.fullname)
 
-        collector.add_values(2, self.module2)
-        collector.add_values([3, 3], self.module2)
+        collector.add_values(2, self.module2.fullname)
+        collector.add_values([3, 3], self.module2.fullname)
 
         unique_values = collector.values()
         unique_values1 = collector.values(filterfunc=lambda s: s.module == "repo:module1")
@@ -89,11 +89,11 @@ class CollectorTest(unittest.TestCase):
 
         def function():
             pass
-        collector.add_values(function, self.module1)
+        collector.add_values(function, self.module1.fullname)
         self.assertEqual([function], collector.values())
 
         self.assertRaises(le.LbuildOptionInputException,
-                          lambda: collector.add_values(1, self.module1))
+                          lambda: collector.add_values(1, self.module1.fullname))
 
 
 if __name__ == '__main__':
diff --git a/test/config_test.py b/test/config_test.py
index 0142331..d3ede70 100644
--- a/test/config_test.py
+++ b/test/config_test.py
@@ -74,14 +74,14 @@ class ConfigTest(unittest.TestCase):
         self.assertIn("::submodule3", modules)
 
         self.assertEqual(7, len(config.options))
-        self.assertEqual(config.options[':target'], 'hosted')
-        self.assertEqual(config.options['repo1:foo'], '43')
+        self.assertEqual(config.options[':target'][0], 'hosted')
+        self.assertEqual(config.options['repo1:foo'][0], '43')
 
-        self.assertEqual(config.options['repo1:other:foo'], '456')
-        self.assertEqual(config.options['repo1::bar'], '768')
-        self.assertEqual(config.options[':other:xyz'], 'No')
-        self.assertEqual(config.options['::abc'], 'Hello World!')
-        self.assertEqual(config.options['::submodule3::price'], '15')
+        self.assertEqual(config.options['repo1:other:foo'][0], '456')
+        self.assertEqual(config.options['repo1::bar'][0], '768')
+        self.assertEqual(config.options[':other:xyz'][0], 'No')
+        self.assertEqual(config.options['::abc'][0], 'Hello World!')
+        self.assertEqual(config.options['::submodule3::price'][0], '15')
 
     def test_should_parse_base_configuration(self):
         config = self._parse_config("configfile_inheritance/depth_0.xml")
@@ -90,8 +90,8 @@ class ConfigTest(unittest.TestCase):
         self.assertIn(self._get_path("configfile_inheritance/repo3.lb"), config.repositories)
 
         self.assertEqual(2, len(config.options))
-        self.assertEqual(config.options[":other:xyz"], "No")
-        self.assertEqual(config.options["::abc"], "Hello World!")
+        self.assertEqual(config.options[":other:xyz"][0], "No")
+        self.assertEqual(config.options["::abc"][0], "Hello World!")
 
         self.assertEqual(1, len(config.modules))
         self.assertIn("repo1:other", config.modules)
@@ -104,11 +104,11 @@ class ConfigTest(unittest.TestCase):
         self.assertIn(self._get_path("configfile_inheritance/repo3.lb"), config.repositories)
 
         self.assertEqual(5, len(config.options))
-        self.assertEqual(config.options[":other:xyz"], "No")
-        self.assertEqual(config.options["::abc"], "Hello World!")
-        self.assertEqual(config.options["repo1:foo"], "43")
-        self.assertEqual(config.options["repo1:other:foo"], "456")
-        self.assertEqual(config.options["repo1::bar"], "768")
+        self.assertEqual(config.options[":other:xyz"][0], "No")
+        self.assertEqual(config.options["::abc"][0], "Hello World!")
+        self.assertEqual(config.options["repo1:foo"][0], "43")
+        self.assertEqual(config.options["repo1:other:foo"][0], "456")
+        self.assertEqual(config.options["repo1::bar"][0], "768")
 
         self.assertEqual(2, len(config.modules))
         self.assertIn("repo1:other", config.modules)
@@ -122,14 +122,14 @@ class ConfigTest(unittest.TestCase):
         self.assertIn(self._get_path("configfile_inheritance/repo3.lb"), config.repositories)
 
         self.assertEqual(6, len(config.options))
-        self.assertEqual(config.options[":other:xyz"], "No")
-        self.assertEqual(config.options["::abc"], "Hello World!")
+        self.assertEqual(config.options[":other:xyz"][0], "No")
+        self.assertEqual(config.options["::abc"][0], "Hello World!")
 
-        self.assertEqual(config.options["repo1:other:foo"], "456")
-        self.assertEqual(config.options["repo1::bar"], "768")
+        self.assertEqual(config.options["repo1:other:foo"][0], "456")
+        self.assertEqual(config.options["repo1::bar"][0], "768")
 
-        self.assertEqual(config.options[":target"], "hosted")
-        self.assertEqual(config.options["repo1:foo"], "42")
+        self.assertEqual(config.options[":target"][0], "hosted")
+        self.assertEqual(config.options["repo1:foo"][0], "42")
 
         self.assertEqual(3, len(config.modules))
         self.assertIn("repo1:other", config.modules)
@@ -145,15 +145,15 @@ class ConfigTest(unittest.TestCase):
         self.assertIn(self._get_path("configfile_inheritance/repo3.lb"), config.repositories)
 
         self.assertEqual(7, len(config.options))
-        self.assertEqual(config.options["repo1:other:foo"], "456")
-        self.assertEqual(config.options["repo1::bar"], "768")
+        self.assertEqual(config.options["repo1:other:foo"][0], "456")
+        self.assertEqual(config.options["repo1::bar"][0], "768")
 
-        self.assertEqual(config.options[":other:xyz"], "Yes")
-        self.assertEqual(config.options["::abc"], "Hello World!")
-        self.assertEqual(config.options["::submodule3::price"], "15")
+        self.assertEqual(config.options[":other:xyz"][0], "Yes")
+        self.assertEqual(config.options["::abc"][0], "Hello World!")
+        self.assertEqual(config.options["::submodule3::price"][0], "15")
 
-        self.assertEqual(config.options[":target"], "hosted")
-        self.assertEqual(config.options["repo1:foo"], "42")
+        self.assertEqual(config.options[":target"][0], "hosted")
+        self.assertEqual(config.options["repo1:foo"][0], "42")
 
         self.assertEqual(5, len(config.modules))
         self.assertIn("::submodule3:subsubmodule2", config.modules)
diff --git a/test/dependency_test.py b/test/dependency_test.py
index e2aaa39..6a9578d 100644
--- a/test/dependency_test.py
+++ b/test/dependency_test.py
@@ -50,7 +50,7 @@ class DepedencyTest(unittest.TestCase):
     def test_should_update_option_dependencies(self):
         self.parser.parse_repository(self._get_path("option_dependency/repo.lb"))
         self.parser.prepare_repositories()
-        self.parser.config.options[":module2:dependency"] = ":module1"
+        self.parser.config.options[":module2:dependency"] = (":module1", None)
         self.parser.merge_module_options()
 
         module = self.parser.find_module(":module2")
diff --git a/test/option_test.py b/test/option_test.py
index 11c7355..35ee4d1 100644
--- a/test/option_test.py
+++ b/test/option_test.py
@@ -151,6 +151,24 @@ class OptionTest(unittest.TestCase):
         option = PathOption("test", "description", default="", empty_ok=True)
         self.assertEqual("", option.value)
 
+        option = PathOption("test", "description", default="filename.txt", absolute=True)
+        self.assertEqual("{}/filename.txt".format(os.getcwd()), option.value)
+        option._filename = "/root/test/hello.lb"
+        option.value = "filename.txt"
+        self.assertEqual("/root/test/filename.txt", option.value)
+        option.value = "/absolute/filename.txt"
+        self.assertEqual("/absolute/filename.txt", option.value)
+
+        option = PathOption("test", "description", default="", empty_ok=True, absolute=True)
+        self.assertEqual("", option.value)
+        option.value = "filename.txt"
+        self.assertEqual("{}/filename.txt".format(os.getcwd()), option.value)
+        option._filename = "/root/test/hello.lb"
+        option.value = "filename.txt"
+        self.assertEqual("/root/test/filename.txt", option.value)
+        option.value = "/absolute/filename.txt"
+        self.assertEqual("/absolute/filename.txt", option.value)
+
     def test_should_be_constructable_from_boolean(self):
         option = BooleanOption("test", "description", False)
         self.assertIn("test  [BooleanOption]", option.description)
diff --git a/test/parser_test.py b/test/parser_test.py
index c198be0..481482b 100644
--- a/test/parser_test.py
+++ b/test/parser_test.py
@@ -227,19 +227,19 @@ class ParserTest(unittest.TestCase):
     def test_should_build_fail_to_find_repo_options(self):
         # non-existant options should fail to resolve
         with self.assertRaises(le.LbuildParserNodeNotFoundException):
-            self._get_build_modules({"repo1:NOPE": "NOPE"})
+            self._get_build_modules({"repo1:NOPE": ("NOPE", None)})
 
     def test_should_build_fail_to_find_module_options(self):
         with self.assertRaises(le.LbuildParserNodeNotFoundException):
-            self._get_build_modules({"repo1:other:NOPE": "NOPE"})
+            self._get_build_modules({"repo1:other:NOPE": ("NOPE", None)})
             self.parser.merge_module_options()
 
     def test_should_build_fail_to_validate_options(self):
         # existant options with wrong input
         with self.assertRaises(le.LbuildDumpConfigException):
-            self._get_build_modules({"repo1:target": "NOPE"})
+            self._get_build_modules({"repo1:target": ("NOPE", None)})
         with self.assertRaises(le.LbuildDumpConfigException):
-            self._get_build_modules({"repo1:other:xyz": "NOPE"})
+            self._get_build_modules({"repo1:other:xyz": ("NOPE", None)})
             self.parser.merge_module_options()
 
     def test_should_not_build_modules_for_missing_options(self):
@@ -367,11 +367,11 @@ class ParserTest(unittest.TestCase):
 
         selected_modules = ["repo2:module3"]
         config_options = {
-            'repo1:target': 'hosted',
-            ':other:xyz': 'No',
-            'repo1::bar': '768',
-            'repo1:other:foo': '456',
-            '::abc': 'Hello World!',
+            'repo1:target': ('hosted', None),
+            ':other:xyz': ('No', None),
+            'repo1::bar': ('768', None),
+            'repo1:other:foo': ('456', None),
+            '::abc': ('Hello World!', None),
         }
         build_modules = self.prepare_modules(self.parser, selected_modules, config_options)