Enable plz build --shell` to work correctly with remote execution and subrepos
#3066
Conversation
…nd subrepos Instead of trying to move files around after everything is built, we instead simply download all the original target's inputs from CAS when running in remote execution mode.
| @@ -1298,6 +1301,7 @@ func (state *BuildState) GetPreloadedSubincludes() []BuildLabel { | |||
|
|
|||
| // DownloadInputsIfNeeded downloads all the inputs (or runtime files) for a target if we are building remotely. | |||
| func (state *BuildState) DownloadInputsIfNeeded(target *BuildTarget, runtime bool) error { | |||
| // TODO(jan): Remove this function once `DownloadAllInputs` has been fully implemented across both build & test. | |||
There was a problem hiding this comment.
I don't really understand what is better about DownloadAllInputs vs. this. It just seems to be pushing the check of whether the remote client is set up out to the callers?
There was a problem hiding this comment.
DownloadInputsIfNeeded works by looking at a list of specific inputs on a target and downloading those individually. DownloadAllInputs just graps the whole input directory and downloads it, without doing any of the IterInputs logic, etc.
There was a problem hiding this comment.
It's a bit nuanced but the original bug we were seeing was due to the root field on the subrepo being different when running remotely vs. locally. When it's remote, the root is relative to the output directory of the target (we construct a Tree proto for the outputs and use this as the basis for the remote filesystem). Consumers of the subrepo in the remote package know to resolve these paths against the subrepo FS.
The problem is the IterInputs() code is for local execution and expects the paths to be relative to the host repo's root i.e. contain the plz-out/gen/subrepo_name. It uses the (*SubrepoFileLabel).FullPaths() function which returns a string list, which loses all information about the path being relative to the subrepo filesystem.
This was the simplest solution. We might've been able to make the subrepo root FS the same across local and remote execution if we munged the paths around in the remote package, but that would require wider sweeping changes.
|
I've discovered an issue with non-subrepo targets, so this PR isn't ready to go just yet sadly. |
src/remote/remote.go
Outdated
| pbDir, err = c.uploadInputs(ch, target, isTest) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
You can just return err without the nil check. If it's nil then that's fine.
| @@ -1298,6 +1301,7 @@ func (state *BuildState) GetPreloadedSubincludes() []BuildLabel { | |||
|
|
|||
| // DownloadInputsIfNeeded downloads all the inputs (or runtime files) for a target if we are building remotely. | |||
| func (state *BuildState) DownloadInputsIfNeeded(target *BuildTarget, runtime bool) error { | |||
| // TODO(jan): Remove this function once `DownloadAllInputs` has been fully implemented across both build & test. | |||
There was a problem hiding this comment.
It's a bit nuanced but the original bug we were seeing was due to the root field on the subrepo being different when running remotely vs. locally. When it's remote, the root is relative to the output directory of the target (we construct a Tree proto for the outputs and use this as the basis for the remote filesystem). Consumers of the subrepo in the remote package know to resolve these paths against the subrepo FS.
The problem is the IterInputs() code is for local execution and expects the paths to be relative to the host repo's root i.e. contain the plz-out/gen/subrepo_name. It uses the (*SubrepoFileLabel).FullPaths() function which returns a string list, which loses all information about the path being relative to the subrepo filesystem.
This was the simplest solution. We might've been able to make the subrepo root FS the same across local and remote execution if we munged the paths around in the remote package, but that would require wider sweeping changes.
This is a follow-up for thought-machine#3066
This is a follow-up for #3066
Instead of trying to move files around after everything is built, we instead simply download all the original target's inputs from CAS when running in remote execution mode.