Skip to content

Always check $config_dependencies visibility at use #19139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
fmeum wants to merge 1 commit into from
Closed

Always check $config_dependencies visibility at use #19139

fmeum wants to merge 1 commit into from

Conversation

@fmeum
Copy link
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
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
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
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
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
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
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
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 <[email protected]>
@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