Skip to content

Commit

Permalink
Adjust semantics for config_setting visibility migration.
Browse files Browse the repository at this point in the history 
Today, config_setting_visibility is completely ignored. This also means for select() -> alias -> config_setting, the alias visibility is ignored.

We'll migrate to full visibility checking in two steps:

1) Turn on --incompatible_enforce_config_setting_visibility. This turns on visibility checking but defaults config_setting visibility to public. This only causes depot failures for config_settings that are explicitly set otherwise, which we can clean up.

2) Turn on --incompatible_config_setting_private_default_visibility: this defaults config_setting visibility to private, which is standard visibility semantics. This will break more of the depot (i.e. any config_setting in a package that doesn't change its default visibility). We'll clean that up too.

This change fixes a small issue with aliases. The intention of step 1 is that we allow config_settings that aren't explicitly visibility-restricted. But that would fail for a select() -> alias -> config_setting_with_default_visibility. Visibility checking independently checks the select() -> alias edge, which has the usual private default.

This change skips that edge entirely and directly compares the select() against config_setting_with_default_visibility.

We don't want to ultimately skip alias visibility checking. But we can get that working at step 2. As it stands now, at step 0, this is already skipped. So this change isn't a regression. It just helps smooth migration.

For #12932 and #12933.

PiperOrigin-RevId: 479631810
Change-Id: I8b77f62bb1584cc74d182995afa19fe70a8d4875
  • Loading branch information
@gregestren @copybara-github
gregestren authored and copybara-github committed Oct 7, 2022
1 parent 1e16a97 commit a3a4cf8
Show file tree
Hide file tree
Showing 2 changed files with 126 additions and 3 deletions.
31 changes: 28 additions & 3 deletions src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1728,11 +1728,36 @@ private RuleContext build(boolean attributeChecks) throws InvalidExecGroupExcept
// --config_setting_visibility_policy. This should be removed as soon as it's deemed safe
// to unconditionally check visibility. See
// https://github.com/bazelbuild/bazel/issues/12669.
if (target.getPackage().getConfigSettingVisibilityPolicy()
!= ConfigSettingVisibilityPolicy.LEGACY_OFF) {
ConfigSettingVisibilityPolicy configSettingVisibilityPolicy =
target.getPackage().getConfigSettingVisibilityPolicy();
if (configSettingVisibilityPolicy != ConfigSettingVisibilityPolicy.LEGACY_OFF) {
Attribute configSettingAttr = attributes.getAttributeDefinition("$config_dependencies");
for (ConfiguredTargetAndData condition : configConditions.asConfiguredTargets().values()) {
validateDirectPrerequisite(configSettingAttr, condition);
validateDirectPrerequisite(
configSettingAttr,
// Another nuance: when both --incompatible_enforce_config_setting_visibility and
// --incompatible_config_setting_private_default_visibility are disabled, both of
// these are ignored:
//
// - visibility settings on a select() -> config_setting dep
// - visibility settings on a select() -> alias -> config_setting dep chain
//
// In that scenario, both are ignored because the logic here that checks the
// select() -> ??? edge is completely skipped.
//
// When just --incompatible_enforce_config_setting_visibility is on, that means
// "enforce config_setting visibility with public default". That's a temporary state
// to support depot migration. In that case, we continue to ignore the alias'
// visibility in preference for the config_setting. So skip select() -> alias as
// before, but now enforce select() -> config_setting_the_alias_refers_to.
//
// When we also turn on --incompatible_config_setting_private_default_visibility, we
// expect full standard visibility compliance. In that case we directly evaluate the
// 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);
}
}

Expand Down
98 changes: 98 additions & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1629,6 +1629,104 @@ public void publicVisibilityConfigSetting_defaultIsPublic() throws Exception {
assertThat(getConfiguredTarget("//a:binary")).isNotNull();
assertNoEvents();
}

@Test
public void defaultPublicVisibility_aliasVisibilityIgnored_aliasVisibilityIsDefault()
throws Exception {
// Production builds default to private visibility, but BuildViewTestCase defaults to public.
setPackageOptions(
"--default_visibility=private",
"--incompatible_enforce_config_setting_visibility=true",
"--incompatible_config_setting_private_default_visibility=false");
scratch.file(
"c/BUILD",
"alias(",
" name = 'foo_alias',",
" actual = ':foo')",
"config_setting(",
" name = 'foo',",
" define_values = { 'foo': '1' },",
")");
scratch.file(
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" '//c:foo_alias': 0,",
" '//conditions:default': 1",
" }))");
reporter.removeHandler(failFastHandler);
assertThat(getConfiguredTarget("//a:binary")).isNotNull();
assertNoEvents();
}

@Test
public void defaultPublicVisibility_aliasVisibilityIgnored_aliasVisibilityIsExplicit()
throws Exception {
// Production builds default to private visibility, but BuildViewTestCase defaults to public.
setPackageOptions(
"--default_visibility=private",
"--incompatible_enforce_config_setting_visibility=true",
"--incompatible_config_setting_private_default_visibility=false");
scratch.file(
"c/BUILD",
"alias(",
" name = 'foo_alias',",
" actual = ':foo',",
// Current flag combo skips this and directly checks the config_setting's visibility.
" visibility = ['//visibility:private']",
")",
"config_setting(",
" name = 'foo',",
" define_values = { 'foo': '1' },",
")");
scratch.file(
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" '//c:foo_alias': 0,",
" '//conditions:default': 1",
" }))");
reporter.removeHandler(failFastHandler);
assertThat(getConfiguredTarget("//a:binary")).isNotNull();
assertNoEvents();
}

@Test
public void defaultPublicVisibility_aliasVisibilityIgnored_configSettingVisibilityIsExplicit()
throws Exception {
// Production builds default to private visibility, but BuildViewTestCase defaults to public.
setPackageOptions(
"--default_visibility=private",
"--incompatible_enforce_config_setting_visibility=true",
"--incompatible_config_setting_private_default_visibility=false");
scratch.file(
"c/BUILD",
"alias(",
" name = 'foo_alias',",
" actual = ':foo',",
// Current flag combo skips this and directly checks the config_setting's visibility.
" visibility = ['//visibility:public']",
")",
"config_setting(",
" name = 'foo',",
" define_values = { 'foo': '1' },",
" visibility = ['//visibility:private']",
")");
scratch.file(
"a/BUILD",
"rule_with_boolean_attr(",
" name = 'binary',",
" boolean_attr= select({",
" '//c:foo_alias': 0,",
" '//conditions:default': 1",
" }))");
reporter.removeHandler(failFastHandler);
assertThat(getConfiguredTarget("//a:binary")).isNull();
assertContainsEvent("'//c:foo' is not visible from target '//a:binary'");
}

@Test
public void defaultVisibilityConfigSetting_defaultIsPrivate() throws Exception {
// Production builds default to private visibility, but BuildViewTestCase defaults to public.
Expand Down

0 comments on commit a3a4cf8

Please sign in to comment.