[GitHub] [buildstream] cs-shadow commented on a change in pull request #1433: Allow pulling build scope of artifacts which are not cached locally

[GitBox <gi...@apache.org>]

cs-shadow commented on a change in pull request #1433:
URL: https://github.com/apache/buildstream/pull/1433#discussion_r562199368



##########
File path: src/buildstream/_stream.py
##########
@@ -1513,6 +1513,24 @@ def _resolve_elements(self, targets):
                 if task:
                     task.add_current_progress()
 
+    # _reset()
+    #
+    # Resets the internal state related to a given scheduler run.
+    #
+    # Invocations to the scheduler should start with a _reset() and end
+    # with _run() like so:
+    #
+    #  self._reset()
+    #  self._add_queue(...)
+    #  self._add_queue(...)
+    #  self._enqueue_plan(...)
+    #  self._run()

Review comment:
       minor: this seems like an important piece of comment, I wonder if it can be "elevated" somewhere higher up. Help for `_reset()` may be a bit too deep.

##########
File path: src/buildstream/element.py
##########
@@ -816,6 +819,7 @@ def get_variable(self, varname: str) -> Optional[str]:
            variable was declared with the given name.
         """
         # Flat is not recognized correctly by Pylint as being a dictionary

Review comment:
       is this comment still relevant?
   
   PS: i'm actually not sure what "Flat" even means here.

##########
File path: tests/frontend/artifact_checkout.py
##########
@@ -0,0 +1,79 @@
+# Pylint doesn't play well with fixtures and dependency injection from pytest
+# pylint: disable=redefined-outer-name
+
+import os
+import shutil
+
+import pytest
+
+from buildstream.testing import cli  # pylint: disable=unused-import
+from buildstream.exceptions import ErrorDomain
+
+from tests.testutils import create_artifact_share
+
+
+DATA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), "project")
+
+#
+# Test modes of `bst artifact checkout --pull` when given an artifact name
+#
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.parametrize(
+    "deps,expect_exist,expect_noexist",
+    [
+        # Deps none: We only expect the file from target-import.bst
+        ("none", ["foo"], ["usr/bin/hello", "usr/include/pony.h"]),
+        # Deps build: We only expect the files from the build dependencies
+        ("build", ["usr/bin/hello", "usr/include/pony.h"], ["foo"]),
+        # Deps run: not supported without a local project
+        ("run", [], []),
+        # Deps all: not supported without a local project
+        ("all", [], []),
+    ],
+    ids=["none", "build", "run", "all"],
+)
+def test_checkout(cli, tmpdir, datafiles, deps, expect_exist, expect_noexist):
+    project = str(datafiles)
+    checkout = os.path.join(cli.directory, "checkout")
+
+    with create_artifact_share(os.path.join(str(tmpdir), "artifactshare")) as share:
+        # Build the element to push it to cache
+        cli.configure({"artifacts": {"url": share.repo, "push": True}})
+
+        # Build it
+        result = cli.run(project=project, args=["build", "target-import.bst"])
+        result.assert_success()
+
+        # Assert it is cached locally and remotely
+        assert cli.get_element_state(project, "target-import.bst") == "cached"
+        assert share.get_artifact(cli.get_artifact_name(project, "test", "target-import.bst"))
+
+        # Obtain the artifact name for pulling purposes
+        artifact_name = cli.get_artifact_name(project, "test", "target-import.bst")
+
+        # Discard the local cache
+        shutil.rmtree(str(os.path.join(str(tmpdir), "cache", "cas")))
+        shutil.rmtree(str(os.path.join(str(tmpdir), "cache", "artifacts")))
+        assert cli.get_element_state(project, "target-import.bst") != "cached"
+
+        # Now checkout the artifacy

Review comment:
       ```suggestion
           # Now checkout the artifact
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org