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 <[email protected]>
+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.
[email protected](DATA_DIR)
[email protected](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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
