Skip to content

Commit

Permalink
Add attribute validation to IncompatibleTargetChecker. (#18135)
Browse files Browse the repository at this point in the history
This causes errors with configurable `target_compatible_with` to be
reported as errors, rather than causing a Bazel crash.

Fixes #18021.

(This is functionally the same change as
ebfb529, but applied to the
release-6.2.0 branch)
  • Loading branch information
katre committed Apr 18, 2023
1 parent 1a60fad commit 5e9fa39
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public static Optional<RuleConfiguredTargetValue> createDirectlyIncompatibleTarg
Environment env,
@Nullable PlatformInfo platformInfo,
NestedSetBuilder<Package> transitivePackages)
throws InterruptedException {
throws ConfiguredAttributeMapper.ValidationException, InterruptedException {
Target target = targetAndConfiguration.getTarget();
Rule rule = target.getAssociatedRule();

Expand All @@ -124,7 +124,7 @@ public static Optional<RuleConfiguredTargetValue> createDirectlyIncompatibleTarg
}

// Resolve the constraint labels.
List<Label> labels = attrs.get("target_compatible_with", BuildType.LABEL_LIST);
List<Label> labels = attrs.getAndValidate("target_compatible_with", BuildType.LABEL_LIST);
ImmutableList<ConfiguredTargetKey> constraintKeys =
labels.stream()
.map(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ private ValidationException(String message) {
* Variation of {@link #get} that throws an informative exception if the attribute can't be
* resolved due to intrinsic contradictions in the configuration.
*/
private <T> T getAndValidate(String attributeName, Type<T> type) throws ValidationException {
public <T> T getAndValidate(String attributeName, Type<T> type) throws ValidationException {
SelectorList<T> selectorList = getSelectorList(attributeName, type);
if (selectorList == null) {
// This is a normal attribute.
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
"//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper",
"//src/main/java/com/google/devtools/build/lib/packages:exec_group",
"//src/main/java/com/google/devtools/build/lib/packages:globber",
"//src/main/java/com/google/devtools/build/lib/packages:globber_utils",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper;
import com.google.devtools.build.lib.packages.ExecGroup;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
Expand Down Expand Up @@ -108,6 +109,7 @@
import java.util.function.Predicate;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
import net.starlark.java.syntax.Location;

/**
* SkyFunction for {@link ConfiguredTargetValue}s.
Expand Down Expand Up @@ -335,13 +337,28 @@ public SkyValue compute(SkyKey key, Environment env)
getPrioritizedDetailedExitCode(causes)));
}

Optional<RuleConfiguredTargetValue> incompatibleTarget =
IncompatibleTargetChecker.createDirectlyIncompatibleTarget(
targetAndConfiguration,
configConditions,
env,
platformInfo,
state.transitivePackages);
Optional<RuleConfiguredTargetValue> incompatibleTarget;
try {
incompatibleTarget =
IncompatibleTargetChecker.createDirectlyIncompatibleTarget(
targetAndConfiguration,
configConditions,
env,
platformInfo,
state.transitivePackages);
} catch (ConfiguredAttributeMapper.ValidationException e) {
BuildConfigurationValue configuration = targetAndConfiguration.getConfiguration();
Label label = targetAndConfiguration.getLabel();
Location location = targetAndConfiguration.getTarget().getLocation();
env.getListener().post(new AnalysisRootCauseEvent(configuration, label, e.getMessage()));
throw new DependencyEvaluationException(
new ConfiguredValueCreationException(
location, e.getMessage(), label, configuration.getEventId(), null, null),
// These errors occur within DependencyResolver, which is attached to the current
// target. i.e. no dependent ConfiguredTargetFunction call happens to report its own
// error.
/* depReportedOwnError= */ false);
}
if (incompatibleTarget == null) {
return null;
}
Expand Down
27 changes: 27 additions & 0 deletions src/test/shell/integration/target_compatible_with_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1551,4 +1551,31 @@ EOF
expect_not_log "${debug_message5}"
}

# Regression test for b/277371822.
function test_missing_default() {
cat >> target_skipping/BUILD <<'EOF'
sh_test(
name = "pass_on_foo1_or_foo2_but_not_on_foo3",
srcs = [":pass.sh"],
target_compatible_with = select({
":foo1": [],
# No default branch.
}),
)
EOF

cd target_skipping || fail "couldn't cd into workspace"

bazel test \
--show_result=10 \
--host_platform=@//target_skipping:foo3_platform \
--platforms=@//target_skipping:foo3_platform \
//target_skipping:pass_on_foo1_or_foo2_but_not_on_foo3 &> "${TEST_log}" \
&& fail "Bazel passed unexpectedly."

expect_log 'ERROR:.*configurable attribute "target_compatible_with" in //target_skipping:pass_on_foo1_or_foo2_but_not_on_foo3'
expect_log 'FAILED: Build did NOT complete successfully'
expect_not_log 'FATAL: bazel crashed'
}

run_suite "target_compatible_with tests"

0 comments on commit 5e9fa39

Please sign in to comment.