gtristan commented on a change in pull request #1433:
URL: https://github.com/apache/buildstream/pull/1433#discussion_r562392827
##########
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:
This appears to be a hold out from before the `Variables` refactor.
I removed the comment and also removed the following `# pylint` comment
which disables a linter check, the linting now works without this comment too.
##########
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
+#
[email protected](DATA_DIR)
[email protected](
+ "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:
Fixed
##########
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:
This one I think I'll leave this way at the moment, as there isn't
really any "higher up" location for this.
The alternatives I can see would be:
* Move the general comment to `_run()`, doesn't really change much
* Setup a big commented "block" section for this portion of the stream's
internal API, I don't feel the code would benefit from further artificial
sectioning (when files get big, we usually section by public/private/internal
portions already).
To make this clearer, it would probably be best to add a higher level API to
the `Scheduler` (or perhaps a `Scheduler` abstraction API depending on what is
found to be more appropriate), and remove some visibility of the `Scheduler`
API that might not be needed by `Stream`, i.e. we could migrate this
`run()`/`reset()`/`add_queue()`/`enqueue_plan()` directly to the `Scheduler` -
this would require some additional thinking and I think is out of scope for
this PR.
----------------------------------------------------------------
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:
[email protected]
