You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by "nathanwilliams-ct (via GitHub)" <gi...@apache.org> on 2026/07/06 13:35:05 UTC
[PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
nathanwilliams-ct opened a new pull request, #2147:
URL: https://github.com/apache/buildstream/pull/2147
This enables users to add additional functionality such as debug tooling in the shell sandbox,
without needing to modify the target element. This is achived through introducing a new option to
shell.
All existing API and UX is maintained, to not break existing scripts. An alternative design was
considered, to have the additional elements as positional arguments similar to the existing element,
but this would need manual parsing to handle the cases where `--` is present and not
present, splitting based on a `.bst` suffix. This UX could be re-visited in future.
Example usage:
bst shell --with base.bst example.bst -- cat example.txt
Where:
- example.bst is a simple import element with no dependencies that imports a file called example.txt
- base.bst provides a basic alpine sysroot with a standard set of unix tooling (sh, df, cat etc).
Changes:
- Introduces `test_with_other_targets` integration test to the shell test suite.
- Adds `--with` cli option to the shell subcommand and updates it's documentation.
- option can be used multiple times by caller, providing a list of targets.
- Extends the shell top level calling interface in Buildstream core to accept a list of
other_targets
- This is where the targets are loaded into elements and checked to make sure they are present
- Extends the shell element implementation to accept a list of other targets
- This is where the other elements are staged and integrated into the sandbox
towards: #422+s
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-4936422649
Thanks, whoever triggered the CI... looks like it needs a little work, despite the tests working locally in the tox environment and I missed the linters.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3803200104
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
```
$ tox -e venv -- bst --directory tests/integration/project shell manual/import-file.bst
[--:--:--][ ][ main:core activity ] START Loading elements
[00:00:00][ ][ main:core activity ] SUCCESS Loading elements
[--:--:--][ ][ main:core activity ] START Resolving elements
[00:00:00][ ][ main:core activity ] SUCCESS Resolving elements
[--:--:--][ ][ main:core activity ] START Initializing remote caches
[00:00:00][ ][ main:core activity ] SUCCESS Initializing remote caches
[--:--:--][ ][ main:core activity ] START Query cache
[00:00:00][ ][ main:core activity ] SUCCESS Query cache
[--:--:--][dc2422ef][ main:manual/import-file.bst ] START Staging dependencies
[00:00:00][dc2422ef][ main:manual/import-file.bst ] SUCCESS Staging dependencies
[--:--:--][dc2422ef][ main:manual/import-file.bst ] START Integrating sandbox
[00:00:00][dc2422ef][ main:manual/import-file.bst ] SUCCESS Integrating sandbox
[--:--:--][dc2422ef][ main:manual/import-file.bst ] STATUS Running command
sh -i
Error launching shell: Staged artifacts do not provide command 'sh'
$ tox -e venv -- bst --directory tests/integration/project shell manual/import-file.bst --with base/base-alpine.bst
[--:--:--][ ][ main:core activity ] START Loading elements
[00:00:00][ ][ main:core activity ] SUCCESS Loading elements
[--:--:--][ ][ main:core activity ] START Resolving elements
[00:00:00][ ][ main:core activity ] SUCCESS Resolving elements
[--:--:--][ ][ main:core activity ] START Initializing remote caches
[00:00:00][ ][ main:core activity ] SUCCESS Initializing remote caches
[--:--:--][ ][ main:core activity ] START Query cache
[00:00:00][ ][ main:core activity ] SUCCESS Query cache
[--:--:--][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] START test/manual-import-file.bst_tempshwrznrg/e8d846b7-build.20260818-111411.log
[--:--:--][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] START Staging sources
[00:00:00][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Staging sources
[--:--:--][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] START Caching artifact
[00:00:00][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Caching artifact
[00:00:00][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] SUCCESS test/manual-import-file.bst_tempshwrznrg/e8d846b7-build.20260818-111411.log
[--:--:--][ ][ main:core activity ] START Loading elements
[00:00:00][ ][ main:core activity ] SUCCESS Loading elements
[--:--:--][ ][ main:core activity ] START Resolving elements
[00:00:00][ ][ main:core activity ] SUCCESS Resolving elements
[--:--:--][ ][ main:core activity ] START Initializing remote caches
[00:00:00][ ][ main:core activity ] SUCCESS Initializing remote caches
[--:--:--][ ][ main:core activity ] START Query cache
[00:00:00][ ][ main:core activity ] SUCCESS Query cache
[--:--:--][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] START Staging dependencies
[00:00:00][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Staging dependencies
[--:--:--][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] START Integrating sandbox
[00:00:00][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Integrating sandbox
[--:--:--][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] STATUS Running command
sh -i
[e8d846b7@manual/import-file.bst_tempshwrznrg.bst:/]$ cat test.txt
This is a test
[e8d846b7@manual/import-file.bst_tempshwrznrg.bst:/]$ exit
```
hmm, I went down the route of creating a temporary element.
Due to how the element loading works, it makes it almost impossible to inject additional dependencies in at runtime. Temporary stack element would have made buildtree ones unhelpful. Solving the overlap problem and dealing with overlapping sandboxes and integration commands was too complicated and messy.
The downside of using a temporary element is the shell command needs to attempt to run a build on the temporary element, before it can shell, to cache it's build result.
I'll tidy up my prototype with some tests and push it a bit later on.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3603432644
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
I will look to explore these alternative routes, thanks for the feedback.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3803200104
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
```
$ tox -e venv -- bst --directory tests/integration/project shell manual/import-file.bst
[--:--:--][ ][ main:core activity ] START Loading elements
[00:00:00][ ][ main:core activity ] SUCCESS Loading elements
[--:--:--][ ][ main:core activity ] START Resolving elements
[00:00:00][ ][ main:core activity ] SUCCESS Resolving elements
[--:--:--][ ][ main:core activity ] START Initializing remote caches
[00:00:00][ ][ main:core activity ] SUCCESS Initializing remote caches
[--:--:--][ ][ main:core activity ] START Query cache
[00:00:00][ ][ main:core activity ] SUCCESS Query cache
[--:--:--][dc2422ef][ main:manual/import-file.bst ] START Staging dependencies
[00:00:00][dc2422ef][ main:manual/import-file.bst ] SUCCESS Staging dependencies
[--:--:--][dc2422ef][ main:manual/import-file.bst ] START Integrating sandbox
[00:00:00][dc2422ef][ main:manual/import-file.bst ] SUCCESS Integrating sandbox
[--:--:--][dc2422ef][ main:manual/import-file.bst ] STATUS Running command
sh -i
Error launching shell: Staged artifacts do not provide command 'sh'
$ tox -e venv -- bst --directory tests/integration/project shell manual/import-file.bst --with base/base-alpine.bst
[--:--:--][ ][ main:core activity ] START Loading elements
/var/home/nathanwilliams/buildstream/tests/integration/project/elements/manual/import-file.bst_tempshwrznrg.bst
manual/import-file.bst_tempshwrznrg.bst
manual/import-file.bst_tempshwrznrg.bst
[00:00:00][ ][ main:core activity ] SUCCESS Loading elements
[--:--:--][ ][ main:core activity ] START Resolving elements
[00:00:00][ ][ main:core activity ] SUCCESS Resolving elements
[--:--:--][ ][ main:core activity ] START Initializing remote caches
[00:00:00][ ][ main:core activity ] SUCCESS Initializing remote caches
[--:--:--][ ][ main:core activity ] START Query cache
[00:00:00][ ][ main:core activity ] SUCCESS Query cache
[--:--:--][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] START test/manual-import-file.bst_tempshwrznrg/e8d846b7-build.20260818-111411.log
[--:--:--][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] START Staging sources
[00:00:00][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Staging sources
[--:--:--][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] START Caching artifact
[00:00:00][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Caching artifact
[00:00:00][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] SUCCESS test/manual-import-file.bst_tempshwrznrg/e8d846b7-build.20260818-111411.log
[--:--:--][ ][ main:core activity ] START Loading elements
[00:00:00][ ][ main:core activity ] SUCCESS Loading elements
[--:--:--][ ][ main:core activity ] START Resolving elements
[00:00:00][ ][ main:core activity ] SUCCESS Resolving elements
[--:--:--][ ][ main:core activity ] START Initializing remote caches
[00:00:00][ ][ main:core activity ] SUCCESS Initializing remote caches
[--:--:--][ ][ main:core activity ] START Query cache
[00:00:00][ ][ main:core activity ] SUCCESS Query cache
[--:--:--][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] START Staging dependencies
[00:00:00][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Staging dependencies
[--:--:--][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] START Integrating sandbox
[00:00:00][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Integrating sandbox
[--:--:--][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] STATUS Running command
sh -i
[e8d846b7@manual/import-file.bst_tempshwrznrg.bst:/]$ cat test.txt
This is a test
[e8d846b7@manual/import-file.bst_tempshwrznrg.bst:/]$ exit
```
hmm, I went down the route of creating a temporary element.
Due to how the element loading works, it makes it almost impossible to inject additional dependencies in at runtime. Temporary stack element would have made buildtree ones unhelpful. Solving the overlap problem and dealing with overlapping sandboxes and integration commands was too complicated and messy.
The downside of using a temporary element is the shell command needs to attempt to run a build on the temporary element, before it can shell, to cache it's build result.
I'll tidy up my prototype with some tests and push it a bit later on.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3903264934
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
It might be nice to have a 'reload' method on Element that can cleanly reload the element if there are changes at runtime, but it would involve some heavy refactoring that I currently don't have the context to do. Element is heavily implemented around the idea of everything immutable.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r4015026503
##########
src/buildstream/_stream.py:
##########
@@ -235,6 +240,65 @@ def query_cache(self, elements, *, sources_of_cached_elements=False, only_source
task.add_current_progress()
+ # shell_with()
+ #
+ # Run a shell with other targets.
+ #
+ # Automatically creates a temporary target based on 'target' with 'other_targets' as runtime or build dependencies.
+ #
+ # Note: Method will build the temporary target, before entering into it's shell.
+ #
+ # Args:
+ # target (str): The name of the element to run the shell for
+ # other_targets: (Iterable[str]): The name of the other elements to run the shell with.
+ # scope: _Scope: Either BUILD or RUN
+ # *args, **kwargs: Passed to shell() untouched.
+ #
+ # Returns:
+ # (int): The exit code of the launched shell
+ #
+ def shell_with(self, target: str, other_targets: Iterable[str], scope: _Scope, *args, **kwargs):
+
+ assert self._project, "Must have a project"
+ assert self._project.loader, "Project must have loader"
+
+ _, target_name, target_loader = self._project.loader._parse_name(target, MappingNode.from_dict({}))
+
+ target_path = os.path.join(target_loader._basedir, target_name)
+ target_node: CommentedMap = _yaml.roundtrip_load(target_path)
+
+ if scope == _Scope.RUN:
+ r_depends = target_node.get("runtime-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["runtime-depends"] = r_depends
+ elif scope == _Scope.BUILD:
+ r_depends = target_node.get("build-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["build-depends"] = r_depends
+ else:
+ raise StreamError(
+ "Only BUILD and RUN scopes are supported",
+ detail="Use the --build and --use-buildtree options to shell into a build tree",
+ reason="only-build-run-supported",
+ )
+
+ with tempfile.NamedTemporaryFile(
+ delete_on_close=False, prefix=f"{target_name.replace('/','_')}_temp", suffix=".bst"
+ ) as temp_target_file:
+ _yaml.roundtrip_dump(target_node, temp_target_file)
+ temp_target_file.close() # delete_on_close is false so this doesn't remove the file, but delete is True(default) so we delete the file when we leave the context manager.
+
+ target_loader._set_fullpath_overrides(target_name, temp_target_file.name)
+
+ self.build([target])
Review Comment:
Changing the dependencies messes with the cache key so a build is necessary still.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-5001267366
Static type checking moved out into it's own PR: https://github.com/apache/buildstream/pull/2153
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3988789301
##########
src/buildstream/_loader/loader.py:
##########
@@ -255,6 +255,19 @@ def foreach_parent(parent):
# Private Methods #
###########################################
+ # _set_fullpath_overrides()
Review Comment:
This shouldn't be a private method as it's used by `Stream`. `Loader` is not part of the public (plugin) API, so 'public' methods in `Loader` will still only be part of the internal API.
Also, `_set_fullpath_overrides()` sounds to me like it sets the whole dict as it's plural. I might call it `add_fullpath_override()`.
Also see a related comment: https://github.com/apache/buildstream/pull/2147#discussion_r3989808035
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-5635341335
The test seems to be failing on Python 3.10 (Ubuntu 22.04) and Python 3.11 (Debian 12).
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r4015070031
##########
src/buildstream/_stream.py:
##########
@@ -235,6 +240,65 @@ def query_cache(self, elements, *, sources_of_cached_elements=False, only_source
task.add_current_progress()
+ # shell_with()
+ #
+ # Run a shell with other targets.
+ #
+ # Automatically creates a temporary target based on 'target' with 'other_targets' as runtime or build dependencies.
+ #
+ # Note: Method will build the temporary target, before entering into it's shell.
+ #
+ # Args:
+ # target (str): The name of the element to run the shell for
+ # other_targets: (Iterable[str]): The name of the other elements to run the shell with.
+ # scope: _Scope: Either BUILD or RUN
+ # *args, **kwargs: Passed to shell() untouched.
+ #
+ # Returns:
+ # (int): The exit code of the launched shell
+ #
+ def shell_with(self, target: str, other_targets: Iterable[str], scope: _Scope, *args, **kwargs):
+
+ assert self._project, "Must have a project"
+ assert self._project.loader, "Project must have loader"
+
+ _, target_name, target_loader = self._project.loader._parse_name(target, MappingNode.from_dict({}))
+
+ target_path = os.path.join(target_loader._basedir, target_name)
+ target_node: CommentedMap = _yaml.roundtrip_load(target_path)
+
+ if scope == _Scope.RUN:
+ r_depends = target_node.get("runtime-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["runtime-depends"] = r_depends
+ elif scope == _Scope.BUILD:
+ r_depends = target_node.get("build-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["build-depends"] = r_depends
+ else:
+ raise StreamError(
+ "Only BUILD and RUN scopes are supported",
+ detail="Use the --build and --use-buildtree options to shell into a build tree",
+ reason="only-build-run-supported",
+ )
+
+ with tempfile.NamedTemporaryFile(
+ delete_on_close=False, prefix=f"{target_name.replace('/','_')}_temp", suffix=".bst"
+ ) as temp_target_file:
+ _yaml.roundtrip_dump(target_node, temp_target_file)
+ temp_target_file.close() # delete_on_close is false so this doesn't remove the file, but delete is True(default) so we delete the file when we leave the context manager.
+
+ target_loader._set_fullpath_overrides(target_name, temp_target_file.name)
+
+ self.build([target])
Review Comment:
Direct runtime dependencies shouldn't affect the cache key. And for build shells (where we add build dependencies), we shouldn't invoke a build before entering the shell anyway. Or am I misremembering or missing something? Have you confirmed a cache key issue with the latest version of your branch (with the fullpath override)?
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r4016326235
##########
src/buildstream/_frontend/cli.py:
##########
@@ -748,13 +755,38 @@ def shell(
otherwise bst may respond to them instead. e.g.
\b
- bst shell example.bst -- df -h
+ bst shell base.bst -- df -h
Use the --build option to create a temporary sysroot for
building the element instead.
+ Use the --with option to stage the artifacts of other elements
+ into the temporary sysroot to make them available to run e.g.
+
+ \b
+ bst shell --with base.bst example.bst -- cat example.txt
+
If no COMMAND is specified, the default is to attempt
to run an interactive shell.
+
+ # Examples:
+
+ \b
+ # Attempt to run an interactive shell with example.bst
+ bst shell example.bst
+ # Attempt to run an df -h with example.bst
+ bst shell example.bst -- df h
+ # In a workspace directory, attempt to shell into the workspace element
+ bst shell
+ # Attempt to run cat from base.bst to read example.txt from example.bst
+ bst shell --with base.bst example.bst -- cat example.txt
+ # Attempt to run an interactive shell with the sources and all dependencies of example.bst
+ bst shell --build example.bst
+
+ For all examples on this page:
+ - example.bst is a simple import element with no dependencies
+ that imports a file called example.txt
+ - base.bst provides a basic alpine sysroot with a standard set of unix tooling (sh, df, cat etc).
Review Comment:
hmm does CI do something different to calling tox locally? doing something like WARNINGS_FATAL=TRUE ? I don't see this error when running it locally.
It's also a very unhelpful error message, I haven't touched `using_commands.rst`, it should point at `cli.py`....
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3989808035
##########
src/buildstream/_stream.py:
##########
@@ -235,6 +240,65 @@ def query_cache(self, elements, *, sources_of_cached_elements=False, only_source
task.add_current_progress()
+ # shell_with()
+ #
+ # Run a shell with other targets.
+ #
+ # Automatically creates a temporary target based on 'target' with 'other_targets' as runtime or build dependencies.
+ #
+ # Note: Method will build the temporary target, before entering into it's shell.
+ #
+ # Args:
+ # target (str): The name of the element to run the shell for
+ # other_targets: (Iterable[str]): The name of the other elements to run the shell with.
+ # scope: _Scope: Either BUILD or RUN
+ # *args, **kwargs: Passed to shell() untouched.
+ #
+ # Returns:
+ # (int): The exit code of the launched shell
+ #
+ def shell_with(self, target: str, other_targets: Iterable[str], scope: _Scope, *args, **kwargs):
+
+ assert self._project, "Must have a project"
+ assert self._project.loader, "Project must have loader"
+
+ _, target_name, target_loader = self._project.loader._parse_name(target, MappingNode.from_dict({}))
+
+ target_path = os.path.join(target_loader._basedir, target_name)
Review Comment:
We shouldn't be using the private members `_parse_name()` and `_basedir`. They should either be public/internal, or we need some new public/internal Loader API as alternative.
A possible option could be to add a `Loader` context manager method that takes care of `_parse_name()`, `os.path.join()`, ` _yaml.roundtrip_load()`, creating the temporary file, `_yaml.roundtrip_dump()` and `_set_fullpath_overrides()`. Leaving only the actual YAML manipulation logic in this method.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3903235596
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
Temporary stack element for runtime shell would be possible I did try it out, but because build shells don't work with that approach and with the aim to keep maintenance cost down: I didn't want two separate implementations if we can help it.
I tried a few ways to inject dependencies in-memory, but part the problem I found was a lot of dependency loading and resolving work is done at the early stages immediately after loading the yaml and parsing, where it's basically impossible to inject the dependencies in a sensible way. Doing it at a later stage e.g. in the Stream.shell or the Element.execute_shell method and or trying to add a inject_extra_deps method to the Element class, I couldn't get it to work. Partly due to the complexity of the shell construction, especially where it involves the cached buildtrees being used which means the whole shell construction process is skipped. The lack of type hints to actually understand the control flow also doesn’t help(https://github.com/apache/buildstream/pull/2167).
I will have a play with adjusting the tempfile approach to hide it a bit better like you suggest.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3988772086
##########
src/buildstream/_stream.py:
##########
@@ -235,6 +240,65 @@ def query_cache(self, elements, *, sources_of_cached_elements=False, only_source
task.add_current_progress()
+ # shell_with()
+ #
+ # Run a shell with other targets.
+ #
+ # Automatically creates a temporary target based on 'target' with 'other_targets' as runtime or build dependencies.
+ #
+ # Note: Method will build the temporary target, before entering into it's shell.
+ #
+ # Args:
+ # target (str): The name of the element to run the shell for
+ # other_targets: (Iterable[str]): The name of the other elements to run the shell with.
+ # scope: _Scope: Either BUILD or RUN
+ # *args, **kwargs: Passed to shell() untouched.
+ #
+ # Returns:
+ # (int): The exit code of the launched shell
+ #
+ def shell_with(self, target: str, other_targets: Iterable[str], scope: _Scope, *args, **kwargs):
+
+ assert self._project, "Must have a project"
+ assert self._project.loader, "Project must have loader"
+
+ _, target_name, target_loader = self._project.loader._parse_name(target, MappingNode.from_dict({}))
+
+ target_path = os.path.join(target_loader._basedir, target_name)
+ target_node: CommentedMap = _yaml.roundtrip_load(target_path)
+
+ if scope == _Scope.RUN:
+ r_depends = target_node.get("runtime-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["runtime-depends"] = r_depends
+ elif scope == _Scope.BUILD:
+ r_depends = target_node.get("build-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["build-depends"] = r_depends
+ else:
+ raise StreamError(
+ "Only BUILD and RUN scopes are supported",
+ detail="Use the --build and --use-buildtree options to shell into a build tree",
+ reason="only-build-run-supported",
+ )
+
+ with tempfile.NamedTemporaryFile(
+ delete_on_close=False, prefix=f"{target_name.replace('/','_')}_temp", suffix=".bst"
+ ) as temp_target_file:
+ _yaml.roundtrip_dump(target_node, temp_target_file)
+ temp_target_file.close() # delete_on_close is false so this doesn't remove the file, but delete is True(default) so we delete the file when we leave the context manager.
+
+ target_loader._set_fullpath_overrides(target_name, temp_target_file.name)
+
+ self.build([target])
Review Comment:
This should no longer be necessary with the fullpath override, right?
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3988789301
##########
src/buildstream/_loader/loader.py:
##########
@@ -255,6 +255,19 @@ def foreach_parent(parent):
# Private Methods #
###########################################
+ # _set_fullpath_overrides()
Review Comment:
This shouldn't be a private method as it's used by `Stream`. `Loader` is not part of the public (plugin) API, so 'public' methods in `Loader` will still only be part of the internal API.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3970185275
##########
src/buildstream/_stream.py:
##########
@@ -235,6 +240,65 @@ def query_cache(self, elements, *, sources_of_cached_elements=False, only_source
task.add_current_progress()
+ # shell_with()
+ #
+ # Run a shell with other targets.
+ #
+ # Automatically creates a temporary target based on 'target' with 'other_targets' as runtime or build dependencies.
+ #
+ # Note: Method will build the temporary target, before entering into it's shell.
+ #
+ # Args:
+ # target (str): The name of the element to run the shell for
+ # other_targets: (Iterable[str]): The name of the other elements to run the shell with.
+ # scope: _Scope: Either BUILD or RUN
+ # *args, **kwargs: Passed to shell() untouched.
+ #
+ # Returns:
+ # (int): The exit code of the launched shell
+ #
+ def shell_with(self, target: str, other_targets: Iterable[str], scope: _Scope, *args, **kwargs):
+
+ assert self._project, "Must have a project"
+ assert self._project.loader, "Project must have loader"
+
+ _, target_name, target_loader = self._project.loader._parse_name(target, MappingNode.from_dict({}))
+
+ target_path = os.path.join(target_loader._basedir, target_name)
+ target_node: CommentedMap = _yaml.roundtrip_load(target_path)
+
+ if scope == _Scope.RUN:
+ r_depends = target_node.get("runtime-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["runtime-depends"] = r_depends
+ elif scope == _Scope.BUILD:
+ r_depends = target_node.get("build-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["build-depends"] = r_depends
+ else:
+ raise StreamError(
+ "Only BUILD and RUN scopes are supported",
+ detail="Use the --build and --use-buildtree options to shell into a build tree",
+ reason="only-build-run-supported",
+ )
+
+ with tempfile.NamedTemporaryFile(
+ delete_on_close=False, prefix=f"{target_name.replace("/","_")}_temp", suffix=".bst"
Review Comment:
This works for whichever version of python I am using *sigh*
```suggestion
delete_on_close=False, prefix=f"{target_name.replace('/','_')}_temp", suffix=".bst"
```
I am not very happy with the replace.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3988789301
##########
src/buildstream/_loader/loader.py:
##########
@@ -255,6 +255,19 @@ def foreach_parent(parent):
# Private Methods #
###########################################
+ # _set_fullpath_overrides()
Review Comment:
This shouldn't be a private method as it's used by `Stream`. `Loader` is not part of the public (plugin) API, so 'public' methods in `Loader` will still only be part of the internal API.
Also, `_set_fullpath_overrides()` sounds to me like it sets the whole dict as it's plural. I might call it `add_fullpath_override()`.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-5633976226
Nit: We prefer limiting normal text lines in git commit messages to 72 characters, matching usual git conventions.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-4993328630
```bash
tox
tox -- --integration
tox -e docs
tox -e lint
tox -e mypy
tox -e format
```
And the tests locally only run on 313 and 314 for me, while CI does also 310 311 312
Is complete list right?
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-5000415945
> And the tests locally only run on 313 and 314 for me, while CI does also 310 311 312
tox runs the tests on all Python versions from 3.10 to 3.14 by default, but skipping versions where the interpreter is not installed. It's normally fine to test against just one Python version locally, letting CI take care of the others, unless you're actively debugging a version-specific issue.
> Is complete list right?
CI also runs buildgrid and buildbarn, tests against master of buildstream-plugins-community, and builds wheels. Unless you're working in these specific areas, it shouldn't be necessary to run those locally.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r4015853257
##########
src/buildstream/_stream.py:
##########
@@ -235,6 +240,65 @@ def query_cache(self, elements, *, sources_of_cached_elements=False, only_source
task.add_current_progress()
+ # shell_with()
+ #
+ # Run a shell with other targets.
+ #
+ # Automatically creates a temporary target based on 'target' with 'other_targets' as runtime or build dependencies.
+ #
+ # Note: Method will build the temporary target, before entering into it's shell.
+ #
+ # Args:
+ # target (str): The name of the element to run the shell for
+ # other_targets: (Iterable[str]): The name of the other elements to run the shell with.
+ # scope: _Scope: Either BUILD or RUN
+ # *args, **kwargs: Passed to shell() untouched.
+ #
+ # Returns:
+ # (int): The exit code of the launched shell
+ #
+ def shell_with(self, target: str, other_targets: Iterable[str], scope: _Scope, *args, **kwargs):
+
+ assert self._project, "Must have a project"
+ assert self._project.loader, "Project must have loader"
+
+ _, target_name, target_loader = self._project.loader._parse_name(target, MappingNode.from_dict({}))
+
+ target_path = os.path.join(target_loader._basedir, target_name)
+ target_node: CommentedMap = _yaml.roundtrip_load(target_path)
+
+ if scope == _Scope.RUN:
+ r_depends = target_node.get("runtime-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["runtime-depends"] = r_depends
+ elif scope == _Scope.BUILD:
+ r_depends = target_node.get("build-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["build-depends"] = r_depends
+ else:
+ raise StreamError(
+ "Only BUILD and RUN scopes are supported",
+ detail="Use the --build and --use-buildtree options to shell into a build tree",
+ reason="only-build-run-supported",
+ )
+
+ with tempfile.NamedTemporaryFile(
+ delete_on_close=False, prefix=f"{target_name.replace('/','_')}_temp", suffix=".bst"
+ ) as temp_target_file:
+ _yaml.roundtrip_dump(target_node, temp_target_file)
+ temp_target_file.close() # delete_on_close is false so this doesn't remove the file, but delete is True(default) so we delete the file when we leave the context manager.
+
+ target_loader._set_fullpath_overrides(target_name, temp_target_file.name)
+
+ self.build([target])
Review Comment:
hmmm, No, I will do some further testing.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r4046232097
##########
src/buildstream/_loader/loader.py:
##########
@@ -251,10 +255,61 @@ def foreach_parent(parent):
for parent in self._alternative_parents:
yield from foreach_parent(parent)
+ # temporary_modified_element()
+ #
+ # Temporarily modify an element by loading the element and applying modify_elements_function to make the modifications
+ #
+ #
+ # Args:
+ # target (str): The element-path relative bst file
+ # modify_element_function (Callable[[CommentedMap],None]): A function to modify a given CommentedMap
+ #
+ @contextmanager
+ def temporary_modified_element(
+ self, target: str, modify_element_function: Callable[[CommentedMap], None]
+ ) -> Generator[None, None, None]:
+
+ _, target_name, target_loader = self._parse_name(target, MappingNode.from_dict({}))
+
+ target_path = os.path.join(target_loader._basedir, target_name)
+ target_node: CommentedMap = _yaml.roundtrip_load(target_path)
+
+ modify_element_function(target_node)
+
+ # FIXME When 3.12 hits EOL, replace this with tempfile.NamedTemporaryFile itself.
+ with _legacy_named_temporary_file_delete_on_close(
Review Comment:
We generally use `utils._tempnamedfile()`, which adds cleanup on SIGTERM. And we just `.flush()` the temp file without closing it, which works also without `delete_on_close=False` support.
While moving the utility function to `delete_on_close=False` sounds reasonable once we require Python 3.12, I don't think it makes sense to add a helper to the `Loader` here. Let's be consistent and use the same approach as elsewhere in the codebase.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r4230575676
##########
src/buildstream/_frontend/cli.py:
##########
@@ -772,19 +807,35 @@ def shell(
mounts = [_HostMount(path, host_path) for host_path, path in mount]
try:
- exitcode = app.stream.shell(
- target,
- scope,
- app.shell_prompt,
- mounts=mounts,
- isolate=isolate,
- command=command,
- usebuildtree=cli_buildtree,
- artifact_remotes=artifact_remotes,
- source_remotes=source_remotes,
- ignore_project_artifact_remotes=ignore_project_artifact_remotes,
- ignore_project_source_remotes=ignore_project_source_remotes,
- )
+ if other_targets:
Review Comment:
Either here or in `Stream.shell_with()` we should reject the combination of `--use-buildtree` and `--with` with a clear error message as this combination can't work, right?
##########
src/buildstream/_loader/loader.py:
##########
@@ -14,14 +14,19 @@
# Authors:
# Tristan Van Berkom <tr...@codethink.co.uk>
+from buildstream import utils
+import sys
+import tempfile
Review Comment:
```
src/buildstream/_loader/loader.py:18:0: W0611: Unused import sys (unused-import)
src/buildstream/_loader/loader.py:19:0: W0611: Unused import tempfile (unused-import)
```
##########
src/buildstream/_loader/loader.py:
##########
@@ -251,10 +256,58 @@ def foreach_parent(parent):
for parent in self._alternative_parents:
yield from foreach_parent(parent)
+ # temporary_modified_element()
+ #
+ # Temporarily modify an element by loading the element and applying modify_elements_function to make the modifications
+ #
+ #
+ # Args:
+ # target (str): The element-path relative bst file
+ # modify_element_function (Callable[[CommentedMap],None]): A function to modify a given CommentedMap
+ #
+ @contextmanager
+ def temporary_modified_element(
+ self, target: str, modify_element_function: Callable[[CommentedMap], None]
+ ) -> Generator[None, None, None]:
+
+ _, target_name, target_loader = self._parse_name(target, MappingNode.from_dict({}))
+
+ target_path = os.path.join(target_loader._basedir, target_name)
+ target_node: CommentedMap = _yaml.roundtrip_load(target_path)
+
+ modify_element_function(target_node)
+
+ with utils._tempnamedfile(prefix=f"{target_name.replace('/','_')}_temp", suffix=".bst") as temp_target_file:
+ _yaml.roundtrip_dump(target_node, temp_target_file)
+ temp_target_file.flush()
+ target_loader._set_fullpath_override(target_name, temp_target_file.name)
+
+ try:
+ yield
+ finally:
+ target_loader._set_fullpath_override(target_name, None)
+
###########################################
# Private Methods #
###########################################
+ # _set_fullpath_override()
+ #
+ # Set an fullpath override for a element-path relative bst file
+ #
+ # This enables runtime modified elements to be pulled from a temporary directory
+ # Passing None as a fullpath remove the entry
+ #
+ # Args:
+ # filename (str): The element-path relative bst file
+ # fullpath (str|None): A fullpath to the bst file, or None
+ #
+ def _set_fullpath_override(self, filename: str, fullpath: str | None):
+ if fullpath:
+ self._fullpath_overrides[filename] = fullpath
+ else:
+ self._fullpath_overrides.pop(filename, None)
Review Comment:
Nit: Each branch is a one-liner and used exactly once. Wouldn't it be simpler to inline it at the call site?
If you think it's clearer to have this in a separate method (it could hypothetically also be used for other purposes), then I think this should be a context manager to provide some actual benefit at the call site.
Either variant is fine with me.
##########
tests/integration/shell.py:
##########
@@ -86,6 +107,28 @@ def test_executable(cli, datafiles):
assert result.output == "Horseys!\n"
+# Test staging and running additional targets in the shell of the main target for debugging.
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox")
+def test_with_other_targets(cli, datafiles):
Review Comment:
I'd like to see at least a minimal test for a build shell as well.
##########
src/buildstream/_stream.py:
##########
@@ -235,6 +239,53 @@ def query_cache(self, elements, *, sources_of_cached_elements=False, only_source
task.add_current_progress()
+ # shell_with()
+ #
+ # Run a shell with other targets.
+ #
+ # Automatically creates a temporary target based on 'target' with 'other_targets' as runtime or build dependencies.
+ #
+ # Note: Method will build the temporary target, before entering into it's shell.
+ #
+ # Args:
+ # target (str): The name of the element to run the shell for
+ # other_targets: (Iterable[str]): The name of the other elements to run the shell with.
+ # scope: _Scope: Either BUILD or RUN
+ # *args, **kwargs: Passed to shell() untouched.
+ #
+ # Returns:
+ # (int): The exit code of the launched shell
+ #
+ def shell_with(self, target: str, other_targets: Iterable[str], scope: _Scope, *args, **kwargs):
+
+ assert self._project, "Must have a project"
+ assert self._project.loader, "Project must have loader"
+
+ def add_deps_to_element(target_node: CommentedMap):
+ if scope == _Scope.RUN:
+ r_depends = target_node.get("runtime-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["runtime-depends"] = r_depends
+ elif scope == _Scope.BUILD:
+ r_depends = target_node.get("build-depends", [])
+
+ for other_target in other_targets:
+ r_depends.append(other_target)
+
+ target_node["build-depends"] = r_depends
+ else:
+ raise StreamError(
+ "Only BUILD and RUN scopes are supported",
+ detail="Use the --build and --use-buildtree options to shell into a build tree",
Review Comment:
This detail doesn't make sense to me. Leftover from copy paste?
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-6360736544
> I tested it and while it works fine in some cases, it seems to change the cache keys in others.
Maybe we need to use different approaches for build and runtime shells after all. As far as I can tell, there is no cache key issue with the current approach for build shells. But maybe we need to switch to the virtual stack element approach for runtime shells, which we've discussed before.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3970192851
##########
src/buildstream/_loader/loader.py:
##########
@@ -255,6 +255,18 @@ def foreach_parent(parent):
# Private Methods #
###########################################
+ # _set_fullpath_overrides()
+ #
+ # Set an fullpath override for a element-path relative bst file
+ #
+ # This enables runtime modified elements to be pulled from a temporary directory
+ #
+ # Args:
+ # filename (str): The element-path relative bst file
+ # fullpath (str): A fullpath to the bst file
Review Comment:
```suggestion
# fullpath (str): A fullpath to the bst file
#
```
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3970189150
##########
tests/integration/shell.py:
##########
@@ -46,18 +49,35 @@
# mount (tuple): A (host, target) tuple for the `--mount` option
# element (str): The element to build and run a shell with
# isolate (bool): Whether to pass --isolate to `bst shell`
-#
-def execute_shell(cli, project, command, *, config=None, mount=None, element="base.bst", isolate=False):
+# other_elements (list(str)): Other elements to stage in the sandbox
Review Comment:
```suggestion
# other_elements (list(str)): Other elements to stage in the sandbox
#
```
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3988805600
##########
src/buildstream/_frontend/cli.py:
##########
@@ -748,13 +755,38 @@ def shell(
otherwise bst may respond to them instead. e.g.
\b
- bst shell example.bst -- df -h
+ bst shell base.bst -- df -h
Use the --build option to create a temporary sysroot for
building the element instead.
+ Use the --with option to stage the artifacts of other elements
+ into the temporary sysroot to make them available to run e.g.
+
+ \b
+ bst shell --with base.bst example.bst -- cat example.txt
+
If no COMMAND is specified, the default is to attempt
to run an interactive shell.
+
+ # Examples:
+
+ \b
+ # Attempt to run an interactive shell with example.bst
+ bst shell example.bst
+ # Attempt to run an df -h with example.bst
+ bst shell example.bst -- df h
+ # In a workspace directory, attempt to shell into the workspace element
+ bst shell
+ # Attempt to run cat from base.bst to read example.txt from example.bst
+ bst shell --with base.bst example.bst -- cat example.txt
+ # Attempt to run an interactive shell with the sources and all dependencies of example.bst
+ bst shell --build example.bst
+
+ For all examples on this page:
+ - example.bst is a simple import element with no dependencies
+ that imports a file called example.txt
+ - base.bst provides a basic alpine sysroot with a standard set of unix tooling (sh, df, cat etc).
Review Comment:
build_docs CI fails with
```
/home/testuser/buildstream/doc/source/using_commands.rst:42: WARNING: Block quote ends without a blank line; unexpected unindent. [docutils]
```
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3880135503
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
While injecting deps in the in-memory representation should theoretically be possible, it would probably be too invasive just for this feature.
A possible tweak to the temp file approach could be to add some kind of substitution / path override dict to the `Loader`. This could be set in `Stream.shell_with()`. `_load_file_no_deps()` would then use the path override, instead of constructing the regular `fullpath`. However, it would still use the regular `filename` as `shortname`.
I haven't prototyped this, but the possible advantages are:
* No cache key difference for runtime shells, being able to use a previously cached build
* Logging wouldn't expose temp filenames
* Temp file could be placed in a temporary directory instead of polluting the elements directory with a temporary file
Any thoughts?
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-4991965499
I think this should pass CI now, fixed docs, linters, static type checker, formatter and 3.10 issues
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3602529275
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
The separate staging and integration into the same sandbox could be problematic. The main target element and the other targets may share some dependencies, in which case an element gets staged twice. This may cause confusing overlap warnings or errors (and in some constellations maybe also some real overlap conflicts).
Separate integration means that integration commands of dependencies of the main target can't cover integration with other targets. And integration commands of shared dependencies will be executed twice (or even more with multiple `--with`). Additional sandbox batch execution for other target integration may also not be the most efficient approach, but performance is not my main concern here.
I'm not saying that this approach is definitely unacceptable, but at the very least it needs to have documented and tested behavior for mentioned aspects such as overlaps and integration commands. Also build shells might not be tested at all right now, if I haven't missed anything.
Regarding overlaps, a possible alternative would be to stage the other elements into a separate sandbox / virtual directory (with the usual overlap processing) and then merge it into the shell where the overlap handling may be different (e.g., the `--with` tree allowed to always replace files). This was also suggested in https://mail.gnome.org/archives/buildstream-list/2019-February/msg00001.html. Integration commands will still be problematic but maybe some limitations there are acceptable (but should also be clarified). I would definitely at least use a single integration sandbox for all 'other' elements and don't duplicate integration within that part.
### Virtual stack element
If it was only for runtime shells, I think the behavior should rather be equivalent to creating a stack element that has the main target and all other targets as dependencies, which would likely not even require any changes in `element.py`. One caveat is that runtime shells use the environment variables from the target element, so that would break with the (virtual) stack element approach.
However, build shells make things more complicated as there the main target has essentially full control over the sandbox.
### Inject other targets as dependencies
One possible alternative that comes to mind is that we may be able to inject the `--with` elements as dependencies of the main target (runtime dependency for runtime shells and build dependency for build shells). There could be element plugins where this is problematic for build shells but normal build elements should be fine and build shells anyway can't work with all element plugins.
It's possible that I'm missing something why this would be a bad idea, but it might be worth exploring if nobody can think of a clear blocker right away.
One issue I can think of is that it might not work with buildtrees where we get the full sandbox root from CAS and don't stage anything. It may be possible to support an alternative buildtree support (only used with `--with`) where we first construct a sandbox like for a normal build shell and then only replace the source/build directory with the corresponding directory from the buildtree. If we want to go down this route, this should likely wait for a follow-up 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.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-5000740373
Ah there's a subtle conflict between format and the docs generation.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3803200104
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
```
$ tox -e venv -- bst --directory tests/integration/project shell manual/import-file.bst
[--:--:--][ ][ main:core activity ] START Loading elements
[00:00:00][ ][ main:core activity ] SUCCESS Loading elements
[--:--:--][ ][ main:core activity ] START Resolving elements
[00:00:00][ ][ main:core activity ] SUCCESS Resolving elements
[--:--:--][ ][ main:core activity ] START Initializing remote caches
[00:00:00][ ][ main:core activity ] SUCCESS Initializing remote caches
[--:--:--][ ][ main:core activity ] START Query cache
[00:00:00][ ][ main:core activity ] SUCCESS Query cache
[--:--:--][dc2422ef][ main:manual/import-file.bst ] START Staging dependencies
[00:00:00][dc2422ef][ main:manual/import-file.bst ] SUCCESS Staging dependencies
[--:--:--][dc2422ef][ main:manual/import-file.bst ] START Integrating sandbox
[00:00:00][dc2422ef][ main:manual/import-file.bst ] SUCCESS Integrating sandbox
[--:--:--][dc2422ef][ main:manual/import-file.bst ] STATUS Running command
sh -i
Error launching shell: Staged artifacts do not provide command 'sh'
$ tox -e venv -- bst --directory tests/integration/project shell manual/import-file.bst --with base/base-alpine.bst
[--:--:--][ ][ main:core activity ] START Loading elements
/var/home/nathanwilliams/buildstream/tests/integration/project/elements/manual/import-file.bst_tempshwrznrg.bst
manual/import-file.bst_tempshwrznrg.bst
manual/import-file.bst_tempshwrznrg.bst
[00:00:00][ ][ main:core activity ] SUCCESS Loading elements
[--:--:--][ ][ main:core activity ] START Resolving elements
[00:00:00][ ][ main:core activity ] SUCCESS Resolving elements
[--:--:--][ ][ main:core activity ] START Initializing remote caches
[00:00:00][ ][ main:core activity ] SUCCESS Initializing remote caches
[--:--:--][ ][ main:core activity ] START Query cache
[00:00:00][ ][ main:core activity ] SUCCESS Query cache
[--:--:--][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] START test/manual-import-file.bst_tempshwrznrg/e8d846b7-build.20260818-111411.log
[--:--:--][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] START Staging sources
[00:00:00][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Staging sources
[--:--:--][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] START Caching artifact
[00:00:00][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Caching artifact
[00:00:00][e8d846b7][ build:manual/import-file.bst_tempshwrznrg.bst] SUCCESS test/manual-import-file.bst_tempshwrznrg/e8d846b7-build.20260818-111411.log
[--:--:--][ ][ main:core activity ] START Loading elements
[00:00:00][ ][ main:core activity ] SUCCESS Loading elements
[--:--:--][ ][ main:core activity ] START Resolving elements
[00:00:00][ ][ main:core activity ] SUCCESS Resolving elements
[--:--:--][ ][ main:core activity ] START Initializing remote caches
[00:00:00][ ][ main:core activity ] SUCCESS Initializing remote caches
[--:--:--][ ][ main:core activity ] START Query cache
[00:00:00][ ][ main:core activity ] SUCCESS Query cache
[--:--:--][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] START Staging dependencies
[00:00:00][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Staging dependencies
[--:--:--][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] START Integrating sandbox
[00:00:00][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] SUCCESS Integrating sandbox
[--:--:--][e8d846b7][ main:manual/import-file.bst_tempshwrznrg.bst] STATUS Running command
sh -i
[e8d846b7@manual/import-file.bst_tempshwrznrg.bst:/]$ exit
```
hmm, I went down the route of creating a temporary element.
Due to how the element loading works, it makes it almost impossible to inject additional dependencies in at runtime. Temporary stack element would have made buildtree ones unhelpful. Solving the overlap problem and dealing with overlapping sandboxes and integration commands was too complicated and messy.
The downside of using a temporary element is the shell command needs to attempt to run a build on the temporary element, before it can shell, to cache it's build result.
I'll tidy up my prototype with some tests and push it a bit later on.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3879364823
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
> Temporary stack element would have made buildtree ones unhelpful.
> The downside of using a temporary element is the shell command needs to attempt to run a build on the temporary element, before it can shell, to cache it's build result.
Not being able to use a cached build seems like a major downside to me. I was originally thinking of injecting it in the in-memory representation, not creating a temporary file. But maybe that's not feasible.
A possible mitigation with the temporary file approach could be using different approaches for build and runtime shells. A temporary stack element for runtime shells and what your branch is doing now for build shells. Or have you already considered this as not tenable for some reason?
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3903264934
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
It might be nice to have a 'reload' method on Element that can cleanly reload the element if there are changes at runtime, but it would involve some heavy refactoring that I currently don't have the context to do. Element is heavily implemented around the idea of everything is immutable.
We could almost do with a replacing the whole Element class with an explicit state machine to represent elements at different stages of it's lifecycle. There are so many Optional fields and 'state' booleans, it's difficult to untangle and work out which state the element is currently in, and what order to call things in etc.
<img width="535" height="675" alt="image" src="https://github.com/user-attachments/assets/a45c8cd0-71ce-4a3c-a7ed-a93aa312681c" />
```plantuml
[*] --> LoadElement: Load from YAML
LoadElement --> WeakElement: Calculate weak cache key
WeakElement --> StrongElement: Resolve dependencies and calculate strong key
StrongElement --> CachedElement: Element has cached artifacts
StrongElement --> PreparedElement: Prepare sources and dependencies for a build
PreparedElement --> CachedElement: Build the element
PreparedElement --> FailedBuildElement: cache buildtree
```
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#discussion_r3958739484
##########
src/buildstream/element.py:
##########
@@ -2076,6 +2091,17 @@ def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command
if prompt is not None:
environment["PS1"] = prompt
+ with self.timed_activity("Staging other_targets", silent_nested=True), self.__collect_overlaps(sandbox):
+ self.stage_dependency_artifacts(sandbox, other_elements)
+
+ if other_elements:
+ # Stage artifacts from other_elements into the sandbox.
+ for element in other_elements:
+ # Stage deps in the sandbox root
+ with element.timed_activity("Integrating sandbox"), sandbox.batch():
+ for dep in element._dependencies(_Scope.RUN):
+ dep.integrate(sandbox)
Review Comment:
I've made the changes to how the tempfile is handled to make it more transparent. Original element name now appears in the logs.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-4993286576
pre-commit hooks would be nice. I keep running the commands locally, but It seems I am doing them in the wrong order.. fixing one thing breaks the others XD
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shell API and CLI: Add option for staging additional runtime targets [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #2147:
URL: https://github.com/apache/buildstream/pull/2147#issuecomment-6343726440
I tested it and while it works fine in some cases, it seems to change the cache keys in others.
Using gnome-build-meta commit e9caef3328f1c3804940fc4ee10c8adcfbf7ee83, I build an element using `bst build core/gnome-software.bst`, then running the shell using `bst shell core/gnome-software.bst --with sdk-platform.bst` fails with the error
```
[--:--:--][ ][ main:core activity ] INFO Attempting to fetch missing or incomplete artifact(s)
[--:--:--][25165b5f][ pull:core/gnome-software.bst ] START gnome/core-gnome-software/25165b5f-pull.20261022-110002.log
[--:--:--][25165b5f][ pull:core/gnome-software.bst ] STATUS Pulling artifact 25165b5f <- https://gbm.gnome.org:11003
[--:--:--][25165b5f][ pull:core/gnome-software.bst ] INFO Remote (https://gbm.gnome.org:11003) does not have artifact 25165b5f cached
[00:00:00][25165b5f][ pull:core/gnome-software.bst ] SKIPPED Pull
Error launching shell: Elements need to be built or downloaded before staging a shell environment
core/gnome-software.bst
```
Looking up the cache key of gnome-software, I see:
```
cached 98a4f5e6631898c1d3ba4fd9bd325f961954ae343a42676547f3bdedc1a97df1 core/gnome-software.bst
```
So the cache key was changed by adding the `--with` argument.
(I suspect this is due to adding more runtime dependencies that are also build dependencies, and triggering https://github.com/apache/buildstream/issues/1995)
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org