[Notes] [Git][BuildStream/buildstream][gokcennurlu/remote_url_override

Gökçen Nurlu pushed to branch gokcennurlu/remote_url_override_push_error at BuildStream / buildstream

Commits:

6fcfa452

by Jonathan Maw at 2018-12-03T13:21:05Z

_yamlcache.py: Use a project's junction name if present

This is required because if there are projects with the same name
loaded, that have elements in the same path, with the same contents,
they are considered the same, despite actually belonging to different
Projects.

This fixes buildstream issue #795

d0f9c724

by James Ennis at 2018-12-03T13:21:05Z

tests/loader/junctions.py: Test that we can build a junction that is referred differently, twice.

Test required to close #795 - We should be able to build a project which refers
to the same junction twice, but with two different filenames.

f7581026

by Jürg Billeter at 2018-12-03T16:27:03Z

Merge branch 'jonathan/fix-identical-element' into 'master'

_yamlcache.py: Use a project's junction name if present

Closes #795

See merge request BuildStream/buildstream!980

f859b537

by Gökçen Nurlu at 2018-12-03T18:27:48Z

Set ArtifactCache for push/pull correctly when `--remote` is used

This change makes `ArtifactCache.setup_remotes()` use a boolean parameter, set
by `Stream.push()` or `Stream.pull()`, to configure the overriden `remote_url`
correctly, instead of for `push` by default.

This fixes #798.

ea575b40

by Gökçen Nurlu at 2018-12-03T18:27:48Z

Fix type of artifact_remote_url in _load() docs

ec0468ed

by Gökçen Nurlu at 2018-12-03T18:27:48Z

Refactor setup_remotes() to remove --remote overriding logic

This change simplifies the responsibilities of `setup_remotes()` by extracting
"getting the default remotes" logic to its own function and moving "--remote"
related logic to _load(). The latter, especially, helps to remove the need of
passing the "remote_url" string and "can_push?" boolean to `setup_remotes()`.

22 changed files:

buildstream/_artifactcache/artifactcache.py
buildstream/_stream.py
buildstream/_yamlcache.py
tests/artifactcache/pull.py
tests/artifactcache/push.py
tests/loader/junctions.py
+ tests/loader/junctions/inconsistent-names/elements/junction-A.bst
+ tests/loader/junctions/inconsistent-names/elements/junction-B-diff-name.bst
+ tests/loader/junctions/inconsistent-names/elements/target.bst
+ tests/loader/junctions/inconsistent-names/files/foo
+ tests/loader/junctions/inconsistent-names/junctionA/elements/app.bst
+ tests/loader/junctions/inconsistent-names/junctionA/elements/junction-B.bst
+ tests/loader/junctions/inconsistent-names/junctionA/elements/lib.bst
+ tests/loader/junctions/inconsistent-names/junctionA/files/app
+ tests/loader/junctions/inconsistent-names/junctionA/files/lib
+ tests/loader/junctions/inconsistent-names/junctionA/junctionB/base/baseimg
+ tests/loader/junctions/inconsistent-names/junctionA/junctionB/elements/base.bst
+ tests/loader/junctions/inconsistent-names/junctionA/junctionB/elements/lib2.bst
+ tests/loader/junctions/inconsistent-names/junctionA/junctionB/files/lib2
+ tests/loader/junctions/inconsistent-names/junctionA/junctionB/project.conf
+ tests/loader/junctions/inconsistent-names/junctionA/project.conf
+ tests/loader/junctions/inconsistent-names/project.conf

Changes:

buildstream/_artifactcache/artifactcache.py

@@ -110,36 +110,42 @@ class ArtifactCache():
          # assume project and element names are not allowed to contain slashes
          return '{0}/{1}/{2}'.format(project.name, element_name, key)
 +    # get_remotes_from_projects()
 +    #
 +    # Generates list artifact caches based on project configuration
 +    #
 +    # Returns:
 +    #    (list of (list of ArtifactCacheSpec, Project)): Configurations each are
 +    #        ready to be consumed by `self._set_remotes()`
 +    #
 +    # This requires that all of the projects which are to be processed in the session
 +    # have already been loaded and are observable in the Context.
 +    #
 +    def get_remotes_from_projects(self):
 +        return [
 +            (_configured_remote_artifact_cache_specs(self.context, prj), prj)
 +            for prj in self.context.get_projects()
 +        ]
++
      # setup_remotes():
+     #
      # Sets up which remotes to use
+     #
      # Args:
 -    #    use_config (bool): Whether to use project configuration
 -    #    remote_url (str): Remote artifact cache URL
 +    #    remotes (list of (list of ArtifactCacheSpec, Project)): Configurations each are
 +    #        ready to be consumed by `self._set_remotes()`
+     #
      # This requires that all of the projects which are to be processed in the session
      # have already been loaded and are observable in the Context.
+     #
 -    def setup_remotes(self, *, use_config=False, remote_url=None):
+-
 +    def setup_remotes(self, *, remotes):
          # Ensure we do not double-initialise since this can be expensive
          assert not self._remotes_setup
          self._remotes_setup = True
 -        # Initialize remote artifact caches. We allow the commandline to override
 -        # the user config in some cases (for example `bst push --remote=...`).
 -        has_remote_caches = False
 -        if remote_url:
 -            self._set_remotes([ArtifactCacheSpec(remote_url, push=True)])
 -            has_remote_caches = True
 -        if use_config:
 -            for project in self.context.get_projects():
 -                artifact_caches = _configured_remote_artifact_cache_specs(self.context, project)
 -                if artifact_caches:  # artifact_caches is a list of ArtifactCacheSpec instances
 -                    self._set_remotes(artifact_caches, project=project)
 -                    has_remote_caches = True
 -        if has_remote_caches:
 +        if remotes:
 +            for caches, project in remotes:
 +                self._set_remotes(caches, project=project)
              self._initialize_remotes()
      # specs_from_config_node()

buildstream/_stream.py

@@ -28,6 +28,7 @@ import tarfile
  from contextlib import contextmanager
  from tempfile import TemporaryDirectory
 +from ._artifactcache import ArtifactCacheSpec
  from ._exceptions import StreamError, ImplError, BstError, set_last_task_error
  from ._message import Message, MessageType
  from ._scheduler import Scheduler, SchedStatus, TrackQueue, FetchQueue, BuildQueue, PullQueue, PushQueue
@@ -305,6 +306,7 @@ class Stream():
                                   selection=selection,
                                   use_artifact_config=use_config,
                                   artifact_remote_url=remote,
 +                                 artifact_remote_can_push=False,
                                   fetch_subprojects=True)
          if not self._artifacts.has_fetch_remotes():
