Skip to content

Commit

Permalink
Automated rollback of commit 650142f.
Browse files Browse the repository at this point in the history
*** Reason for rollback ***

Rollforward with fixes

Preserve analysis cache through `--run_under` command changes

Work towards #3325
Fixes #10782

RELNOTES: Changing any part of --run_under that isn't the label (such as the shell command) no longer invalidates the analysis cache.

Closes #24411.

PiperOrigin-RevId: 703516108
Change-Id: Ief9bd2547fe87b4c09b132ada87aed369753e550
  • Loading branch information
fmeum authored and copybara-github committed Dec 6, 2024
1 parent d93c1cb commit 45d4cff
Show file tree
Hide file tree
Showing 7 changed files with 173 additions and 24 deletions.
22 changes: 20 additions & 2 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1720,6 +1720,8 @@ java_library(
":config/run_under",
":config/starlark_defined_config_transition",
":platform_options",
":test/test_configuration",
":test/test_trimming_logic",
"//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",
Expand Down Expand Up @@ -2063,6 +2065,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",
],
)

Expand Down Expand Up @@ -2817,9 +2820,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",
Expand Down Expand Up @@ -2851,13 +2856,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",
Expand All @@ -2866,6 +2870,20 @@ 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/test_configuration",
"//third_party:guava",
],
)

java_library(
name = "test/test_progress",
srcs = ["test/TestProgress.java"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -307,8 +313,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -30,6 +31,19 @@ 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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})
Expand All @@ -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<? extends FragmentOptions> 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;
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// 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.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);
// 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() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<RuleTransitionData> {

Expand Down Expand Up @@ -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<Boolean> cache =
new BuildOptionsCache<>(
(options, unused, unusedNonEventHandler) ->
options.underlying().toBuilder().removeFragmentOptions(TestOptions.class).build());

@Override
public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
return ImmutableSet.of(TestOptions.class, CoreOptions.class);
return TestTrimmingLogic.REQUIRED_FRAGMENTS;
}

@Override
Expand All @@ -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);
}
}

Expand Down
40 changes: 40 additions & 0 deletions src/test/shell/integration/run_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,46 @@ 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 -n world", 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_not_log "hello"
expect_log "world"

bazel run --run_under="echo -n hello &&" "${pkg}:my_rule" >$TEST_log 2>&1 \
|| fail "expected run to pass"
expect_not_log "my_rule is being analyzed"
expect_log "hello"
expect_log "world"
}

function test_build_id_env_var() {
local -r pkg="pkg${LINENO}"
mkdir -p "${pkg}"
Expand Down

0 comments on commit 45d4cff

Please sign in to comment.