Automated rollback of commit 4515bb6.

*** Reason for rollback ***

Trimming CoverageOptions causes and transitions on coverage flags to fail unexpectedly. Starlark transitions have no API to determine if a flag is valid in the current configuration, and because of trimming this may not be a static list.

*** Original change description ***

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 #3325

PiperOrigin-RevId: 698169645
Change-Id: I9b69ad7c275d6b2d65f1cd5d5ea9941bdc9cf42c

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

@@ -1720,12 +1720,10 @@ 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",
@@ -2065,7 +2063,6 @@ 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",
     ],
 )
 
@@ -2830,11 +2827,9 @@ 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",
@@ -2866,12 +2861,13 @@ 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",
@@ -2880,21 +2876,6 @@ 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,8 +25,6 @@
 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;
@@ -300,10 +298,6 @@ 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.
@@ -313,8 +307,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,7 +16,6 @@
 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 {
@@ -31,19 +30,6 @@ 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,12 +21,10 @@
 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;
@@ -46,7 +44,6 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 
 /** Test-related options. */
 @RequiresOptions(options = {TestConfiguration.TestOptions.class})
@@ -57,21 +54,13 @@ 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, 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));
-          }
+          // options outside of TestOptions always prompt invalidation
           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/TestTrimmingTransitionFactory.java

@@ -21,7 +21,9 @@
 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;
@@ -35,8 +37,7 @@
 import com.google.devtools.common.options.Options;
 
 /**
- * Trimming transition factory which removes the test config fragment and certain options that are
- * only relevant for tests when entering a non-test rule.
+ * Trimming transition factory which removes the test config fragment when entering a non-test rule.
  */
 public final class TestTrimmingTransitionFactory implements TransitionFactory<RuleTransitionData> {
 
@@ -66,9 +67,24 @@ 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 TestTrimmingLogic.REQUIRED_FRAGMENTS;
+      return ImmutableSet.of(TestOptions.class, CoreOptions.class);
     }
 
     @Override
@@ -85,7 +101,7 @@ public BuildOptions patch(BuildOptionsView originalOptions, EventHandler eventHa
         return originalOptions.underlying();
       }
       // No context needed, use the constant Boolean.TRUE.
-      return TestTrimmingLogic.trim(originalOptions);
+      return cache.applyTransition(originalOptions, Boolean.TRUE, null);
     }
   }
 

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

@@ -113,7 +113,6 @@ 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,7 +27,6 @@
 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;
@@ -1354,12 +1353,11 @@ public void noopReturnValues(String returnLine) throws Exception {
 
         my_rule(name = "test")
         """);
-    // --trim_test_configuration means only the top-level configuration has CoverageOptions and
-    // TestOptions.
+    // --trim_test_configuration means only the top-level configuration has TestOptions.
     assertConfigurationsEqual(
         getConfiguration(getConfiguredTarget("//test")),
         targetConfig,
-        ImmutableSet.of(CoverageOptions.class, TestOptions.class));
+        ImmutableSet.of(TestOptions.class));
   }
 
   @Test

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

@@ -1200,10 +1200,7 @@ 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",
-        "--notrim_test_configuration");
+    useConfiguration("--collect_code_coverage", "--coverage_output_generator=//my:tool");
 
     StructImpl provider =
         getProvider("//subrule_testing:foo", "//subrule_testing:myrule.bzl", "MyInfo");