Skip to content

Commit

Permalink
Enable analysis caching codepaths only for commands that execute acti…
Browse files Browse the repository at this point in the history
…ons.

For example, this includes `test` and `build`, but does not include `cquery`.

This prevents the use case where the someone adds `build
--experimental_remote_analysis_cache_mode=download` by default into their
bazelrc, and runs a cquery command (which, WAI, picks up the flag, because
`CqueryCommand` inherits options from `TestCommand`, which inherits from
`BuildCommand`), and fails to this cryptic error:

```
$ bazel cquery 'somepath(//foo, //bar)'
ERROR: Failed to find PROJECT.scl file for: [//foo:foo, //bar:bar]
INFO: Elapsed time: 0.078s
ERROR: Build did NOT complete successfully
```

It also doesn't make sense to support analysis caching with cquery anyway, because correct results require a complete, unpruned analysis graph.

Same goes for `aquery`.

PiperOrigin-RevId: 689397164
Change-Id: Ib63ab41450b84e782d69e148196b2bfa6413e65f
  • Loading branch information
jin authored and copybara-github committed Oct 24, 2024
1 parent 8d0e66b commit 68f94a1
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ static ProjectEvaluationResult evaluateProjectFile(
throws LoadingFailedException, InvalidConfigurationException {
EnumSet<ProjectFileFeature> featureFlags = EnumSet.noneOf(ProjectFileFeature.class);

if (env.getOptions().getOptions(RemoteAnalysisCachingOptions.class).mode != OFF) {
if (env.getCommand().buildPhase().executes()
&& env.getOptions().getOptions(RemoteAnalysisCachingOptions.class).mode != OFF) {
// RemoteAnalysisCachingOptions is never null because it's a build command flag, and this
// method only runs for build commands.
featureFlags.add(ANALYSIS_CACHING);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,7 @@ private void logAnalysisCachingStatsAndMaybeUploadFrontier(
}

RemoteAnalysisCachingOptions options = request.getOptions(RemoteAnalysisCachingOptions.class);
if (options == null) {
if (options == null || !env.getCommand().buildPhase().executes()) {
return;
}

Expand Down Expand Up @@ -1108,6 +1108,7 @@ public static RemoteAnalysisCachingDependenciesProvider forAnalysis(
CommandEnvironment env, Optional<PathFragmentPrefixTrie> activeDirectoriesMatcher) {
var options = env.getOptions().getOptions(RemoteAnalysisCachingOptions.class);
if (options == null
|| !env.getCommand().buildPhase().executes()
|| !options.mode.downloadForAnalysis()
|| activeDirectoriesMatcher.isEmpty()) {
return DisabledDependenciesProvider.INSTANCE;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/runtime/commands",
"//src/main/java/com/google/devtools/build/lib/skyframe:aspect_key_creator",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
"//src/main/java/com/google/devtools/build/lib/skyframe:rule_configured_target_value",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
import com.google.devtools.build.lib.runtime.commands.CqueryCommand;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectBaseKey;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.skyframe.RemoteConfiguredTargetValue;
Expand Down Expand Up @@ -534,6 +535,37 @@ public void undoneNodesFromIncrementalChanges_ignoredForSerialization() throws E
.hasCount(SkyFunctions.CONFIGURED_TARGET, serializedConfiguredTargetCount - 1);
}

@Test
public void cquery_succeedsAndDoesNotTriggerUpload() throws Exception {
setupScenarioWithConfiguredTargets();
addOptions("--experimental_remote_analysis_cache_mode=upload");
runtimeWrapper.newCommand(CqueryCommand.class);
buildTarget("//foo:A"); // passes, even though there's no PROJECT.scl
assertThat(
getCommandEnvironment()
.getRemoteAnalysisCachingEventListener()
.getSerializedKeysCount())
.isEqualTo(0);
}

@Test
public void cquery_succeedsAndDoesNotTriggerUploadWithProjectScl() throws Exception {
setupScenarioWithConfiguredTargets();
write(
"foo/PROJECT.scl",
"""
active_directories = { "default": ["foo"] }
""");
addOptions("--experimental_remote_analysis_cache_mode=upload");
runtimeWrapper.newCommand(CqueryCommand.class);
buildTarget("//foo:A");
assertThat(
getCommandEnvironment()
.getRemoteAnalysisCachingEventListener()
.getSerializedKeysCount())
.isEqualTo(0);
}

protected void setupScenarioWithConfiguredTargets() throws Exception {
// ┌───────┐ ┌───────┐
// │ bar:C │ ◀── │ foo:A │
Expand Down

0 comments on commit 68f94a1

Please sign in to comment.