@@ -343,6 +345,7 @@ class Stream():
                                   selection=selection,
                                   use_artifact_config=use_config,
                                   artifact_remote_url=remote,
 +                                 artifact_remote_can_push=True,
                                   fetch_subprojects=True)
          if not self._artifacts.has_push_remotes():
@@ -922,7 +925,8 @@ class Stream():
      #    track_except_targets (list of str): Specified targets to except from fetching
      #    track_cross_junctions (bool): Whether tracking should cross junction boundaries
      #    use_artifact_config (bool): Whether to initialize artifacts with the config
 -    #    artifact_remote_url (bool): A remote url for initializing the artifacts
 +    #    artifact_remote_url (str): A remote url for initializing the artifacts
 +    #    artifact_remote_can_push (bool): Whether `artifact_remote_url` can be used to push
      #    fetch_subprojects (bool): Whether to fetch subprojects while loading
+     #
      # Returns:
@@ -937,6 +941,7 @@ class Stream():
                track_cross_junctions=False,
                use_artifact_config=False,
                artifact_remote_url=None,
 +              artifact_remote_can_push=False,
                fetch_subprojects=False,
                dynamic_plan=False):
@@ -1000,12 +1005,20 @@ class Stream():
              self._pipeline.resolve_elements(track_selected)
              return [], track_selected
 -        # ArtifactCache.setup_remotes expects all projects to be fully loaded
 -        for project in self._context.get_projects():
 -            project.ensure_fully_loaded()
