Skip to content

Commit

Permalink
Fix a couple of bugs with Incompatible Target Skipping
Browse files Browse the repository at this point in the history
While adding `target_compatible_with` attributes in the FRC team 971's
code base I came across bug bazelbuild#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 bazelbuild#12553

Closes bazelbuild#12560.

PiperOrigin-RevId: 346796174
  • Loading branch information
philsc authored and copybara-github committed Dec 10, 2020
1 parent 230be16 commit 3e969ff
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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()
Expand Down
102 changes: 101 additions & 1 deletion src/test/shell/integration/target_compatible_with_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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 \
Expand Down Expand Up @@ -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."
Expand Down

0 comments on commit 3e969ff

Please sign in to comment.