Preserve analysis cache through --run_under command changes

Along the way, also trim `CoverageOptions` as part of the test trimming configuration, matching the logic in `TestConfiguration#SHOULD_INVALIDATE_FOR_OPTION_DIFF`. Also refactor `RunUnder` into a sealed interface implemented by two records.

Work towards bazelbuild#3325
Fixes bazelbuild#10782

RELNOTES: Changing any part of `--run_under` that isn't the label (such as the shell command) no longer invalidates the analysis cache.

Closes bazelbuild#24303.

PiperOrigin-RevId: 696887935
Change-Id: I79a2c153862c33b2ff25eefa4069bc11e99ea9d6

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -1718,10 +1718,12 @@ java_library(
         ":config/run_under",
         ":config/starlark_defined_config_transition",
         ":platform_options",
+        ":test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/actions:action_environment",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:build_configuration_event",
         "//src/main/java/com/google/devtools/build/lib/actions:commandline_limits",
+        "//src/main/java/com/google/devtools/build/lib/analysis:test/test_trimming_logic",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
@@ -2061,6 +2063,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 
@@ -2825,9 +2828,11 @@ java_library(
     deps = [
         ":config/build_options",
         ":config/core_option_converters",
+        ":config/core_options",
         ":config/fragment",
         ":config/fragment_options",
         ":config/per_label_options",
+        ":config/run_under",
         ":options_diff_predicate",
         ":test/coverage_configuration",
         ":test/test_sharding_strategy",
@@ -2859,13 +2864,12 @@ java_library(
     deps = [
         ":analysis_cluster",
         ":config/build_options",
-        ":config/build_options_cache",
-        ":config/core_options",
         ":config/fragment_options",
         ":config/transitions/no_transition",
         ":config/transitions/patch_transition",
         ":config/transitions/transition_factory",
         ":test/test_configuration",
+        ":test/test_trimming_logic",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/packages",
@@ -2874,6 +2878,21 @@ java_library(
     ],
 )
 
+java_library(
+    name = "test/test_trimming_logic",
+    srcs = ["test/TestTrimmingLogic.java"],
+    deps = [
+        ":config/build_options",
+        ":config/build_options_cache",
+        ":config/core_options",
+        ":config/fragment_options",
+        ":config/run_under",
+        ":test/coverage_configuration",
+        ":test/test_configuration",
+        "//third_party:guava",
+    ],
+)
+
 java_library(
     name = "test/test_progress",
     srcs = ["test/TestProgress.java"],

src/main/java/com/google/devtools/build/lib/analysis/config/OutputPathMnemonicComputer.java

@@ -25,6 +25,8 @@
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.analysis.PlatformOptions;
+import com.google.devtools.build.lib.analysis.test.TestConfiguration;
+import com.google.devtools.build.lib.analysis.test.TestTrimmingLogic;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.server.FailureDetails.BuildConfiguration.Code;
 import com.google.devtools.build.lib.util.Fingerprint;
@@ -298,6 +300,10 @@ public static String computeNameFragmentWithDiff(
       return "";
     }
 
+    if (!toOptions.contains(TestConfiguration.TestOptions.class)) {
+      baselineOptions = TestTrimmingLogic.trim(baselineOptions);
+    }
+
     // TODO(blaze-configurability-team): As a mild performance update, getFirst already includes
     //   details of the corresponding option. Could incorporate this instead of hashChosenOptions
     //   regenerating the OptionDefinitions and values.
@@ -307,8 +313,8 @@ public static String computeNameFragmentWithDiff(
     //   trimmings. See longform note in {@link ConfiguredTargetKey} for details.
     ImmutableSet<String> chosenNativeOptions =
         diff.getFirst().keySet().stream()
-            .filter(optionDef -> !explicitInOutputPathOptions.contains(optionDef.getOptionName()))
             .map(OptionDefinition::getOptionName)
+            .filter(optionName -> !explicitInOutputPathOptions.contains(optionName))
             .collect(toImmutableSet());
     // Note: getChangedStarlarkOptions includes all changed options, added options and removed
     //   options between baselineOptions and toOptions. This is necessary since there is no current

src/main/java/com/google/devtools/build/lib/analysis/config/RunUnder.java

@@ -16,6 +16,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import javax.annotation.Nullable;
 
 /** Components of the {@code --run_under} option. */
 public sealed interface RunUnder {
@@ -30,6 +31,19 @@ public sealed interface RunUnder {
    */
   ImmutableList<String> options();
 
+  /**
+   * Returns a new instance that only retains the information that is relevant for the analysis of
+   * non-test targets.
+   */
+  @Nullable
+  static RunUnder trimForNonTestConfiguration(@Nullable RunUnder runUnder) {
+    return switch (runUnder) {
+      case LabelRunUnder labelRunUnder ->
+          new LabelRunUnder("", ImmutableList.of(), labelRunUnder.label());
+      case null, default -> null;
+    };
+  }
+
   /**
    * Represents a value of {@code --run_under} whose first word (according to shell tokenization)
    * starts with {@code "//"} or {@code "@"}. It is treated as a label referencing a target that

src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java

@@ -21,10 +21,12 @@
 import com.google.devtools.build.lib.analysis.OptionsDiffPredicate;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelConverter;
+import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.Fragment;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
 import com.google.devtools.build.lib.analysis.config.RequiresOptions;
+import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.analysis.test.CoverageConfiguration.CoverageOptions;
 import com.google.devtools.build.lib.analysis.test.TestShardingStrategy.ShardingStrategyConverter;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -44,6 +46,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
 /** Test-related options. */
 @RequiresOptions(options = {TestConfiguration.TestOptions.class})
@@ -54,13 +57,21 @@ public class TestConfiguration extends Fragment {
           // changes in --trim_test_configuration itself or related flags always prompt invalidation
           return true;
         }
+        // LINT.IfChange
         Class<? extends FragmentOptions> affectedOptionsClass =
             changedOption.getDeclaringClass(FragmentOptions.class);
         if (!affectedOptionsClass.equals(TestOptions.class)
             && !affectedOptionsClass.equals(CoverageOptions.class)) {
-          // options outside of TestOptions always prompt invalidation
+          // options outside of TestOptions always prompt invalidation, except for --run_under.
+          if (affectedOptionsClass.equals(CoreOptions.class)
+              && changedOption.getOptionName().equals("run_under")) {
+            return !Objects.equals(
+                RunUnder.trimForNonTestConfiguration((RunUnder) oldValue),
+                RunUnder.trimForNonTestConfiguration((RunUnder) newValue));
+          }
           return true;
         }
+        // LINT.ThenChange(TestTrimmingLogic.java)
         // other options in TestOptions require invalidation when --trim_test_configuration is off
         return !options.get(TestOptions.class).trimTestConfiguration;
       };

src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingLogic.java

@@ -0,0 +1,78 @@
+// Copyright 2024 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.analysis.test;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.BuildOptionsCache;
+import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
+import com.google.devtools.build.lib.analysis.config.CoreOptions;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
+import com.google.devtools.build.lib.analysis.config.RunUnder;
+import com.google.devtools.build.lib.analysis.test.CoverageConfiguration.CoverageOptions;
+import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
+
+/**
+ * Contains the pure logic for trimming test configuration from non-test targets that backs {@link
+ * com.google.devtools.build.lib.analysis.test.TestTrimmingTransitionFactory}.
+ */
+public final class TestTrimmingLogic {
+
+  static final ImmutableSet<Class<? extends FragmentOptions>> REQUIRED_FRAGMENTS =
+      ImmutableSet.of(CoreOptions.class, TestOptions.class);
+
+  // This cache is to prevent major slowdowns when using --trim_test_configuration. This
+  // transition is always invoked on every target in the top-level invocation. Thus, a wide
+  // invocation, like //..., will cause the transition to be invoked on a large number of targets
+  // leading to significant performance degradation. (Notably, the transition itself is somewhat
+  // fast; however, the post-processing of the BuildOptions into the actual BuildConfigurationValue
+  // takes a significant amount of time).
+  //
+  // Test any caching changes for performance impact in a longwide scenario with
+  // --trim_test_configuration on versus off.
+  // LINT.IfChange
+  private static final BuildOptionsCache<Boolean> CACHE =
+      new BuildOptionsCache<>(
+          (options, unused, unusedNonEventHandler) -> {
+            BuildOptions.Builder builder = options.underlying().toBuilder();
+            builder.removeFragmentOptions(TestOptions.class);
+            builder.removeFragmentOptions(CoverageOptions.class);
+            // Only the label of the --run_under target (if any) needs to be part of the
+            // configuration for non-test targets, all other information is directly obtained
+            // from the options in RunCommand.
+            CoreOptions coreOptions = builder.getFragmentOptions(CoreOptions.class);
+            coreOptions.runUnder = RunUnder.trimForNonTestConfiguration(coreOptions.runUnder);
+            return builder.build();
+          });
+
+  // LINT.ThenChange(TestConfiguration.java)
+
+  /** Returns a new {@link BuildOptions} instance with test configuration removed. */
+  public static BuildOptions trim(BuildOptions buildOptions) {
+    return trim(new BuildOptionsView(buildOptions, REQUIRED_FRAGMENTS));
+  }
+
+  /** Returns a new {@link BuildOptions} instance with test configuration removed. */
+  static BuildOptions trim(BuildOptionsView buildOptions) {
+    try {
+      return CACHE.applyTransition(buildOptions, Boolean.TRUE, /* eventHandler= */ null);
+    } catch (InterruptedException e) {
+      // The transition logic doesn't throw InterruptedException.
+      throw new IllegalStateException(e);
+    }
+  }
+
+  private TestTrimmingLogic() {}
+}

src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java

@@ -21,9 +21,7 @@
 import com.google.devtools.build.lib.analysis.AliasProvider;
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
-import com.google.devtools.build.lib.analysis.config.BuildOptionsCache;
 import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
-import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
@@ -37,7 +35,8 @@
 import com.google.devtools.common.options.Options;
 
 /**
- * Trimming transition factory which removes the test config fragment when entering a non-test rule.
+ * Trimming transition factory which removes the test config fragment and certain options that are
+ * only relevant for tests when entering a non-test rule.
  */
 public final class TestTrimmingTransitionFactory implements TransitionFactory<RuleTransitionData> {
 
@@ -67,24 +66,9 @@ private TestTrimmingTransition(boolean testonly) {
       this.testonly = testonly;
     }
 
-    // This cache is to prevent major slowdowns when using --trim_test_configuration. This
-    // transition is always invoked on every target in the top-level invocation. Thus, a wide
-    // invocation, like //..., will cause the transition to be invoked on a large number of targets
-    // leading to significant performance degradation. (Notably, the transition itself is somewhat
-    // fast; however, the post-processing of the BuildOptions into the actual
-    // BuildConfigurationValue
-    // takes a significant amount of time).
-    //
-    // Test any caching changes for performance impact in a longwide scenario with
-    // --trim_test_configuration on versus off.
-    private static final BuildOptionsCache<Boolean> cache =
-        new BuildOptionsCache<>(
-            (options, unused, unusedNonEventHandler) ->
-                options.underlying().toBuilder().removeFragmentOptions(TestOptions.class).build());
-
     @Override
     public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
-      return ImmutableSet.of(TestOptions.class, CoreOptions.class);
+      return TestTrimmingLogic.REQUIRED_FRAGMENTS;
     }
 
     @Override
@@ -101,7 +85,7 @@ public BuildOptions patch(BuildOptionsView originalOptions, EventHandler eventHa
         return originalOptions.underlying();
       }
       // No context needed, use the constant Boolean.TRUE.
-      return cache.applyTransition(originalOptions, Boolean.TRUE, null);
+      return TestTrimmingLogic.trim(originalOptions);
     }
   }
 

src/test/java/com/google/devtools/build/lib/analysis/starlark/BUILD

@@ -113,6 +113,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:platform_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:required_config_fragments_provider",
+        "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/packages",

src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProviderTest.java

@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.analysis.RequiredConfigFragmentsProvider;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
+import com.google.devtools.build.lib.analysis.test.CoverageConfiguration.CoverageOptions;
 import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.analysis.util.DummyTestFragment;
@@ -1353,11 +1354,12 @@ public void noopReturnValues(String returnLine) throws Exception {
 
         my_rule(name = "test")
         """);
-    // --trim_test_configuration means only the top-level configuration has TestOptions.
+    // --trim_test_configuration means only the top-level configuration has CoverageOptions and
+    // TestOptions.
     assertConfigurationsEqual(
         getConfiguration(getConfiguredTarget("//test")),
         targetConfig,
-        ImmutableSet.of(TestOptions.class));
+        ImmutableSet.of(CoverageOptions.class, TestOptions.class));
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java

@@ -1200,7 +1200,10 @@ def _rule_impl(ctx):
         my_rule(name = "foo")
         """);
     // TODO: b/293304174 - use a custom fragment instead of coverage
-    useConfiguration("--collect_code_coverage", "--coverage_output_generator=//my:tool");
+    useConfiguration(
+        "--collect_code_coverage",
+        "--coverage_output_generator=//my:tool",
+        "--notrim_test_configuration");
 
     StructImpl provider =
         getProvider("//subrule_testing:foo", "//subrule_testing:myrule.bzl", "MyInfo");

src/test/shell/integration/run_test.sh

@@ -703,6 +703,47 @@ EOF
   fi
 }
 
+function test_run_under_command_change_preserves_cache() {
+  if $is_windows; then
+    echo "This test requires --run_under to be able to run echo."
+    return
+  fi
+
+  local -r pkg="pkg${LINENO}"
+  mkdir -p "${pkg}"
+  cat > "$pkg/BUILD" <<'EOF'
+load(":defs.bzl", "my_rule")
+my_rule(
+  name = "my_rule",
+)
+EOF
+  cat > "$pkg/defs.bzl" <<'EOF'
+def _my_rule_impl(ctx):
+  print("my_rule is being analyzed")
+  out = ctx.actions.declare_file(ctx.label.name)
+  ctx.actions.write(out, "echo 'from rule'", is_executable = True)
+  return [DefaultInfo(executable = out)]
+
+my_rule = rule(
+  implementation = _my_rule_impl,
+  executable = True,
+)
+EOF
+
+  bazel run "${pkg}:my_rule" >$TEST_log 2>&1 \
+   || fail "expected run to pass"
+  expect_log "my_rule is being analyzed"
+  expect_log "from rule"
+  expect_not_log "from run_under"
+
+  # Use > to clear the previous log.
+  bazel run --run_under="echo 'from run_under' &&" "${pkg}:my_rule" >$TEST_log 2>&1 \
+   || fail "expected run to pass"
+  expect_not_log "my_rule is being analyzed"
+  expect_log "from rule"
+  expect_log "from run_under"
+}
+
 function test_build_id_env_var() {
   local -r pkg="pkg${LINENO}"
   mkdir -p "${pkg}"