+-
 +        if use_artifact_config:
 +            # ArtifactCache.get_remotes_from_projects expects all projects to be
 +            # fully loaded
 +            for project in self._context.get_projects():
 +                project.ensure_fully_loaded()
 +            remotes = self._artifacts.get_remotes_from_projects()
 +        else:
 +            # Build the ArtifactCacheSpec instance based on `--remote`
 +            remotes = [(
 +                [ArtifactCacheSpec(artifact_remote_url, push=artifact_remote_can_push)],
 +                None
 +            )]
          # Connect to remote caches, this needs to be done before resolving element state
 -        self._artifacts.setup_remotes(use_config=use_artifact_config, remote_url=artifact_remote_url)
 +        self._artifacts.setup_remotes(remotes=remotes)
          # Now move on to loading primary selection.
+         #

buildstream/_yamlcache.py

@@ -68,7 +68,7 @@ class YamlCache():
      #    (bool): Whether the file is cached.
      def is_cached(self, project, filepath):
          cache_path = self._get_filepath(project, filepath)
 -        project_name = project.name if project else ""
 +        project_name = self.get_project_name(project)
          try:
              project_cache = self._project_caches[project_name]
              if cache_path in project_cache.elements:
@@ -167,7 +167,7 @@ class YamlCache():
      #    value (decorated dict): The data to put into the cache.
      def put_from_key(self, project, filepath, key, value):
          cache_path = self._get_filepath(project, filepath)
 -        project_name = project.name if project else ""
 +        project_name = self.get_project_name(project)
          try:
              project_cache = self._project_caches[project_name]
          except KeyError:
@@ -237,7 +237,7 @@ class YamlCache():
      #    (decorated dict): The parsed yaml from the cache, or None if the file isn't in the cache.
      def _get(self, project, filepath, key):
          cache_path = self._get_filepath(project, filepath)
 -        project_name = project.name if project else ""
 +        project_name = self.get_project_name(project)
          try:
              project_cache = self._project_caches[project_name]
              try:
@@ -253,6 +253,30 @@ class YamlCache():
              pass
          return None
 +    # get_project_name():
 +    #
 +    # Gets a name appropriate for Project. Projects must use their junction's
 +    # name if present, otherwise elements with the same contents under the
 +    # same path with identically-named projects are considered the same yaml
 +    # object, despite existing in different Projects.
 +    #
 +    # Args:
 +    #    project (Project): The project this file is in, or None.
 +    #
 +    # Returns:
 +    #    (str): The project's junction's name if present, the project's name,
 +    #           or an empty string if there is no project
 +    @staticmethod
 +    def get_project_name(project):
 +        if project:
 +            if project.junction:
 +                project_name = project.junction.name
 +            else:
 +                project_name = project.name
 +        else:
 +            project_name = ""
 +        return project_name
++
  CachedProject = namedtuple('CachedProject', ['elements'])
@@ -287,7 +311,7 @@ class BstPickler(pickle.Pickler):
          if isinstance(obj, _yaml.ProvenanceFile):
              if obj.project:
                  # ProvenanceFile's project object cannot be stored as it is.
 -                project_tag = obj.project.name
 +                project_tag = YamlCache.get_project_name(obj.project)
                  # ProvenanceFile's filename must be stored relative to the
                  # project, as the project dir may move.
                  name = os.path.relpath(obj.name, obj.project.directory)
@@ -319,14 +343,14 @@ class BstUnpickler(pickle.Unpickler):
              if project_tag is not None:
                  for p in self._context.get_projects():
 -                    if project_tag == p.name:
 +                    if YamlCache.get_project_name(p) == project_tag:
                          project = p
                          break
                  name = os.path.join(project.directory, tagged_name)
                  if not project:
 -                    projects = [p.name for p in self._context.get_projects()]
 +                    projects = [YamlCache.get_project_name(p) for p in self._context.get_projects()]
                      raise pickle.UnpicklingError("No project with name {} found in {}"
                                                   .format(project_tag, projects))
              else:

tests/artifactcache/pull.py

