From 43bcf80a3dfdc5ac89c1e4d615d6f29a495855fb Mon Sep 17 00:00:00 2001 From: ajurkowski Date: Wed, 12 Jan 2022 09:23:56 -0800 Subject: [PATCH] Disable implicitly collecting baseline coverage for toolchain targets. Toolchain targets can be evaluated with a [custom execution platform](https://github.com/bazelbuild/bazel/blob/b0a01af6fd0c5f3dce634cb827c75e818835e402/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java#L168) without affecting the configuration. In particular, we can have 2 `ToolchainDependencyConfiguredTargetKey` with the same label and configuration, but different `executionPlatformLabel`. When coverage is enabled, [every target](https://github.com/bazelbuild/bazel/blob/b0a01af6fd0c5f3dce634cb827c75e818835e402/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java#L160-L166) will have a [`baseline_coverage.dat` file generated with `BaselineCoverageAction`](https://github.com/bazelbuild/bazel/blob/33f7648fbaa875f73416be47f0b3c10ed93f30d6/src/main/java/com/google/devtools/build/lib/analysis/test/BaselineCoverageAction.java#L108-L114). This action will use the execution platform from the rule, meaning that in case of the same 2 toolchains, differing by execution platform only, we will create a coverage action with the same output (path based on configuration), but [different key](https://github.com/bazelbuild/bazel/blob/d657619c9c162c6e8c8f56b66e8bef4285d81944/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java#L65-L70). This fails in analysis since that constitutes a conflicting shared action. Disable coverage for toolchain targets to prevent actions from being created for those. Fixes #14521. PiperOrigin-RevId: 421317916 --- .../analysis/RuleConfiguredTargetBuilder.java | 4 +- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../skyframe/ToolchainsForTargetsTest.java | 60 ++++++++++++++++++- 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index accd20941edddd..f7e147a74ef6ec 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.analysis.constraints.SupportedEnvironments; import com.google.devtools.build.lib.analysis.constraints.SupportedEnvironmentsProvider; import com.google.devtools.build.lib.analysis.constraints.SupportedEnvironmentsProvider.RemovedEnvironmentCulprit; +import com.google.devtools.build.lib.analysis.platform.ToolchainInfo; import com.google.devtools.build.lib.analysis.test.AnalysisTestActionBuilder; import com.google.devtools.build.lib.analysis.test.AnalysisTestResultInfo; import com.google.devtools.build.lib.analysis.test.ExecutionInfo; @@ -161,7 +162,8 @@ public ConfiguredTarget build() throws ActionConflictException, InterruptedExcep // rule doesn't configure InstrumentedFilesInfo. This needs to be done for non-test rules // as well, but should be done before initializeTestProvider, which uses that. if (ruleContext.getConfiguration().isCodeCoverageEnabled() - && !providersBuilder.contains(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR.getKey())) { + && !providersBuilder.contains(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR.getKey()) + && !providersBuilder.contains(ToolchainInfo.PROVIDER.getKey())) { addNativeDeclaredProvider(InstrumentedFilesCollector.forwardAll(ruleContext)); } // Create test action and artifacts if target was successfully initialized diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index e900a849361b99..1c7fda0ef0412a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -151,6 +151,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/analysis:target_and_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_collection", + "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_context", "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context", "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider", "//src/main/java/com/google/devtools/build/lib/analysis:view_creation_failed_exception", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java index 76c6a9a2694de6..15ff014850c88f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java @@ -13,15 +13,23 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.analysis.testing.ToolchainCollectionSubject.assertThat; +import static com.google.devtools.build.lib.analysis.testing.ToolchainContextSubject.assertThat; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.TargetAndConfiguration; import com.google.devtools.build.lib.analysis.ToolchainCollection; +import com.google.devtools.build.lib.analysis.ToolchainContext; +import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; +import com.google.devtools.build.lib.analysis.test.BaselineCoverageAction; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.AnalysisTestCase; import com.google.devtools.build.lib.cmdline.Label; @@ -57,7 +65,7 @@ * doesn't support direct access to environments. */ @RunWith(JUnit4.class) -public class ToolchainsForTargetsTest extends AnalysisTestCase { +public final class ToolchainsForTargetsTest extends AnalysisTestCase { /** Returns a {@link SkyKey} for a given pair. */ private static Key key( TargetAndConfiguration targetAndConfiguration, ConfiguredTargetKey configuredTargetKey) { @@ -475,4 +483,54 @@ public void keepParentToolchainContext() throws Exception { .defaultToolchainContext() .hasExecutionPlatform("//platforms:local_platform_b"); } + + /** Regression test for b/214105142, https://github.com/bazelbuild/bazel/issues/14521 */ + @Test + public void toolchainWithDifferentExecutionPlatforms_doesNotGenerateConflictingCoverageAction() + throws Exception { + scratch.file( + "platforms/BUILD", + "constraint_setting(name = 'local_setting')", + "constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')", + "constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')", + "platform(name = 'local_platform_a', constraint_values = [':local_value_a'])", + "platform(name = 'local_platform_b', constraint_values = [':local_value_b'])"); + scratch.file( + "a/BUILD", + "load('//toolchain:rule.bzl', 'my_rule')", + "my_rule(name='a', exec_compatible_with=['//platforms:local_value_a'])", + "my_rule(name='b', exec_compatible_with=['//platforms:local_value_b'])"); + useConfiguration( + "--collect_code_coverage", + "--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b"); + + update("//a:a", "//a:b"); + + // Sanity check that a coverage action was generated for the rule itself. + assertHasBaselineCoverageAction("//a:a", "Writing file a/a/baseline_coverage.dat"); + assertHasBaselineCoverageAction("//a:b", "Writing file a/b/baseline_coverage.dat"); + assertThat(getActions("//toolchains:toolchain_1_impl")).isEmpty(); + ToolchainContext toolchainAContext = + getToolchainCollection("//a:a").getDefaultToolchainContext(); + assertThat(toolchainAContext).hasExecutionPlatform("//platforms:local_platform_a"); + assertThat(toolchainAContext).hasToolchainType("//toolchain:test_toolchain"); + assertThat(toolchainAContext).hasResolvedToolchain("//toolchains:toolchain_1_impl"); + ToolchainContext toolchainBContext = + getToolchainCollection("//a:b").getDefaultToolchainContext(); + assertThat(toolchainBContext).hasExecutionPlatform("//platforms:local_platform_b"); + assertThat(toolchainBContext).hasToolchainType("//toolchain:test_toolchain"); + assertThat(toolchainBContext).hasResolvedToolchain("//toolchains:toolchain_1_impl"); + } + + private void assertHasBaselineCoverageAction(String label, String progressMessage) + throws InterruptedException { + Action coverageAction = Iterables.getOnlyElement(getActions(label)); + assertThat(coverageAction).isInstanceOf(BaselineCoverageAction.class); + assertThat(coverageAction.getProgressMessage()).isEqualTo(progressMessage); + } + + private ImmutableList getActions(String label) throws InterruptedException { + return ((RuleConfiguredTarget) getConfiguredTarget(label)) + .getActions().stream().map(Action.class::cast).collect(toImmutableList()); + } }