Remote: Don't upload action result if declared outputs are not created. #15016
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brentleyjones
pushed a commit
to brentleyjones/bazel
that referenced
this pull request
Mar 15, 2022
Even if the exit code is 0. Missing declared outputs will be detected by Bazel later and fail the build, so avoid uploading this false positive cache entry. Wrap buildUploadManifest inside a `Single.fromCallable` since there are many IOs (both the check we add in this PR and stats already there). If `--experimental_remote_cache_async` is set, these IOs will now be executed in a background thread. Fixes bazelbuild#14543. Closes bazelbuild#15016. PiperOrigin-RevId: 434448255 (cherry picked from commit 5b54588)
|
Just found that this PR breaks remote cache for tests. Tests are somehow different than normal actions: the spawn created by it declares all the possible outputs but they are not necessary generated by the spawn itself. Checking whether a fix available, otherwise have to rollback this change. |
|
Luckily, a similar problem has already been solved internally and related code is available (namely |
bazel-io
pushed a commit
that referenced
this pull request
Mar 17, 2022
An AC entry that misses declared outputs of an action is invalid because, if Bazel accepts this from remote cache, it will detect the missing declared outputs error at a later stage and fail the build. This PR adds a check for mandatory outputs when downloading outputs from remote cache. If a mandatory output is missing from AC entry, Bazel will ignore the cache entry so build can continue. Also fixes an issue introduced by #15016 that tests result are not uploaded to remote cache. Test spawns declare all the possible outputs but they usually don't generate them all. This change fixes that by only check for mandatory outputs via `spawn.isMandatoryOutput()`. Fixes #14543. Closes #15051. PiperOrigin-RevId: 435307260
brentleyjones
pushed a commit
to brentleyjones/bazel
that referenced
this pull request
Mar 17, 2022
An AC entry that misses declared outputs of an action is invalid because, if Bazel accepts this from remote cache, it will detect the missing declared outputs error at a later stage and fail the build. This PR adds a check for mandatory outputs when downloading outputs from remote cache. If a mandatory output is missing from AC entry, Bazel will ignore the cache entry so build can continue. Also fixes an issue introduced by bazelbuild#15016 that tests result are not uploaded to remote cache. Test spawns declare all the possible outputs but they usually don't generate them all. This change fixes that by only check for mandatory outputs via `spawn.isMandatoryOutput()`. Fixes bazelbuild#14543. Closes bazelbuild#15051. PiperOrigin-RevId: 435307260 (cherry picked from commit a151116)
coeuvre
added a commit
to coeuvre/bazel
that referenced
this pull request
Mar 18, 2022
Even if the exit code is 0. Missing declared outputs will be detected by Bazel later and fail the build, so avoid uploading this false positive cache entry. Wrap buildUploadManifest inside a `Single.fromCallable` since there are many IOs (both the check we add in this PR and stats already there). If `--experimental_remote_cache_async` is set, these IOs will now be executed in a background thread. Fixes bazelbuild#14543. Closes bazelbuild#15016. PiperOrigin-RevId: 434448255
coeuvre
added a commit
to coeuvre/bazel
that referenced
this pull request
Mar 18, 2022
An AC entry that misses declared outputs of an action is invalid because, if Bazel accepts this from remote cache, it will detect the missing declared outputs error at a later stage and fail the build. This PR adds a check for mandatory outputs when downloading outputs from remote cache. If a mandatory output is missing from AC entry, Bazel will ignore the cache entry so build can continue. Also fixes an issue introduced by bazelbuild#15016 that tests result are not uploaded to remote cache. Test spawns declare all the possible outputs but they usually don't generate them all. This change fixes that by only check for mandatory outputs via `spawn.isMandatoryOutput()`. Fixes bazelbuild#14543. Closes bazelbuild#15051. PiperOrigin-RevId: 435307260
Wyverald
pushed a commit
that referenced
this pull request
Mar 18, 2022
…ere not created (#15071) * Add a method, Spawn#isOutputMandatory, to indicate whether a Spawn must create an output by the end of its execution. If not, a Spawn (like a test) may succeed without necessarily creating that output. The 4 categories of actions that do this are: 1. Tests (tests can create XML and other files, but may not). 2. Java compilations with reduced classpaths (the initial compilation with a reduced classpath may fail, but produce a usable jdeps proto file, so the Spawn will claim that it succeeded so that the action can process the proto file). 3. Extra actions with a dummy output that is produced locally, not by the Spawn. However, by changing the outputs of the Spawn to be empty, this situation is avoided. 4. C++ compilations with coverage (a .gcno coverage file is not produced for an empty translation unit). In particular, all SpawnActions' Spawns have #isMandatoryOutput always return true, and there should be no new cases of #isMandatoryOutput returning false besides the ones added in this CL. PiperOrigin-RevId: 425616085 * Remote: Don't upload action result if declared outputs are not created. Even if the exit code is 0. Missing declared outputs will be detected by Bazel later and fail the build, so avoid uploading this false positive cache entry. Wrap buildUploadManifest inside a `Single.fromCallable` since there are many IOs (both the check we add in this PR and stats already there). If `--experimental_remote_cache_async` is set, these IOs will now be executed in a background thread. Fixes #14543. Closes #15016. PiperOrigin-RevId: 434448255 * Remote: Check declared outputs when downloading outputs. An AC entry that misses declared outputs of an action is invalid because, if Bazel accepts this from remote cache, it will detect the missing declared outputs error at a later stage and fail the build. This PR adds a check for mandatory outputs when downloading outputs from remote cache. If a mandatory output is missing from AC entry, Bazel will ignore the cache entry so build can continue. Also fixes an issue introduced by #15016 that tests result are not uploaded to remote cache. Test spawns declare all the possible outputs but they usually don't generate them all. This change fixes that by only check for mandatory outputs via `spawn.isMandatoryOutput()`. Fixes #14543. Closes #15051. PiperOrigin-RevId: 435307260 Co-authored-by: janakr <janakr@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Even if the exit code is 0. Missing declared outputs will be detected by Bazel later and fail the build, so avoid uploading this false positive cache entry.
Wrap buildUploadManifest inside a
Single.fromCallablesince there are many IOs (both the check we add in this PR and stats already there). If--experimental_remote_cache_asyncis set, these IOs will now be executed in a background thread.Fixes #14543.