Shell API and CLI: Add option for staging additional runtime targets #2147
Shell API and CLI: Add option for staging additional runtime targets #2147nathanwilliams-ct wants to merge 2 commits into
Conversation
|
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. |
88a3419 to
6c72e88
Compare
6c82853 to
fb98811
Compare
|
I think this should pass CI now, fixed docs, linters, static type checker, formatter and 3.10 issues |
5c266ec to
4857d9e
Compare
|
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 |
tox
tox -- --integration
tox -e docs
tox -e lint
tox -e mypy
tox -e formatAnd the tests locally only run on 313 and 314 for me, while CI does also 310 311 312 Is complete list right? |
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.
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. |
|
Ah there's a subtle conflict between format and the docs generation. |
4857d9e to
abffc99
Compare
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
abffc99 to
5a52dae
Compare
|
Static type checking moved out into it's own PR: #2153 |
| 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) |
There was a problem hiding this 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.
There was a problem hiding this comment.
I will look to explore these alternative routes, thanks for the feedback.
2 participants
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 notpresent, splitting based on a
.bstsuffix. This UX could be re-visited in future.Example usage:
bst shell --with base.bst example.bst -- cat example.txt
Where:
Changes:
test_with_other_targetsintegration test to the shell test suite.--withcli option to the shell subcommand and updates it's documentation.other_targets
towards: #422