From 202107e7151a47ff112f849f57770fa62bd30ada Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Wed, 10 Jun 2020 14:01:34 +0900 Subject: _yaml.pdx: Remove false presumption from EISDIR error message Change the error message to reflect that the file is a directory, do not suggest that it should be a .bst file, it should not be a .bst file. A .bst file might be expected from the loader code, which can feel free to express that, but other files can be loaded, for example include files. --- src/buildstream/_yaml.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx index 373311a47..1e59b2a1c 100644 --- a/src/buildstream/_yaml.pyx +++ b/src/buildstream/_yaml.pyx @@ -283,7 +283,7 @@ cpdef MappingNode load(str filename, str shortname=None, bint copy_tree=False, o raise LoadError("Could not find file at {}".format(filename), LoadErrorReason.MISSING_FILE) from e except IsADirectoryError as e: - raise LoadError("{} is a directory. bst command expects a .bst file.".format(filename), + raise LoadError("{} is a directory".format(filename), LoadErrorReason.LOADING_DIRECTORY) from e except LoadError as e: raise LoadError("{}: {}".format(displayname, e), e.reason) from e -- cgit v1.2.1 From dd7b31e2135096086275b233982092d8a26ba393 Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Wed, 10 Jun 2020 14:04:22 +0900 Subject: _includes.py: Propagate provenance through Loader.get_loader() Instead of raising a customized error message which adds little value to the provenance, just pass the provenance along. This is important so that the Loader is aware of the provenance of loaded junctions, so that it can more precisely report errors about conflicting junctions when includes cause conflicts. This commit also adjusts tests/format/includes.py --- src/buildstream/_includes.py | 41 +++++++++++++++++------------------------ tests/format/include.py | 4 ++-- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/buildstream/_includes.py b/src/buildstream/_includes.py index 0c77e5fa1..e4b144337 100644 --- a/src/buildstream/_includes.py +++ b/src/buildstream/_includes.py @@ -78,39 +78,24 @@ class Includes: if includes_node: if type(includes_node) is ScalarNode: # pylint: disable=unidiomatic-typecheck - includes = [includes_node.as_str()] + includes = [includes_node] else: - includes = includes_node.as_str_list() + includes = includes_node del node["(@)"] for include in reversed(includes): - if only_local and ":" in include: + if only_local and ":" in include.as_str(): continue - try: - include_node, file_path, sub_loader = self._include_file(include, current_loader) - except LoadError as e: - include_provenance = includes_node.get_provenance() - if e.reason == LoadErrorReason.MISSING_FILE: - message = "{}: Include block references a file that could not be found: '{}'.".format( - include_provenance, include - ) - raise LoadError(message, LoadErrorReason.MISSING_FILE) from e - if e.reason == LoadErrorReason.LOADING_DIRECTORY: - message = "{}: Include block references a directory instead of a file: '{}'.".format( - include_provenance, include - ) - raise LoadError(message, LoadErrorReason.LOADING_DIRECTORY) from e - - # Otherwise, we don't know the reason, so just raise - raise + include_node, file_path, sub_loader = self._include_file(include, current_loader) if file_path in included: include_provenance = includes_node.get_provenance() raise LoadError( "{}: trying to recursively include {}".format(include_provenance, file_path), LoadErrorReason.RECURSIVE_INCLUDE, ) + # Because the included node will be modified, we need # to copy it so that we do not modify the toplevel # node of the provenance. @@ -144,14 +129,16 @@ class Includes: # Load include YAML file from with a loader. # # Args: - # include (str): file path relative to loader's project directory. - # Can be prefixed with junctio name. + # include (ScalarNode): file path relative to loader's project directory. + # Can be prefixed with junctio name. # loader (Loader): Loader for the current project. def _include_file(self, include, loader): + provenance = include.get_provenance() + include = include.as_str() shortname = include if ":" in include: junction, include = include.rsplit(":", 1) - current_loader = loader.get_loader(junction) + current_loader = loader.get_loader(junction, provenance=provenance) current_loader.project.ensure_fully_loaded() else: current_loader = loader @@ -160,7 +147,13 @@ class Includes: file_path = os.path.join(directory, include) key = (current_loader, file_path) if key not in self._loaded: - self._loaded[key] = _yaml.load(file_path, shortname=shortname, project=project, copy_tree=self._copy_tree) + try: + self._loaded[key] = _yaml.load( + file_path, shortname=shortname, project=project, copy_tree=self._copy_tree + ) + except LoadError as e: + raise LoadError("{}: {}".format(provenance, e), e.reason, detail=e.detail) from e + return self._loaded[key], file_path, current_loader # _process_value() diff --git a/tests/format/include.py b/tests/format/include.py index 5c273e1a0..d57dd8c19 100644 --- a/tests/format/include.py +++ b/tests/format/include.py @@ -44,7 +44,7 @@ def test_include_missing_file(cli, tmpdir): result = cli.run(project=str(tmpdir), args=["show", str(element.basename)]) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.MISSING_FILE) # Make sure the root cause provenance is in the output. - assert "line 4 column 2" in result.stderr + assert "include_missing_file.bst [line 4 column 4]" in result.stderr def test_include_dir(cli, tmpdir): @@ -68,7 +68,7 @@ def test_include_dir(cli, tmpdir): result = cli.run(project=str(tmpdir), args=["show", str(element.basename)]) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.LOADING_DIRECTORY) # Make sure the root cause provenance is in the output. - assert "line 4 column 2" in result.stderr + assert "include_dir.bst [line 4 column 4]" in result.stderr @pytest.mark.datafiles(DATA_DIR) -- cgit v1.2.1 From 25fda166974409a68419c74805d988ef26cc8ec3 Mon Sep 17 00:00:00 2001 From: Tristan van Berkom Date: Wed, 10 Jun 2020 15:39:14 +0900 Subject: _loader/loader.py: Make provenance a positional argument This forces the `provenance` to `Loader.get_loader()` to be a mandatory argument, ensuring that there are never any callers which fail to provide provenance. --- src/buildstream/_includes.py | 2 +- src/buildstream/_loader/loader.py | 10 ++++------ src/buildstream/_pluginfactory/pluginoriginjunction.py | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/buildstream/_includes.py b/src/buildstream/_includes.py index e4b144337..1556bc613 100644 --- a/src/buildstream/_includes.py +++ b/src/buildstream/_includes.py @@ -138,7 +138,7 @@ class Includes: shortname = include if ":" in include: junction, include = include.rsplit(":", 1) - current_loader = loader.get_loader(junction, provenance=provenance) + current_loader = loader.get_loader(junction, provenance) current_loader.project.ensure_fully_loaded() else: current_loader = loader diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py index 17f0d906f..5a5d24dc4 100644 --- a/src/buildstream/_loader/loader.py +++ b/src/buildstream/_loader/loader.py @@ -164,15 +164,15 @@ class Loader: # # Args: # name (str): Name of junction, may have multiple `:` in the name + # provenance (ProvenanceInformation): The provenance # rewritable (bool): Whether the loaded files should be rewritable # this is a bit more expensive due to deep copies # ticker (callable): An optional function for tracking load progress - # provenance (ProvenanceInformation): The provenance # # Returns: # (Loader): loader for sub-project # - def get_loader(self, name, *, rewritable=False, ticker=None, level=0, provenance=None): + def get_loader(self, name, provenance, *, rewritable=False, ticker=None, level=0): junction_path = name.split(":") loader = self @@ -421,9 +421,7 @@ class Loader: current_element[2].append(dep.name) if dep.junction: - loader = self.get_loader( - dep.junction, rewritable=rewritable, ticker=ticker, provenance=dep.provenance - ) + loader = self.get_loader(dep.junction, dep.provenance, rewritable=rewritable, ticker=ticker) dep_element = loader._load_file(dep.name, rewritable, ticker, dep.provenance) else: dep_element = self._elements.get(dep.name) @@ -780,7 +778,7 @@ class Loader: if len(junction_path) == 1: return None, junction_path[-1], self else: - loader = self.get_loader(junction_path[-2], rewritable=rewritable, ticker=ticker, provenance=provenance) + loader = self.get_loader(junction_path[-2], provenance, rewritable=rewritable, ticker=ticker) return junction_path[-2], junction_path[-1], loader # Print a warning message, checks warning_token against project configuration diff --git a/src/buildstream/_pluginfactory/pluginoriginjunction.py b/src/buildstream/_pluginfactory/pluginoriginjunction.py index 8c1d560fb..4e0a53cfb 100644 --- a/src/buildstream/_pluginfactory/pluginoriginjunction.py +++ b/src/buildstream/_pluginfactory/pluginoriginjunction.py @@ -35,7 +35,7 @@ class PluginOriginJunction(PluginOrigin): # Get access to the project indicated by the junction, # possibly loading it as a side effect. # - loader = self.project.loader.get_loader(self._junction, provenance=self.provenance) + loader = self.project.loader.get_loader(self._junction, self.provenance) project = loader.project project.ensure_fully_loaded() -- cgit v1.2.1