From 3e969ff24a6a0e03139b9f288c88451a7dfa97cd Mon Sep 17 00:00:00 2001 From: Philipp Schrader 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. + * + *

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. + * + *

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 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 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 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 <> target_skipping/BUILD <&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 <> target_skipping/BUILD <&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."