From bbbe1f14e0c2644f62c628406c0a26fc9bb67554 Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Thu, 14 Mar 2019 13:21:34 +0000 Subject: loader: clearer error if no junction project.conf I found the original message confusing - it would talk about wanting a project at '.', without a frame of reference. After re-reading the code and junction docs it made more sense, I think this version would save folks the trouble. Removing all mention of it, if it's not specified. If it is specified then explain it a bit more, i.e. Before: Could not find the project.conf file for junction element at hello-junction.bst [line 1 column 0]. Expecting a project at path 'project' After: Could not find the project.conf file in project referred to by junction element 'hello-junction.bst'. Was expecting it at path 'project' in the junction's source. If no 'path' specified in the junction: Could not find the project.conf file in project referred to by junction element 'hello-junction.bst'. --- buildstream/_loader/loader.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/buildstream/_loader/loader.py b/buildstream/_loader/loader.py index 9b91e91fe..f7fd3203e 100644 --- a/buildstream/_loader/loader.py +++ b/buildstream/_loader/loader.py @@ -570,10 +570,14 @@ class Loader(): parent_loader=self) except LoadError as e: if e.reason == LoadErrorReason.MISSING_PROJECT_CONF: + message = ( + "Could not find the project.conf file in the project " + "referred to by junction element '{}'.".format(element.name) + ) + if element.path: + message += " Was expecting it at path '{}' in the junction's source.".format(element.path) raise LoadError(reason=LoadErrorReason.INVALID_JUNCTION, - message="Could not find the project.conf file for {}. " - "Expecting a project at path '{}'" - .format(element, element.path or '.')) from e + message=message) from e else: raise -- cgit v1.2.1 From 0c6e1183851adcce649e842a0b13412b3abbc968 Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Thu, 14 Mar 2019 16:57:00 +0000 Subject: runcli.run(): coerce project and args to strings It would be convenient if we could use datafiles directly, instead of fussing about with os.path.join(). The standard library supports path-like things, I see no reason why we shouldn't. In later work we can refactor our tests to take advantage of this. --- buildstream/plugintestutils/runcli.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/buildstream/plugintestutils/runcli.py b/buildstream/plugintestutils/runcli.py index 5dbecdd60..c08dd0ff3 100644 --- a/buildstream/plugintestutils/runcli.py +++ b/buildstream/plugintestutils/runcli.py @@ -310,6 +310,10 @@ class Cli(): if options is None: options = [] + # We may have been passed e.g. pathlib.Path or py.path + args = [str(x) for x in args] + project = str(project) + options = self.default_options + options with ExitStack() as stack: -- cgit v1.2.1 From da3479322289bc699919ca24e774e21dd0b4a563 Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Thu, 14 Mar 2019 11:59:46 +0000 Subject: _project: don't _find_project_dir if a junction If we're creating a project for a junction, then don't go looking for the project directory - the path has already been given to us. This means that we can now detect when junctions are missing a 'project.conf', and importantly we don't accidentally start resolving elements in the enclosing project. Add tests to cover workspaced, local, and git repo cases. Note that this is also the first test coverage for the INVALID_JUNCTION path. In later work we might extract the _find_project_dir magic out of the Project class, so that there are no surprises when instantiating it. --- buildstream/_loader/loader.py | 2 +- buildstream/_project.py | 10 ++++-- tests/format/junctions.py | 77 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 4 deletions(-) diff --git a/buildstream/_loader/loader.py b/buildstream/_loader/loader.py index f7fd3203e..804dc5f7e 100644 --- a/buildstream/_loader/loader.py +++ b/buildstream/_loader/loader.py @@ -567,7 +567,7 @@ class Loader(): try: from .._project import Project project = Project(project_dir, self._context, junction=element, - parent_loader=self) + parent_loader=self, search_for_project=False) except LoadError as e: if e.reason == LoadErrorReason.MISSING_PROJECT_CONF: message = ( diff --git a/buildstream/_project.py b/buildstream/_project.py index 21ea91683..b6a8727f5 100644 --- a/buildstream/_project.py +++ b/buildstream/_project.py @@ -96,15 +96,19 @@ class ProjectConfig: class Project(): def __init__(self, directory, context, *, junction=None, cli_options=None, - default_mirror=None, parent_loader=None): + default_mirror=None, parent_loader=None, + search_for_project=True): # The project name self.name = None self._context = context # The invocation Context, a private member - # The project directory, and whether the element whose workspace it was invoked from - self.directory, self._invoked_from_workspace_element = self._find_project_dir(directory) + if search_for_project: + self.directory, self._invoked_from_workspace_element = self._find_project_dir(directory) + else: + self.directory = directory + self._invoked_from_workspace_element = None # Absolute path to where elements are loaded from within the project self.element_path = None diff --git a/tests/format/junctions.py b/tests/format/junctions.py index 5c6ebd0bd..ab369b906 100644 --- a/tests/format/junctions.py +++ b/tests/format/junctions.py @@ -48,6 +48,57 @@ def test_simple_build(cli, tmpdir, datafiles): assert(os.path.exists(os.path.join(checkoutdir, 'foo.txt'))) +@pytest.mark.datafiles(DATA_DIR) +def test_junction_missing_project_conf(cli, datafiles): + project = datafiles / 'foo' + copy_subprojects(project, datafiles, ['base']) + + # TODO: see if datafiles can tidy this concat up + + # py3.5 requires this str conversion. + os.remove(str(project / 'base' / 'project.conf')) + + # Note that both 'foo' and 'base' projects have a 'target.bst'. The + # 'app.bst' in 'foo' depends on the 'target.bst' in 'base', i.e.: + # + # foo/base/target.bst + # foo/app.bst -> foo/base/target.bst + # foo/target.bst -> foo/app.bst, foor/base/target.bst + # + # In a previous bug (issue #960) if the 'project.conf' was not found in the + # junction's dir then we were continuing the search in the parent dirs. + # + # This would mean that the dep on 'target.bst' would resolve to + # 'foo/target.bst' instead of 'foo/base/target.bst'. + # + # That would lead to a 'circular dependency error' in this setup, when we + # expect an 'invalid junction'. + # + result = cli.run(project=project, args=['build', 'app.bst']) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION) + + +@pytest.mark.datafiles(DATA_DIR) +def test_workspaced_junction_missing_project_conf(cli, datafiles): + # See test_junction_missing_project_conf for some more background. + + project = datafiles / 'foo' + workspace_dir = project / 'base_workspace' + copy_subprojects(project, datafiles, ['base']) + + result = cli.run( + project=project, + args=['workspace', 'open', 'base.bst', '--directory', workspace_dir]) + print(result) + result.assert_success() + + # py3.5 requires this str conversion. + os.remove(str(workspace_dir / 'project.conf')) + + result = cli.run(project=project, args=['build', 'app.bst']) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION) + + @pytest.mark.datafiles(DATA_DIR) def test_build_of_same_junction_used_twice(cli, datafiles): project = os.path.join(str(datafiles), 'inconsistent-names') @@ -300,6 +351,32 @@ def test_git_build(cli, tmpdir, datafiles): assert(os.path.exists(os.path.join(checkoutdir, 'foo.txt'))) +@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available") +@pytest.mark.datafiles(DATA_DIR) +def test_git_missing_project_conf(cli, tmpdir, datafiles): + project = datafiles / 'foo' + + # See test_junction_missing_project_conf for some more background. + # py3.5 requires this str conversion. + os.remove(str(datafiles / 'base' / 'project.conf')) + + # Create the repo from 'base' subdir + repo = create_repo('git', str(tmpdir)) + ref = repo.create(os.path.join(str(datafiles), 'base')) + + # Write out junction element with git source + element = { + 'kind': 'junction', + 'sources': [ + repo.source_config(ref=ref) + ] + } + _yaml.dump(element, str(project / 'base.bst')) + + result = cli.run(project=project, args=['build', 'app.bst']) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION) + + @pytest.mark.datafiles(DATA_DIR) def test_cross_junction_names(cli, datafiles): project = os.path.join(str(datafiles), 'foo') -- cgit v1.2.1