From 68f94a1d6f2eede5552fd52986a74b6e9ecaa1b6 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 24 Oct 2024 08:47:00 -0700 Subject: [PATCH] Enable analysis caching codepaths only for commands that execute actions. 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 --- .../lib/buildtool/AnalysisPhaseRunner.java | 3 +- .../build/lib/buildtool/BuildTool.java | 3 +- .../lib/skyframe/serialization/analysis/BUILD | 1 + .../analysis/FrontierSerializerTestBase.java | 32 +++++++++++++++++++ 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java index b0ce8b443f4ae0..c15e3378ef6cd2 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java @@ -208,7 +208,8 @@ static ProjectEvaluationResult evaluateProjectFile( throws LoadingFailedException, InvalidConfigurationException { EnumSet 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); diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java index 53333e122afda1..1718530ca76d92 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java @@ -850,7 +850,7 @@ private void logAnalysisCachingStatsAndMaybeUploadFrontier( } RemoteAnalysisCachingOptions options = request.getOptions(RemoteAnalysisCachingOptions.class); - if (options == null) { + if (options == null || !env.getCommand().buildPhase().executes()) { return; } @@ -1108,6 +1108,7 @@ public static RemoteAnalysisCachingDependenciesProvider forAnalysis( CommandEnvironment env, Optional activeDirectoriesMatcher) { var options = env.getOptions().getOptions(RemoteAnalysisCachingOptions.class); if (options == null + || !env.getCommand().buildPhase().executes() || !options.mode.downloadForAnalysis() || activeDirectoriesMatcher.isEmpty()) { return DisabledDependenciesProvider.INSTANCE; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/BUILD index e062e577df2d5a..ee487740f87020 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/BUILD @@ -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", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializerTestBase.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializerTestBase.java index edee0161326b13..d74be6943c2f7f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializerTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializerTestBase.java @@ -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; @@ -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 │