Skip to content

Always check $config_dependencies visibility at use#19139

Closed
fmeum wants to merge 1 commit intobazelbuild:masterfrom
fmeum:19126-fix-config-setting-checked-at-rule
Closed

Always check $config_dependencies visibility at use#19139
fmeum wants to merge 1 commit intobazelbuild:masterfrom
fmeum:19126-fix-config-setting-checked-at-rule

Conversation

@fmeum
Copy link
Copy Markdown
Collaborator

@fmeum fmeum commented Aug 1, 2023

All dependencies for select expressions are collected in an implicit $config_dependencies attribute. Even with
--incompatible_visibility_private_attributes_at_definition, visibility for this attribute is now checked relative to the use rather than the definition of the rule.

Fixes #19126

@fmeum
Always check $config_dependencies visibility at use
ca83c84 
All dependencies for `select` expressions are collected in an implicit
`$config_dependencies` attribute. Even with
`--incompatible_visibility_private_attributes_at_definition`, visibility
for this attribute is now checked relative to the use rather than the
definition of the rule.
@fmeum fmeum requested a review from a team as a code owner August 1, 2023 10:03
@fmeum fmeum requested review from sdtwigg and removed request for a team August 1, 2023 10:03
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels Aug 1, 2023
@fmeum fmeum requested review from gregestren and removed request for sdtwigg August 1, 2023 10:03
@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Aug 1, 2023

@gregestren I got the results of a downstream run with this flag flipped and it looks promising. Do you think I could attempt to flip it for Bazel 7?

@gregestren
Copy link
Copy Markdown
Contributor

gregestren commented Aug 1, 2023

@brandjon what's the story for --incompatible_visibility_private_attributes_at_definition? I'm not familiar with this flag: it came from 752ffcc.

@fmeum this seems useful to me but I don't have much on-the-ground perspective. Are you finding real failures addressed by this? I pinged @brandjon since he's done a lot of work on clarifying visibility checking, including definition vs. use.

gregestren
gregestren reviewed Aug 1, 2023
View reviewed changes
src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java
"config_setting(",
" name = 'my_setting',",
" values = {'cpu': 'does_not_matter'},",
" visibility = ['//:__pkg__'],",
Copy link
Copy Markdown
Contributor

@gregestren gregestren Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this intersect at all with #12932 or #12933?

Copy link
Copy Markdown
Collaborator Author

@fmeum fmeum Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, the visibility check is bypassed with --noincompatible_config_setting_private_default_visibility, which means that this issue has only been around since the flip of --incompatible_config_setting_private_default_visibility pre Bazel 6.

Copy link
Copy Markdown
Contributor

@gregestren gregestren Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, isn't that still off by default?

bazel/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java

Lines 137 to 138 in 895969d

name = "incompatible_config_setting_private_default_visibility",
defaultValue = "false",

Or is this the connection?

bazel/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java

Lines 123 to 126 in 895969d

name = "incompatible_enforce_config_setting_visibility",
// TODO(b/179944632): set this and --incompatible_config_setting_private_default_visibility
// to true, then make these no-ops, then remove.
defaultValue = "true",

Copy link
Copy Markdown
Collaborator Author

@fmeum fmeum Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the connection, the issue goes away with --noincompatible_enforce_config_setting_visibility. Without the explicit visibility on the config_setting, the test added in this PR passes without the changes to the production code.

@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Aug 2, 2023

@fmeum this seems useful to me but I don't have much on-the-ground perspective. Are you finding real failures addressed by this?

I do. From my perspective as a ruleset author, the current behavior is pretty unintuitive and effectively forces all tools used by rules to be publicly visible, even when those tools are considered internal. Examples include rules_go and rules_jvm_external.

Since all tools in public rulesets need to be visible globally due to the current behavior, I would consider the risk of undetected breakages to be minimal.

@gregestren gregestren added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Aug 3, 2023
@copybara-service copybara-service bot closed this in 5ca89d4 Aug 4, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 4, 2023
@fmeum fmeum deleted the 19126-fix-config-setting-checked-at-rule branch August 4, 2023 19:15
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Aug 7, 2023
@fmeum @iancha1992
Always check $config_dependencies visibility at use
e70af95 
All dependencies for `select` expressions are collected in an implicit `$config_dependencies` attribute. Even with
`--incompatible_visibility_private_attributes_at_definition`, visibility for this attribute is now checked relative to the use rather than the definition of the rule.

Fixes bazelbuild#19126

Closes bazelbuild#19139.

PiperOrigin-RevId: 553830083
Change-Id: Ic4af0f5ad9a0c627c973cd5ce88dd1e1bf51474e
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Aug 7, 2023
@fmeum @iancha1992
Always check $config_dependencies visibility at use
081b0ce 
All dependencies for `select` expressions are collected in an implicit `$config_dependencies` attribute. Even with
`--incompatible_visibility_private_attributes_at_definition`, visibility for this attribute is now checked relative to the use rather than the definition of the rule.

Fixes bazelbuild#19126

Closes bazelbuild#19139.

PiperOrigin-RevId: 553830083
Change-Id: Ic4af0f5ad9a0c627c973cd5ce88dd1e1bf51474e
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Aug 7, 2023
@fmeum @iancha1992
Always check $config_dependencies visibility at use
2a39d6f 
All dependencies for `select` expressions are collected in an implicit `$config_dependencies` attribute. Even with
`--incompatible_visibility_private_attributes_at_definition`, visibility for this attribute is now checked relative to the use rather than the definition of the rule.

Fixes bazelbuild#19126

Closes bazelbuild#19139.

PiperOrigin-RevId: 553830083
Change-Id: Ic4af0f5ad9a0c627c973cd5ce88dd1e1bf51474e
@iancha1992 iancha1992 mentioned this pull request Aug 7, 2023
Merged
iancha1992 added a commit that referenced this pull request Aug 7, 2023
@iancha1992 @fmeum
Always check $config_dependencies visibility at use (#19197)
5dd60a6 
All dependencies for `select` expressions are collected in an implicit `$config_dependencies` attribute. Even with
`--incompatible_visibility_private_attributes_at_definition`, visibility for this attribute is now checked relative to the use rather than the definition of the rule.

Fixes #19126

Closes #19139.

PiperOrigin-RevId: 553830083
Change-Id: Ic4af0f5ad9a0c627c973cd5ce88dd1e1bf51474e

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
@fmeum fmeum mentioned this pull request Aug 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gregestren gregestren gregestren approved these changes

Assignees

No one assigned

Labels

team-Configurability platforms, toolchains, cquery, select(), config transitions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

config_setting incorrectly treated as implicit dependency by --incompatible_visibility_private_attributes_at_definition

2 participants

@fmeum @gregestren