abderrahim commented on code in PR #2129:
URL: https://github.com/apache/buildstream/pull/2129#discussion_r3497414176
##########
src/buildstream/_stream.py:
##########
@@ -916,6 +901,74 @@ def artifact_delete(self, targets, *,
selection=_PipelineSelection.NONE):
if not ref_removed:
self._context.messenger.info("No artifacts were removed")
+
+ # buildtree_checkout()
+ #
+ # Checkout target buildtree artifact to the specified location
+ #
+ # Args:
+ # target: Target to checkout
+ # location: Location to checkout the artifact to
+ # force: Whether files can be overwritten if necessary
+ # hardlinks: Whether checking out files hardlinked to
+ # their artifacts is acceptable
+ # tar: If true, a tarball from the artifact contents will
+ # be created, otherwise the file tree of the artifact
+ # will be placed at the given location. If true and
+ # location is '-', the tarball will be dumped on the
+ # standard output.
+ # artifact_remotes: Artifact cache remotes specified on the commmand
line
+ # ignore_project_artifact_remotes: Whether to ignore artifact remotes
specified by projects
+ #
+ def buildtree_checkout(
+ self,
+ target: str,
+ *,
+ location: Optional[str] = None,
+ buildroot: bool = False,
+ force: bool = False,
+ hardlinks: bool = False,
+ compression: str = "",
+ tar: bool = False,
+ artifact_remotes: Iterable[RemoteSpec] = (),
+ ignore_project_artifact_remotes: bool = False,
+ ):
+
+ elements = self._load(
+ (target,),
+ selection=_PipelineSelection.NONE,
+ load_artifacts=True,
+ attempt_artifact_metadata=True,
+ connect_artifact_cache=True,
+ artifact_remotes=artifact_remotes,
+ ignore_project_artifact_remotes=ignore_project_artifact_remotes,
+ )
+
+ # self.targets contains a list of the loaded target objects
+ # if we specify --deps build, Stream._load() will return a list
+ # of build dependency objects, however, we need to prepare a sandbox
+ # with the target (which has had its appropriate dependencies loaded)
+ element: Element = self.targets[0]
+
+ self._check_location_writable(location, force=force, tar=tar)
+
+ # Check whether the required elements are cached, and then
+ # try to pull them if they are not already cached.
+ #
+ self.query_cache(elements)
+ self._pull_missing_artifacts(elements)
Review Comment:
I think we only need to pull the artifact for the element in question. No
need for dependencies here.
as an aside, I wonder if we should try to override the `pull-buildtrees`
configuration in this case: if we're pulling to extract a build tree, we should
probably try to pull the buildtree even if we usually don't.
##########
src/buildstream/_stream.py:
##########
@@ -916,6 +901,74 @@ def artifact_delete(self, targets, *,
selection=_PipelineSelection.NONE):
if not ref_removed:
self._context.messenger.info("No artifacts were removed")
+
+ # buildtree_checkout()
+ #
+ # Checkout target buildtree artifact to the specified location
+ #
+ # Args:
+ # target: Target to checkout
+ # location: Location to checkout the artifact to
+ # force: Whether files can be overwritten if necessary
+ # hardlinks: Whether checking out files hardlinked to
+ # their artifacts is acceptable
+ # tar: If true, a tarball from the artifact contents will
+ # be created, otherwise the file tree of the artifact
+ # will be placed at the given location. If true and
+ # location is '-', the tarball will be dumped on the
+ # standard output.
+ # artifact_remotes: Artifact cache remotes specified on the commmand
line
+ # ignore_project_artifact_remotes: Whether to ignore artifact remotes
specified by projects
+ #
+ def buildtree_checkout(
+ self,
+ target: str,
+ *,
+ location: Optional[str] = None,
+ buildroot: bool = False,
+ force: bool = False,
+ hardlinks: bool = False,
+ compression: str = "",
+ tar: bool = False,
+ artifact_remotes: Iterable[RemoteSpec] = (),
+ ignore_project_artifact_remotes: bool = False,
+ ):
+
+ elements = self._load(
+ (target,),
+ selection=_PipelineSelection.NONE,
+ load_artifacts=True,
+ attempt_artifact_metadata=True,
+ connect_artifact_cache=True,
+ artifact_remotes=artifact_remotes,
+ ignore_project_artifact_remotes=ignore_project_artifact_remotes,
+ )
+
+ # self.targets contains a list of the loaded target objects
+ # if we specify --deps build, Stream._load() will return a list
+ # of build dependency objects, however, we need to prepare a sandbox
+ # with the target (which has had its appropriate dependencies loaded)
+ element: Element = self.targets[0]
Review Comment:
Does this make sense for the build tree? Either we're checking out just the
buildtree, and then dependencies don't make sense, or we check out the whole
buildroot and it already contains the build dependencies.
Am I missing something?
##########
src/buildstream/_stream.py:
##########
@@ -916,6 +901,74 @@ def artifact_delete(self, targets, *,
selection=_PipelineSelection.NONE):
if not ref_removed:
self._context.messenger.info("No artifacts were removed")
+
+ # buildtree_checkout()
+ #
+ # Checkout target buildtree artifact to the specified location
+ #
+ # Args:
+ # target: Target to checkout
+ # location: Location to checkout the artifact to
+ # force: Whether files can be overwritten if necessary
+ # hardlinks: Whether checking out files hardlinked to
+ # their artifacts is acceptable
+ # tar: If true, a tarball from the artifact contents will
+ # be created, otherwise the file tree of the artifact
+ # will be placed at the given location. If true and
+ # location is '-', the tarball will be dumped on the
+ # standard output.
+ # artifact_remotes: Artifact cache remotes specified on the commmand
line
+ # ignore_project_artifact_remotes: Whether to ignore artifact remotes
specified by projects
+ #
+ def buildtree_checkout(
+ self,
+ target: str,
+ *,
+ location: Optional[str] = None,
+ buildroot: bool = False,
+ force: bool = False,
+ hardlinks: bool = False,
+ compression: str = "",
+ tar: bool = False,
+ artifact_remotes: Iterable[RemoteSpec] = (),
+ ignore_project_artifact_remotes: bool = False,
+ ):
+
+ elements = self._load(
+ (target,),
+ selection=_PipelineSelection.NONE,
+ load_artifacts=True,
+ attempt_artifact_metadata=True,
+ connect_artifact_cache=True,
+ artifact_remotes=artifact_remotes,
+ ignore_project_artifact_remotes=ignore_project_artifact_remotes,
+ )
+
+ # self.targets contains a list of the loaded target objects
+ # if we specify --deps build, Stream._load() will return a list
+ # of build dependency objects, however, we need to prepare a sandbox
+ # with the target (which has had its appropriate dependencies loaded)
+ element: Element = self.targets[0]
+
+ self._check_location_writable(location, force=force, tar=tar)
+
+ # Check whether the required elements are cached, and then
+ # try to pull them if they are not already cached.
+ #
+ self.query_cache(elements)
+ self._pull_missing_artifacts(elements)
+
+ self._check_buildtree(element)
+
+ try:
+ artifact = element._get_artifact()
+ virdir = artifact.get_buildroot() if buildroot else
artifact.get_buildtree()
Review Comment:
Another thing we need to be aware of (and also give a suggestion about to
the user) is that not all elements have a build tree: only build elements do.
So it's not enough to check for the build root, we also need to check for
the build tree. Then depending on the availability and the user request, we can
suggest an alternative to the user.
| buildtree available | buildroot available | user requested | result |
| --- | --- | --- | --- |
| Y | Y | build tree | extract build tree |
| Y | Y | build root | extract build root |
| N | Y | build tree | error + suggest extract build root |
| N | Y | build root | extract build root |
| N | N | build tree | error |
| N | N | build root | error |
##########
tests/integration/buildtree.py:
##########
@@ -0,0 +1,133 @@
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# Pylint doesn't play well with fixtures and dependency injection from pytest
+# pylint: disable=redefined-outer-name
+
+import os
+import tarfile
+import shutil
+
+import pytest
+
+from buildstream._testing import cli, cli_integration, Cli # pylint:
disable=unused-import
+from buildstream.exceptions import ErrorDomain
+from buildstream._testing._utils.site import HAVE_SANDBOX
+
+pytestmark = pytest.mark.integration
+
+
+DATA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), "project")
+
+#
+# Verify fail cases when checkout buildtree
+#
[email protected](DATA_DIR)
[email protected](not HAVE_SANDBOX, reason="Only available with a
functioning sandbox")
+def test_buildtree_checkout_fail(cli, datafiles):
+ project = str(datafiles)
+ element_name = "build-shell/buildtree.bst"
+ checkout = os.path.join(cli.directory, "checkout")
+ tar = os.path.join(cli.directory, "source-checkout.tar")
Review Comment:
```suggestion
tar = os.path.join(cli.directory, "buildtree.tar")
```
##########
tests/integration/buildtree.py:
##########
@@ -0,0 +1,133 @@
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# Pylint doesn't play well with fixtures and dependency injection from pytest
+# pylint: disable=redefined-outer-name
+
+import os
+import tarfile
+import shutil
+
+import pytest
+
+from buildstream._testing import cli, cli_integration, Cli # pylint:
disable=unused-import
+from buildstream.exceptions import ErrorDomain
+from buildstream._testing._utils.site import HAVE_SANDBOX
+
+pytestmark = pytest.mark.integration
+
+
+DATA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), "project")
+
+#
+# Verify fail cases when checkout buildtree
+#
[email protected](DATA_DIR)
[email protected](not HAVE_SANDBOX, reason="Only available with a
functioning sandbox")
+def test_buildtree_checkout_fail(cli, datafiles):
+ project = str(datafiles)
+ element_name = "build-shell/buildtree.bst"
Review Comment:
We should also test the buildtree of a script element (using
`build-shell/script.bst`). This is the corner case I mentioned above where we
have a build root but not a build tree.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
