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 5a630fcbf6b3b8..f2cf88382037c8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/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"], 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 c0b3b71ec0590a..ef4d3bdb49abfe 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,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 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 8dcf4620efba69..2add13d406437e 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,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 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 0c6080bc1214cf..7874dc6eb9ae1a 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,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 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; }; 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 new file mode 100644 index 00000000000000..a72574254c3d4b --- /dev/null +++ b/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> 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 879c50c5bc2875..adc5764f8dfcff 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,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 { @@ -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 cache = - new BuildOptionsCache<>( - (options, unused, unusedNonEventHandler) -> - options.underlying().toBuilder().removeFragmentOptions(TestOptions.class).build()); - @Override public ImmutableSet> 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); } } 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 22bea24aae1c07..3580ed8e6bdc99 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,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", 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 f6b083c41a33a0..56bc15f15f115d 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,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 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 3bc78375906d12..3f06eddcb7462d 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,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"); diff --git a/src/test/shell/integration/run_test.sh b/src/test/shell/integration/run_test.sh index 94503cdcc7ae9e..771d6ce3de5fb4 100755 --- a/src/test/shell/integration/run_test.sh +++ b/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}"