Add --experimental_retain_test_configuration_across_testonly

Action conflicts continue to be an issue when non-test (e.g. genquery, filegroup, etc.) depends on cc_test.

This flag averts the issue by skipping trimming when testonly is true (and testonly must already be true for a non-test target to depend on a test). This does mean you only get the performance gain for non-testonly targets; however, this is still sufficiently substantial.

The expected progression of this flag is False -(soon-after-release)-> True -(eventually-after-depot-migration)-> False -> gone/deprecated.

Alternative name was experimental_trim_test_configuration_ignores_testonly with reversed behavior (flag True is pre-CL behavior) which would have incurred a flag progression of False (default, causing immediate behavior change on release) -(eventually-after-depot-migration)-> True -> gone/deprecated.

PiperOrigin-RevId: 372479312

Author: twigg

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

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.analysis.test;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.analysis.OptionsDiffPredicate;
@@ -46,8 +47,8 @@
 public class TestConfiguration extends Fragment {
   public static final OptionsDiffPredicate SHOULD_INVALIDATE_FOR_OPTION_DIFF =
       (options, changedOption, oldValue, newValue) -> {
-        if (TestOptions.TRIM_TEST_CONFIGURATION.equals(changedOption)) {
-          // changes in --trim_test_configuration itself always prompt invalidation
+        if (TestOptions.ALWAYS_INVALIDATE_WHEN_CHANGED.contains(changedOption)) {
+          // changes in --trim_test_configuration itself or related flags always prompt invalidation
           return true;
         }
         if (!changedOption.getField().getDeclaringClass().equals(TestOptions.class)) {
@@ -60,8 +61,11 @@ public class TestConfiguration extends Fragment {
 
   /** Command-line options. */
   public static class TestOptions extends FragmentOptions {
-    private static final OptionDefinition TRIM_TEST_CONFIGURATION =
-        OptionsParser.getOptionDefinitionByName(TestOptions.class, "trim_test_configuration");
+    private static final ImmutableSet<OptionDefinition> ALWAYS_INVALIDATE_WHEN_CHANGED =
+        ImmutableSet.of(
+            OptionsParser.getOptionDefinitionByName(TestOptions.class, "trim_test_configuration"),
+            OptionsParser.getOptionDefinitionByName(
+                TestOptions.class, "experimental_retain_test_configuration_across_testonly"));
 
     @Option(
         name = "test_timeout",
@@ -140,6 +144,21 @@ public static class TestOptions extends FragmentOptions {
                 + " re-analyzed.")
     public boolean trimTestConfiguration;
 
+    @Option(
+        name = "experimental_retain_test_configuration_across_testonly",
+        defaultValue = "false",
+        documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION,
+        effectTags = {
+          OptionEffectTag.LOADING_AND_ANALYSIS,
+          OptionEffectTag.LOSES_INCREMENTAL_STATE,
+        },
+        help =
+            "When enabled, --trim_test_configuration will not trim the test configuration for rules"
+                + " marked testonly=1. This is meant to reduce action conflict issues when non-test"
+                + " rules depend on cc_test rules. No effect if --trim_test_configuration is"
+                + " false.")
+    public boolean experimentalRetainTestConfigurationAcrossTestonly;
+
     @Option(
         name = "test_arg",
         allowMultiple = true,
@@ -291,6 +310,8 @@ public FragmentOptions getHost() {
       hostOptions.coverageReportGenerator = this.coverageReportGenerator;
       // trimTestConfiguration is a global analysis option and should be platform-agnostic
       hostOptions.trimTestConfiguration = this.trimTestConfiguration;
+      hostOptions.experimentalRetainTestConfigurationAcrossTestonly =
+          this.experimentalRetainTestConfigurationAcrossTestonly;
       return hostOptions;
     }
   }

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

@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis.test;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.BuildOptionsCache;
@@ -24,9 +25,11 @@
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
 import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.TargetUtils;
+import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.common.options.Options;
 
 /**
@@ -39,15 +42,29 @@ public final class TestTrimmingTransitionFactory implements TransitionFactory<Ru
 
   /**
    * Trimming transition which removes the test config fragment if --trim_test_configuration is on.
+   *
+   * <p>At the moment, need to know the value of the testonly attribute from the underlying rule. So
+   * the factory, which has access to attributes but not the configuration, attaches the appropriate
+   * TestTrimmingTransition, which will have access to the configuration.
    */
-  public enum TestTrimmingTransition implements PatchTransition {
-    INSTANCE;
+  public static class TestTrimmingTransition implements PatchTransition {
+    // These are essentially a cache of the two versions of the transition depending on if
+    // the associated rule is testonly = true or not.
+    private static final TestTrimmingTransition TESTONLY_TRUE = new TestTrimmingTransition(true);
+    private static final TestTrimmingTransition TESTONLY_FALSE = new TestTrimmingTransition(false);
+    @VisibleForTesting public static final TestTrimmingTransition INSTANCE = TESTONLY_FALSE;
+
+    private final boolean testonly;
+
+    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 results into a BuildConfiguration
+    // fast; however, the post-processing of the BuildOptions into the actual BuildConfiguration
     // takes a significant amount of time).
     private static final BuildOptionsCache<Integer> cache = new BuildOptionsCache<>();
 
@@ -65,6 +82,7 @@ public BuildOptions patch(BuildOptionsView originalOptions, EventHandler eventHa
       CoreOptions originalCoreOptions = originalOptions.get(CoreOptions.class);
       TestOptions originalTestOptions = originalOptions.get(TestOptions.class);
       if (!originalTestOptions.trimTestConfiguration
+          || (originalTestOptions.experimentalRetainTestConfigurationAcrossTestonly && testonly)
           || !originalCoreOptions.useDistinctHostConfiguration) {
         // nothing to do, trimming is disabled
         // Due to repercussions of b/117932061, do not trim when `--nodistinct_host_configuration`
@@ -103,6 +121,13 @@ public PatchTransition create(Rule rule) {
     }
 
     // Non-test rule. Trim it!
-    return TestTrimmingTransition.INSTANCE;
+    // Use an attribute mapper to ensure testonly is resolved to an actual boolean value.
+    // It is expected all rules should have a boolean testonly value so the `has` check is only
+    //   there as an over-abundance of caution.
+    NonconfigurableAttributeMapper attrs = NonconfigurableAttributeMapper.of(rule);
+    if (attrs.has("testonly", Type.BOOLEAN) && attrs.get("testonly", Type.BOOLEAN)) {
+      return TestTrimmingTransition.TESTONLY_TRUE;
+    }
+    return TestTrimmingTransition.TESTONLY_FALSE;
   }
 }

src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java

@@ -824,6 +824,49 @@ public void flagOnTestSuiteWithTestDependencies_CanBeAnalyzed() throws Exception
     assertThat(getAnalysisResult().getTargetsToBuild()).hasSize(2);
   }
 
+  @Test
+  public void flagOnNonTestTargetWithTestDependencies_isTrimmed() throws Exception {
+    scratch.file(
+        "test/BUILD",
+        "load(':test.bzl', 'starlark_test')",
+        "load(':lib.bzl', 'starlark_lib')",
+        "starlark_lib(",
+        "    name = 'starlark_dep',",
+        "    deps = [':starlark_test'],",
+        "    testonly = 1,",
+        ")",
+        "starlark_test(",
+        "    name = 'starlark_test',",
+        ")");
+    useConfiguration(
+        "--trim_test_configuration", "--noexperimental_retain_test_configuration_across_testonly");
+    update("//test:starlark_dep");
+    ConfiguredTarget top = getConfiguredTarget("//test:starlark_dep");
+    assertThat(getConfiguration(top).hasFragment(TestConfiguration.class)).isFalse();
+  }
+
+  @Test
+  public void flagOnNonTestTargetWithTestDependencies_isNotTrimmedWithExperimentalFlag()
+      throws Exception {
+    scratch.file(
+        "test/BUILD",
+        "load(':test.bzl', 'starlark_test')",
+        "load(':lib.bzl', 'starlark_lib')",
+        "starlark_lib(",
+        "    name = 'starlark_dep',",
+        "    deps = [':starlark_test'],",
+        "    testonly = 1,",
+        ")",
+        "starlark_test(",
+        "    name = 'starlark_test',",
+        ")");
+    useConfiguration(
+        "--trim_test_configuration", "--experimental_retain_test_configuration_across_testonly");
+    update("//test:starlark_dep");
+    ConfiguredTarget top = getConfiguredTarget("//test:starlark_dep");
+    assertThat(getConfiguration(top).hasFragment(TestConfiguration.class)).isTrue();
+  }
+
   // Test Starlark API of AnalysisFailure{,Info}.
   @Test
   public void testAnalysisFailureInfo() throws Exception {