Skip to content
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.

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

bazel visibility extension should allow override default_visibility #1467

Closed
sluongng opened this issue Mar 6, 2023 · 3 comments · Fixed by #1472
Closed

bazel visibility extension should allow override default_visibility #1467

sluongng opened this issue Mar 6, 2023 · 3 comments · Fixed by #1472

Comments

@sluongng
Copy link
Contributor

sluongng commented Mar 6, 2023

Let's say I have a setup like this

foo/BUILD
foo/bar/BUILD

with these file contents

cat foo/BUILD
# gazelle:default_visibility //foo:__subpackages
cat foo/bar/BUILD
# gazelle:default_visibility //foo/bar:__subpackages

Running gazelle bazel/visibility extension would generate something like this

cat foo/bar/BUILD
# gazelle:default_visibility //foo/bar:__subpackages
package(default_visibility = [
    "//foo:__subpackages__",
    "//foo/bar:__subpackages__",
])

while the sensible expected result should be

cat foo/bar/BUILD
# gazelle:default_visibility //foo/bar:__subpackages
package(default_visibility = ["//foo/bar:__subpackages__"])
@sluongng
Copy link
Contributor Author

sluongng commented Mar 6, 2023

cc: @linzhp @moisesvega @dnathe4th

@dnathe4th
Copy link
Contributor

Yup you're right. I accumulated visibilities here with the idea that it would allow someone to define multiple back to back directives, but I did not consider the case that it is even appending directive values it finds in other files.

Maybe it is better to only accumulate within the same file (i.e. same call to Configure) and overwrite the config's list of targets at the end of that method?

@sluongng
Copy link
Contributor Author

sluongng commented Mar 7, 2023

Yeah, I think it's better to treat a subpackage's default_visibility directive as an override of the parent package's default_visibility directive. In my mind, I cannot think of a use case where I would want to inherit the parent's visibility config. Usually, it's always going to be some complete override.

If a change were made to parent scope visibility, the change's author should be responsible to replicate such changes to children's packages that have overridden parents' visibility. That way, such change would impact code ownership and trigger various code review systems.

So it's gonna be a +1 for me to get rid of the accumulation.

Side note: Imo this behavior is relatively safe to change. Although the language plugin was part of the last release and we recently added documentation for the new directive, the language is not yet included as part of DEFAULT_LANGUAGE exported in def.bzl so folks are not enabling this by default. Given that (a) this is a relatively new feature and (b) it's an opt-in feature without clear documentation, I think an update to this behavior should be ok.

dnathe4th added a commit to dnathe4th/bazel-gazelle that referenced this issue Mar 9, 2023
…b#1467)

Previously the logic of this extension accumulated default visibilities
to the list across all configurations without any reset. As noted in the
attached issue, this is unintuitive and did not add real value so long
as multiple directives within the same file can still aggregate values.
linzhp pushed a commit that referenced this issue Mar 11, 2023
Previously the logic of this extension accumulated default visibilities
to the list across all configurations without any reset. As noted in the
attached issue, this is unintuitive and did not add real value so long
as multiple directives within the same file can still aggregate values.
sluongng added a commit to buildbuddy-io/buildbuddy that referenced this issue Apr 1, 2023
Fix all the visibility scope issues that was reported in
bazel-contrib/bazel-gazelle#1467
sluongng added a commit to buildbuddy-io/buildbuddy that referenced this issue Apr 1, 2023
Fix all the visibility scope issues that was reported in
bazel-contrib/bazel-gazelle#1467
sluongng added a commit to buildbuddy-io/buildbuddy that referenced this issue Apr 1, 2023
Fix all the visibility scope issues that was reported in
bazel-contrib/bazel-gazelle#1467
sluongng added a commit to buildbuddy-io/buildbuddy that referenced this issue Apr 1, 2023
Fix all the visibility scope issues that was reported in
bazel-contrib/bazel-gazelle#1467
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants