Skip to content

Commit

Permalink
Fix crash from --incompatible_enforce_config_setting_visibility
Browse files Browse the repository at this point in the history
Reproducing requires bazelbuild@a3a4cf8 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 bazelbuild#12932.
Fixes bazelbuild#16446.

PiperOrigin-RevId: 480356600
Change-Id: I27227616dd2d1681b7769a29b73dac457f59d9b7
  • Loading branch information
gregestren authored and aiuto committed Oct 12, 2022
1 parent 45f84e9 commit f4a4642
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1756,7 +1756,8 @@ private RuleContext build(boolean attributeChecks) throws InvalidExecGroupExcept
// 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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,22 @@ public ConfiguredTargetAndData(
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:"
Expand Down Expand Up @@ -124,6 +136,19 @@ public ConfiguredTargetAndData fromConfiguredTarget(ConfiguredTarget maybeNew) {
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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1028,10 +1028,11 @@ public void dictsWithSameKey() throws Exception {
@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,",
" }))");
Expand Down Expand Up @@ -1476,7 +1477,7 @@ public void multipleMatchErrorWhenAliasResolvesToSameSetting() throws Exception
" actual = ':foo')",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" ':foo': 0,",
" 'alias_to_foo': 1,",
" }))");
Expand All @@ -1500,7 +1501,7 @@ public void defaultVisibilityConfigSetting_noVisibilityEnforcement() throws Exce
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
Expand All @@ -1525,7 +1526,7 @@ public void privateVisibilityConfigSetting_noVisibilityEnforcement() throws Exce
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
Expand All @@ -1550,7 +1551,7 @@ public void publicVisibilityConfigSetting__noVisibilityEnforcement() throws Exce
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
Expand All @@ -1569,7 +1570,7 @@ public void defaultVisibilityConfigSetting_defaultIsPublic() throws Exception {
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
Expand All @@ -1595,7 +1596,7 @@ public void privateVisibilityConfigSetting_defaultIsPublic() throws Exception {
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
Expand All @@ -1621,7 +1622,7 @@ public void publicVisibilityConfigSetting_defaultIsPublic() throws Exception {
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
Expand Down Expand Up @@ -1651,7 +1652,7 @@ public void defaultPublicVisibility_aliasVisibilityIgnored_aliasVisibilityIsDefa
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo_alias': 0,",
" '//conditions:default': 1",
" }))");
Expand Down Expand Up @@ -1684,7 +1685,7 @@ public void defaultPublicVisibility_aliasVisibilityIgnored_aliasVisibilityIsExpl
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo_alias': 0,",
" '//conditions:default': 1",
" }))");
Expand Down Expand Up @@ -1718,7 +1719,7 @@ public void defaultPublicVisibility_aliasVisibilityIgnored_configSettingVisibili
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo_alias': 0,",
" '//conditions:default': 1",
" }))");
Expand All @@ -1727,6 +1728,57 @@ public void defaultPublicVisibility_aliasVisibilityIgnored_configSettingVisibili
assertContainsEvent("'//c:foo' is not visible from target '//a:binary'");
}

@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.
Expand All @@ -1738,7 +1790,7 @@ public void defaultVisibilityConfigSetting_defaultIsPrivate() throws Exception {
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
Expand All @@ -1764,7 +1816,7 @@ public void privateVisibilityConfigSetting_defaultIsPrivate() throws Exception {
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
Expand All @@ -1790,7 +1842,7 @@ public void publicVisibilityConfigSetting_defaultIsPrivate() throws Exception {
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" boolean_attr = select({",
" '//c:foo': 0,",
" '//conditions:default': 1",
" }))");
Expand Down

0 comments on commit f4a4642

Please sign in to comment.