Fix crash from --incompatible_enforce_config_setting_visibility
Reproducing requires https://github.com/bazelbuild/bazel/commit/a3a4cf8c486d1d27e72d9e595c9d718efb21071a and --incompatible_enforce_config_setting_visibility=1.
This is an obscure crash caused by a) building a test, b) with a select() on an alias, c) with --trim_test_configuration=1.
Details in commit.
For https://github.com/bazelbuild/bazel/issues/12932.
Fixes https://github.com/bazelbuild/bazel/issues/16446.
PiperOrigin-RevId: 480356600
Change-Id: I27227616dd2d1681b7769a29b73dac457f59d9b7
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 838f872..7cd33fa 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
@@ -1756,7 +1756,8 @@
// alias visibility, as is usual semantics. So two the following two edges are
// checked: 1: select() -> alias and 2: alias -> config_setting.
configSettingVisibilityPolicy == ConfigSettingVisibilityPolicy.DEFAULT_PUBLIC
- ? condition.fromConfiguredTarget(condition.getConfiguredTarget().getActual())
+ ? condition.fromConfiguredTargetNoCheck(
+ condition.getConfiguredTarget().getActual())
: condition);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java
index 7171920..e7d8956 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java
@@ -50,10 +50,22 @@
Target target,
BuildConfigurationValue configuration,
ImmutableList<String> transitionKeys) {
+ this(configuredTarget, target, configuration, transitionKeys, /*checkConsistency=*/ true);
+ }
+
+ private ConfiguredTargetAndData(
+ ConfiguredTarget configuredTarget,
+ Target target,
+ BuildConfigurationValue configuration,
+ ImmutableList<String> transitionKeys,
+ boolean checkConsistency) {
this.configuredTarget = configuredTarget;
this.target = target;
this.configuration = configuration;
this.transitionKeys = transitionKeys;
+ if (!checkConsistency) {
+ return;
+ }
Preconditions.checkState(
configuredTarget.getLabel().equals(target.getLabel()),
"Unable to construct ConfiguredTargetAndData:"
@@ -124,6 +136,19 @@
return new ConfiguredTargetAndData(maybeNew, target, configuration, transitionKeys);
}
+ /**
+ * Variation of {@link #fromConfiguredTarget} that doesn't check the new target has the same
+ * configuration as the original.
+ *
+ * <p>Intended for trimming (like {@code --trim_test_configuration}).
+ */
+ public ConfiguredTargetAndData fromConfiguredTargetNoCheck(ConfiguredTarget maybeNew) {
+ if (configuredTarget.equals(maybeNew)) {
+ return this;
+ }
+ return new ConfiguredTargetAndData(maybeNew, target, configuration, transitionKeys, false);
+ }
+
public Target getTarget() {
return target;
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
index 8581035..4df31b5 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
@@ -1028,10 +1028,11 @@
@Test
public void selectConcatenatedWithNonSupportingType() throws Exception {
writeConfigRules();
- scratch.file("foo/BUILD",
+ scratch.file(
+ "foo/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= 0 + select({",
+ " boolean_attr = 0 + select({",
" '//conditions:a': 0,",
" '//conditions:b': 1,",
" }))");
@@ -1476,7 +1477,7 @@
" actual = ':foo')",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" ':foo': 0,",
" 'alias_to_foo': 1,",
" }))");
@@ -1500,7 +1501,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
@@ -1525,7 +1526,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
@@ -1550,7 +1551,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
@@ -1569,7 +1570,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
@@ -1595,7 +1596,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
@@ -1621,7 +1622,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
@@ -1651,7 +1652,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo_alias': 0,",
" '//conditions:default': 1",
" }))");
@@ -1684,7 +1685,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo_alias': 0,",
" '//conditions:default': 1",
" }))");
@@ -1718,7 +1719,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo_alias': 0,",
" '//conditions:default': 1",
" }))");
@@ -1728,6 +1729,57 @@
}
@Test
+ public void defaultPublicVisibility_trimmedConfigsDontCrash() throws Exception {
+ // When enforcing config_setting visibility with
+ // --incompatible_config_setting_private_default_visibility=false, the alias
+ // ConfiguredTargetAndData clones itself with the ConfiguredTargetAndData of the config_setting
+ // it refers to. ConfiguredTargetAndData.fromConfiguredTarget has a safety check that both
+ // configs are the same. When the target with a select() is a test and --trim_test_configuration
+ // is on, the alias takes the parent's config (with TestOptions) but the config_setting has it
+ // stripped. This is a regression test that Blaze doesn't crash expecting those configs to be
+ // equal.
+ setPackageOptions(
+ "--incompatible_enforce_config_setting_visibility=true",
+ "--incompatible_config_setting_private_default_visibility=false");
+ useConfiguration("--trim_test_configuration=true");
+ scratch.file(
+ "c/defs.bzl",
+ "def _impl(ctx):",
+ " output = ctx.outputs.out",
+ " ctx.actions.write(output = output, content = 'hi', is_executable = True)",
+ " return [DefaultInfo(executable = output)]",
+ "",
+ "fake_test = rule(",
+ " attrs = {",
+ " 'msg': attr.string(),",
+ " },",
+ " test = True,",
+ " outputs = {'out': 'foo.out'},",
+ " implementation = _impl,",
+ ")");
+ scratch.file(
+ "c/BUILD",
+ "load(':defs.bzl', 'fake_test')",
+ "alias(",
+ " name = 'foo_alias',",
+ " actual = ':foo',",
+ ")",
+ "config_setting(",
+ " name = 'foo',",
+ " define_values = { 'foo': '1' },",
+ ")",
+ "fake_test(",
+ " name = 'foo_test',",
+ " msg = select({",
+ " ':foo_alias': 'hi',",
+ " '//conditions:default': 'there'",
+ " }))");
+ reporter.removeHandler(failFastHandler);
+ assertThat(getConfiguredTarget("//c:foo_test")).isNotNull();
+ assertNoEvents();
+ }
+
+ @Test
public void defaultVisibilityConfigSetting_defaultIsPrivate() throws Exception {
// Production builds default to private visibility, but BuildViewTestCase defaults to public.
setPackageOptions("--default_visibility=private",
@@ -1738,7 +1790,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
@@ -1764,7 +1816,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
@@ -1790,7 +1842,7 @@
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
- " boolean_attr= select({",
+ " boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
