Use ToplevelArtifactsDownloader to download toplevel artifacts #16524
Conversation
coeuvre
force-pushed
the
use-toplevel-downloader
branch
3 times, most recently
from
d4953da
to
9f0bdf2
Compare
coeuvre
force-pushed
the
use-toplevel-downloader
branch
from
9f0bdf2
to
85dff6e
Compare
src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/remote/util/Utils.java
Outdated
Show resolved
Hide resolved
| @@ -221,12 +221,54 @@ EOF | |||
| --remote_download_toplevel \ | |||
| //a:foobar >& $TEST_log || fail "Failed to build //a:foobar" | |||
|
|
|||
| expect_log "2 processes: 1 remote cache hit, 1 internal" | |||
| expect_log "1 process: 1 internal" | |||
There was a problem hiding this comment.
Interesting, so we won't report a fetch as a cache hit unless the action actually ran?
I guess this makes total sense under the guise of "separating action execution from downloading outputs", but now I'm wondering if we should add an additional "x files downloaded" stanza to the log message. Probably not in this PR, though.
There was a problem hiding this comment.
Interesting, so we won't report a fetch as a cache hit unless the action actually ran?
This is a little bit complex.
Before this change, we decide whether to download outputs of a spawn by checking whether any of its output is included in the set filesToDownload (the set is computed in the afterAnalysis callback in the RemoteModule based on analysis result). If we decided to download the outputs of the spawn, we download all the outputs but do not inject the metadata. It means we let skyframe to track the outputs on the local filesystem. So if we delete the output, skyframe will rerun the generating action, in which case, it will hit the remote cache and re-download the output.
With this change, we always inject metadata for outputs even if we decide to download them later. It means skyframe will use the metadata to track outputs. If we delete the local outputs, the metadata is still there, in which case, it will hit the skyframe cache.
ShreeM01
added
team-Remote-Exec
coeuvre
force-pushed
the
use-toplevel-downloader
branch
from
0414cab
to
05e5231
Compare
src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java
Outdated
Show resolved
Hide resolved
With #16524, we download toplevel artifacts by listening to `TargetCompleteEvent` so that it works for incremental builds (e.g. change toplevel targets, delete downloaded files, etc.). However, BES sends `TargetComplete` event also by listening to `TargetCompleteEvent` which means at the time when the consumers of BEP received `TargetComplete` events, the outputs might not have been downloaded. This CL changes the way we download toplevel artifacts from listening `TargetCompleteEvent` to checking whether an output is a toplevel artifact after action execution. It still works for incremental builds because of the RemoteOutputChecker we introduced in 60c9cf3. It doesn't work for BEP yet because the download still happens in the background. A following CL will fix that. Note that this change makes RemoteOutputChecker depending on *FULL* analysis result in incremental builds because it needs that to determine whether an output is toplevel artifact during action dirtiness check. It's incompatible with skymeld. A way to fix it is described in the code. PiperOrigin-RevId: 529026614 Change-Id: I6c0c9bf4b3f5bb076f404e46c00bc231789a2820
With bazelbuild#16524, we download toplevel artifacts by listening to `TargetCompleteEvent` so that it works for incremental builds (e.g. change toplevel targets, delete downloaded files, etc.). However, BES sends `TargetComplete` event also by listening to `TargetCompleteEvent` which means at the time when the consumers of BEP received `TargetComplete` events, the outputs might not have been downloaded. This CL changes the way we download toplevel artifacts from listening `TargetCompleteEvent` to checking whether an output is a toplevel artifact after action execution. It still works for incremental builds because of the RemoteOutputChecker we introduced in bazelbuild@60c9cf3. It doesn't work for BEP yet because the download still happens in the background. A following CL will fix that. Note that this change makes RemoteOutputChecker depending on *FULL* analysis result in incremental builds because it needs that to determine whether an output is toplevel artifact during action dirtiness check. It's incompatible with skymeld. A way to fix it is described in the code. PiperOrigin-RevId: 529026614 Change-Id: I6c0c9bf4b3f5bb076f404e46c00bc231789a2820
Previously, with
--remote_download_toplevel, Bazel only downloads toplevel outputs during spawn execution. It has a drawback that, if the toplevel targets are changed in a following invocation, but the generating actions are not able to be executed because of skyframe/action cache, the outputs of new toplevel targets are not downloaded.ToplevelArtifactsDownloaderfixes that issue by listening to theTargetCompleteEvent(which is fired every time after the toplevel target is built event if it hit the cache) and download outputs in the background. Additionally, it can listen to more events during the build hence is more flexible to define additional outputs to be downloaded as toplevel outputs.Working towards #12665.
Fixes #13625.
Fixes #11834.
Fixes #10525.