From f1153d2a18adadf63d2e9fea8be25a306dbe5c4e Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 19 Nov 2024 15:42:27 -0800 Subject: [PATCH] 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 --- .../google/devtools/build/lib/analysis/BUILD | 23 +----- .../config/OutputPathMnemonicComputer.java | 8 +- .../build/lib/analysis/config/RunUnder.java | 14 ---- .../lib/analysis/test/TestConfiguration.java | 13 +--- .../lib/analysis/test/TestTrimmingLogic.java | 78 ------------------- .../test/TestTrimmingTransitionFactory.java | 24 +++++- .../build/lib/analysis/starlark/BUILD | 1 - .../StarlarkRuleTransitionProviderTest.java | 6 +- .../starlark/StarlarkSubruleTest.java | 5 +- 9 files changed, 27 insertions(+), 145 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingLogic.java 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 c71bd737a76e6a..a7d5d302058d79 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 @@ 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"], 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 ef4d3bdb49abfe..c0b3b71ec0590a 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 @@ 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 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 2add13d406437e..8dcf4620efba69 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 { @@ -31,19 +30,6 @@ public sealed interface RunUnder { */ ImmutableList 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 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 7874dc6eb9ae1a..0c6080bc1214cf 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 @@ public class TestConfiguration extends Fragment { // changes in --trim_test_configuration itself or related flags always prompt invalidation return true; } - // LINT.IfChange Class 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 a72574254c3d4b..00000000000000 --- 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> 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 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 adc5764f8dfcff..879c50c5bc2875 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 { @@ -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 cache = + new BuildOptionsCache<>( + (options, unused, unusedNonEventHandler) -> + options.underlying().toBuilder().removeFragmentOptions(TestOptions.class).build()); + @Override public ImmutableSet> 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); } } 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 3580ed8e6bdc99..22bea24aae1c07 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 @@ 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", 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 18c8b4d8cedb8f..1d91f308ce390c 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 @@ 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 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 3f06eddcb7462d..3bc78375906d12 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 @@ 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");