Automated rollback of commit 4515bb6c932ce62c7889cf322319a3b49158acad.
*** 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
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
index c71bd73..a7d5d30 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -1720,12 +1720,10 @@
":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 @@
"//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 @@
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 @@
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",
@@ -2881,21 +2877,6 @@
)
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"],
deps = [
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputPathMnemonicComputer.java b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputPathMnemonicComputer.java
index ef4d3bd..c0b3b71 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputPathMnemonicComputer.java
+++ b/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 @@
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 @@
// 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
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/RunUnder.java b/src/main/java/com/google/devtools/build/lib/analysis/config/RunUnder.java
index 2add13d..8dcf462 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/RunUnder.java
+++ b/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 {
@@ -32,19 +31,6 @@
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
* should be used as the {@code --run_under} executable.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
index 7874dc6..0c6080b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
+++ b/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 @@
// 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;
};
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingLogic.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingLogic.java
deleted file mode 100644
index a725742..0000000
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingLogic.java
+++ /dev/null
@@ -1,78 +0,0 @@
-// 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() {}
-}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java
index adc5764..879c50c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java
+++ b/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 @@
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 @@
return originalOptions.underlying();
}
// No context needed, use the constant Boolean.TRUE.
- return TestTrimmingLogic.trim(originalOptions);
+ return cache.applyTransition(originalOptions, Boolean.TRUE, null);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/starlark/BUILD
index 3580ed8e..22bea24 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/BUILD
@@ -113,7 +113,6 @@
"//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",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProviderTest.java
index 18c8b4d..1d91f30 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProviderTest.java
+++ b/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 @@
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
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java
index 3f06edd..3bc7837 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkSubruleTest.java
@@ -1200,10 +1200,7 @@
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");
