From 3e969ff24a6a0e03139b9f288c88451a7dfa97cd Mon Sep 17 00:00:00 2001
From: Philipp Schrader <philipp.schrader@gmail.com>
Date: Thu, 10 Dec 2020 09:05:37 -0800
Subject: [PATCH] Fix a couple of bugs with Incompatible Target Skipping
While adding `target_compatible_with` attributes in the FRC team 971's
code base I came across bug #12553. It wasn't possible to make a
Python target depend on another incompatible Python target.
This patch fixes that issue by teaching `RuleContext` about
incompatible prerequisites. This also fixes an issue with the
validation of file extensions.
It's possible that `validateDirectPrerequisite()` skips a bit too much
validation. It was unfortunately the cleanest way I could think of.
I struggled a bit to come up with what ended up becoming
`RuleContextConstraintSemantics.IncompatibleCheckResult`. The purpose
of that class is to centralize the logic for checking for
`OutputFileConfiguredTarget` dependencies. Such dependencies need a
bit of special logic in order to find `IncompatiblePlatformProvider`
instances. When I implemented this patch, I realized that the
`TopLevelConstraintSemantics` logic had a very similar problem. It
didn't deal with the `OutputFileConfiguredTarget` problem at all. I
took the liberty of fixing that issue in this patch also because it
nicely re-used the same new helper.
I could not figure out a good way to avoid making `RuleContext` depend
on `RuleContextConstraintSemantics`.
Fixes #12553
Closes #12560.
PiperOrigin-RevId: 346796174
---
.../build/lib/analysis/RuleContext.java | 9 ++
.../RuleContextConstraintSemantics.java | 62 ++++++++---
.../TopLevelConstraintSemantics.java | 10 +-
.../target_compatible_with_test.sh | 102 +++++++++++++++++-
4 files changed, 162 insertions(+), 21 deletions(-)
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 26a039416aac92..8c8251b501b499 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -54,6 +54,7 @@
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics;
+import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
@@ -2306,6 +2307,14 @@ private boolean checkRuleDependencyMandatoryProviders(
private void validateDirectPrerequisite(
Attribute attribute, ConfiguredTargetAndData prerequisite) {
+ if (RuleContextConstraintSemantics.checkForIncompatibility(prerequisite.getConfiguredTarget())
+ .isIncompatible()) {
+ // If the prerequisite is incompatible (e.g. has an incompatible provider), we pretend that
+ // there is no further validation needed. Otherwise, it would be difficult to make the
+ // incompatible target satisfy things like required providers and file extensions.
+ return;
+ }
+
validateDirectPrerequisiteType(prerequisite, attribute);
validateDirectPrerequisiteFileTypes(prerequisite, attribute);
if (attribute.performPrereqValidatorCheck()) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
index 68697aed40cf8c..7323298c504500 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
@@ -17,6 +17,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.Streams.stream;
+import com.google.auto.value.AutoValue;
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.base.Verify;
@@ -830,6 +831,45 @@ private static void addSelectValuesToSet(BuildType.Selector<?> select, final Set
}
}
+ /**
+ * Provides information about a target's incompatibility.
+ *
+ * <p>After calling {@code checkForIncompatibility()}, the {@code isIncompatible} getter tells you
+ * whether the target is incompatible. If the target is incompatible, then {@code
+ * underlyingTarget} tells you which underlying target provided the incompatibility. For the vast
+ * majority of targets this is the same one passed to {@code checkForIncompatibility()}. In some
+ * instances like {@link OutputFileConfiguredTarget}, however, the {@code underlyingTarget} is the
+ * rule that generated the file.
+ */
+ @AutoValue
+ public abstract static class IncompatibleCheckResult {
+ private static IncompatibleCheckResult create(
+ boolean isIncompatible, ConfiguredTarget underlyingTarget) {
+ return new AutoValue_RuleContextConstraintSemantics_IncompatibleCheckResult(
+ isIncompatible, underlyingTarget);
+ }
+
+ public abstract boolean isIncompatible();
+
+ public abstract ConfiguredTarget underlyingTarget();
+ }
+
+ /**
+ * Checks whether the target is incompatible.
+ *
+ * <p>See the documentation for {@link RuleContextConstraintSemantics.IncompatibleCheckResult} for
+ * more information.
+ */
+ public static IncompatibleCheckResult checkForIncompatibility(ConfiguredTarget target) {
+ if (target instanceof OutputFileConfiguredTarget) {
+ // For generated files, we want to query the generating rule for providers. genrule() for
+ // example doesn't attach providers like IncompatiblePlatformProvider to its outputs.
+ target = ((OutputFileConfiguredTarget) target).getGeneratingRule();
+ }
+ return IncompatibleCheckResult.create(
+ target.getProvider(IncompatiblePlatformProvider.class) != null, target);
+ }
+
/**
* Creates an incompatible {@link ConfiguredTarget} if the corresponding rule is incompatible.
*
@@ -870,22 +910,12 @@ public static ConfiguredTarget incompatibleConfiguredTarget(
}
// This is incompatible if one of the dependencies is as well.
- ImmutableList.Builder<ConfiguredTarget> incompatibleDependenciesBuilder =
- ImmutableList.builder();
- for (ConfiguredTargetAndData infoCollection : prerequisiteMap.values()) {
- ConfiguredTarget dependency = infoCollection.getConfiguredTarget();
- if (dependency instanceof OutputFileConfiguredTarget) {
- // For generated files, we want to query the generating rule for providers. genrule() for
- // example doesn't attach providers like IncompatiblePlatformProvider to its outputs.
- dependency = ((OutputFileConfiguredTarget) dependency).getGeneratingRule();
- }
- if (dependency.getProvider(IncompatiblePlatformProvider.class) != null) {
- incompatibleDependenciesBuilder.add(dependency);
- }
- }
-
ImmutableList<ConfiguredTarget> incompatibleDependencies =
- incompatibleDependenciesBuilder.build();
+ prerequisiteMap.values().stream()
+ .map(value -> checkForIncompatibility(value.getConfiguredTarget()))
+ .filter(result -> result.isIncompatible())
+ .map(result -> result.underlyingTarget())
+ .collect(toImmutableList());
if (!incompatibleDependencies.isEmpty()) {
return createIncompatibleConfiguredTarget(ruleContext, incompatibleDependencies, null);
}
@@ -987,7 +1017,7 @@ private static ConfiguredTarget createIncompatibleConfiguredTarget(
new FailAction(
ruleContext.getActionOwner(),
outputArtifacts,
- "Can't build this. This target is incompatible.",
+ "Can't build this. This target is incompatible. Please file a bug upstream.",
Code.CANT_BUILD_INCOMPATIBLE_TARGET));
}
return builder.build();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java
index 29e91224c2d214..06e658f8f0fd68 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java
@@ -123,11 +123,11 @@ public PlatformRestrictionsResult checkPlatformRestrictions(
ImmutableSet.Builder<ConfiguredTarget> incompatibleButRequestedTargets = ImmutableSet.builder();
for (ConfiguredTarget target : topLevelTargets) {
- IncompatiblePlatformProvider incompatibleProvider =
- target.getProvider(IncompatiblePlatformProvider.class);
+ RuleContextConstraintSemantics.IncompatibleCheckResult result =
+ RuleContextConstraintSemantics.checkForIncompatibility(target);
// Move on to the next target if this one is compatible.
- if (incompatibleProvider == null) {
+ if (!result.isIncompatible()) {
continue;
}
@@ -143,7 +143,9 @@ public PlatformRestrictionsResult checkPlatformRestrictions(
String.format(
TARGET_INCOMPATIBLE_ERROR_TEMPLATE,
target.getLabel().toString(),
- reportOnIncompatibility(target));
+ // We need access to the provider so we pass in the underlying target here that is
+ // responsible for the incompatibility.
+ reportOnIncompatibility(result.underlyingTarget()));
throw new ViewCreationFailedException(
targetIncompatibleMessage,
FailureDetail.newBuilder()
diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh
index 34e42b83752b69..fd14f2ac9452bd 100755
--- a/src/test/shell/integration/target_compatible_with_test.sh
+++ b/src/test/shell/integration/target_compatible_with_test.sh
@@ -447,6 +447,92 @@ EOF
expect_log 'FAILED: Build did NOT complete successfully'
}
+# Validates that rules with custom providers are skipped when incompatible.
+# This is valuable because we use providers to convey incompatibility.
+function test_dependencies_with_providers() {
+ cat > target_skipping/rules.bzl <<EOF
+DummyProvider = provider()
+
+def _dummy_rule_impl(ctx):
+ return [DummyProvider()]
+
+dummy_rule = rule(
+ implementation = _dummy_rule_impl,
+ attrs = {
+ "deps": attr.label_list(providers=[DummyProvider]),
+ },
+)
+EOF
+
+ cat >> target_skipping/BUILD <<EOF
+load("//target_skipping:rules.bzl", "dummy_rule")
+
+dummy_rule(
+ name = "dummy1",
+ target_compatible_with = [
+ "//target_skipping:foo1",
+ ],
+)
+
+dummy_rule(
+ name = "dummy2",
+ deps = [
+ ":dummy1",
+ ],
+)
+EOF
+
+ cd target_skipping || fail "couldn't cd into workspace"
+
+ pwd >&2
+ bazel build \
+ --show_result=10 \
+ --host_platform=@//target_skipping:foo3_platform \
+ --platforms=@//target_skipping:foo3_platform \
+ //target_skipping/... &> "${TEST_log}" || fail "Bazel failed unexpectedly."
+ expect_log '^Target //target_skipping:dummy2 was skipped'
+}
+
+function test_dependencies_with_extensions() {
+ cat > target_skipping/rules.bzl <<EOF
+def _dummy_rule_impl(ctx):
+ out = ctx.actions.declare_file(ctx.attr.name + ".cc")
+ ctx.actions.write(out, "Dummy content")
+ return DefaultInfo(files = depset([out]))
+
+dummy_rule = rule(
+ implementation = _dummy_rule_impl,
+)
+EOF
+
+ cat >> target_skipping/BUILD <<EOF
+load("//target_skipping:rules.bzl", "dummy_rule")
+
+# Generates a dummy.cc file.
+dummy_rule(
+ name = "dummy_file",
+ target_compatible_with = [":foo1"],
+)
+
+cc_library(
+ name = "dummy_cc_lib",
+ srcs = [
+ "dummy_file",
+ ],
+)
+EOF
+
+ cd target_skipping || fail "couldn't cd into workspace"
+
+ pwd >&2
+ bazel build \
+ --show_result=10 \
+ --host_platform=@//target_skipping:foo3_platform \
+ --platforms=@//target_skipping:foo3_platform \
+ //target_skipping/... &> "${TEST_log}" || fail "Bazel failed unexpectedly."
+ expect_log '^Target //target_skipping:dummy_cc_lib was skipped'
+}
+
# Validates the same thing as test_non_top_level_skipping, but with a cc_test
# and adding one more level of dependencies.
function test_cc_test() {
@@ -480,6 +566,21 @@ EOF
cd target_skipping || fail "couldn't cd into workspace"
+ # Validate the generated file that makes up the test.
+ bazel test \
+ --show_result=10 \
+ --host_platform=@//target_skipping:foo2_bar1_platform \
+ --platforms=@//target_skipping:foo2_bar1_platform \
+ //target_skipping:generated_test.cc &> "${TEST_log}" && fail "Bazel passed unexpectedly."
+ expect_log "ERROR: Target //target_skipping:generated_test.cc is incompatible and cannot be built, but was explicitly requested"
+
+ # Validate that we get the dependency chain printed out.
+ expect_log '^Dependency chain:$'
+ expect_log '^ //target_skipping:generate_with_tool$'
+ expect_log "^ //target_skipping:generator_tool <-- target platform didn't satisfy constraint //target_skipping:foo1$"
+ expect_log 'FAILED: Build did NOT complete successfully'
+
+ # Validate the test.
bazel test \
--show_result=10 \
--host_platform=@//target_skipping:foo2_bar1_platform \
@@ -727,7 +828,6 @@ EOF
bazel test --show_result=10 \
--host_platform=@//target_skipping:foo3_platform \
- --toolchain_resolution_debug \
--platforms=@//target_skipping:foo3_platform \
//target_skipping:foo3_analysistest_test &> "${TEST_log}" \
|| fail "Bazel failed unexpectedly."