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