@@ -146,7 +146,8 @@ def _test_pull(user_config_file, project_dir, artifact_dir,
      element = project.load_elements([element_name])[0]
      # Manually setup the CAS remote
 -    cas.setup_remotes(use_config=True)
 +    remotes = cas.get_remotes_from_projects()
 +    cas.setup_remotes(remotes=remotes)
      if cas.has_push_remotes(element=element):
          # Push the element's artifact
@@ -284,7 +285,8 @@ def _test_push_tree(user_config_file, project_dir, artifact_dir, artifact_digest
      cas = artifactcache.cas
      # Manually setup the CAS remote
 -    artifactcache.setup_remotes(use_config=True)
 +    remotes = artifactcache.get_remotes_from_projects()
 +    artifactcache.setup_remotes(remotes=remotes)
      if artifactcache.has_push_remotes():
          directory = remote_execution_pb2.Directory()
@@ -319,7 +321,8 @@ def _test_pull_tree(user_config_file, project_dir, artifact_dir, artifact_digest
      cas = context.artifactcache
      # Manually setup the CAS remote
 -    cas.setup_remotes(use_config=True)
 +    remotes = cas.get_remotes_from_projects()
 +    cas.setup_remotes(remotes=remotes)
      if cas.has_push_remotes():
          # Pull the artifact using the Tree object

tests/artifactcache/push.py

@@ -125,8 +125,8 @@ def _test_push(user_config_file, project_dir, artifact_dir,
      element = project.load_elements([element_name])[0]
      # Manually setup the CAS remote
 -    cas.setup_remotes(use_config=True)
 -    cas.initialize_remotes()
 +    remotes = cas.get_remotes_from_projects()
 +    cas.setup_remotes(remotes=remotes)
      if cas.has_push_remotes(element=element):
          # Push the element's artifact
@@ -185,8 +185,8 @@ def test_push_directory(cli, tmpdir, datafiles):
          assert artifactcache.contains(element, element_key)
          # Manually setup the CAS remote
 -        artifactcache.setup_remotes(use_config=True)
 -        artifactcache.initialize_remotes()
 +        remotes = artifactcache.get_remotes_from_projects()
 +        artifactcache.setup_remotes(remotes=remotes)
          assert artifactcache.has_push_remotes(element=element)
          # Recreate the CasBasedDirectory object from the cached artifact
@@ -231,8 +231,8 @@ def _test_push_directory(user_config_file, project_dir, artifact_dir, artifact_d
      cas = context.artifactcache
      # Manually setup the CAS remote
 -    cas.setup_remotes(use_config=True)
 -    cas.initialize_remotes()
 +    remotes = cas.get_remotes_from_projects()
 +    cas.setup_remotes(remotes=remotes)
      if cas.has_push_remotes():
          # Create a CasBasedDirectory from local CAS cache content
@@ -307,8 +307,8 @@ def _test_push_message(user_config_file, project_dir, artifact_dir, queue):
      cas = context.artifactcache
      # Manually setup the CAS remote
 -    cas.setup_remotes(use_config=True)
 -    cas.initialize_remotes()
 +    remotes = cas.get_remotes_from_projects()
 +    cas.setup_remotes(remotes=remotes)
      if cas.has_push_remotes():
          # Create an example message object

tests/loader/junctions.py

@@ -47,6 +47,16 @@ def test_simple_build(cli, tmpdir, datafiles):
      assert(os.path.exists(os.path.join(checkoutdir, 'foo.txt')))
 +@pytest.mark.datafiles(DATA_DIR)
 +def test_build_of_same_junction_used_twice(cli, tmpdir, datafiles):
 +    project = os.path.join(str(datafiles), 'inconsistent-names')
++
 +    # Check we can build a project that contains the same junction
 +    # that is used twice, but named differently
 +    result = cli.run(project=project, args=['build', 'target.bst'])
 +    assert result.exit_code == 0
++
++
  @pytest.mark.datafiles(DATA_DIR)
  def test_nested_simple(cli, tmpdir, datafiles):
      foo = os.path.join(str(datafiles), 'foo')

tests/loader/junctions/inconsistent-names/elements/junction-A.bst

 +kind: junction
 +sources:
 +- kind: local
 +  path: junctionA

tests/loader/junctions/inconsistent-names/elements/junction-B-diff-name.bst

 +kind: junction
 +sources:
 +- kind: local
 +  path: junctionA/junctionB

tests/loader/junctions/inconsistent-names/elements/target.bst

 +kind: import
 +sources:
 +- kind: local
 +  path: files/foo
 +depends:
 +- filename: lib2.bst
 +  junction: junction-B-diff-name.bst
 +- filename: lib.bst
 +  junction: junction-A.bst