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

fix(gazelle): Cease write narrow visibilities #1681

Closed
wants to merge 1 commit into from
Closed

fix(gazelle): Cease write narrow visibilities #1681

wants to merge 1 commit into from

Conversation

aryamccarthy
Copy link

This PR causes Gazelle to not write visibility lines for generated rules.

Context

This is necessary in monorepo situations where there are multiple # gazelle:python_root directives used, e.g. in the structure below:

ROOT
    proj_1
        src  # is a python root and depends on proj_2
            foo
    pro_2
        src  # is a python root
            bar  ← The visibility here will be incorrect; proj_1 cannot see this.

As it stands, we write a narrowly scoped visibility attribute for every generated rule; the visibility is //{python-root-package}:__subpackages__. Let's not!


Alternatives

Two considered alternatives were:

  • Have the Gazelle Python extension respect the # gazelle:default_visibility directive. This is probably correct in the long run, but I'm not Go-literate.
  • Set all visibilities to //visibility:public. This achieves correct behavior but gives less control than the current PR. Instead of respecting any default visibility stanzas, it forces the entirety to be public.

Preserving current behavior

If someone wants the current behavior, it can still be achieved by setting the following in Python root's BUILD file:

package(
    default_visibility = ["//" + package_name() + ":__subpackages__"],
)

Copy link

google-cla bot commented Jan 11, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@f0rmiga
Copy link
Collaborator

f0rmiga commented Jan 11, 2024

This is very tragic in many ways. Do you want to have less working setting visibility and then set everything to public? You are defeating the purpose of the visibility attribute and wanting everyone to defeat it with you!

Open an issue first to discuss the alternatives, but setting everything to public is most likely not going to happen.

@f0rmiga f0rmiga closed this Jan 11, 2024
@aryamccarthy
Copy link
Author

aryamccarthy commented Jan 11, 2024

I absolutely don't want to set everything to //visibility:public! But in the base implementation, visibilities are written per-rule, using a hardcoded value in Go. There's no way to inherit defaults from the package or up the tree.

If I have a rule like this in proj_2, it should be respected:

package(
    default_visibility = ["proj_1:__subpackages__", "proj_2:__subpackages__"],
)

This isn't public: proj_3 and proj_4 can't see this.

I think that not writing a visibility attribute on each generated rule is safer here than setting to public.

@aryamccarthy
Copy link
Author

Okay, I see our problem. I had uploaded an older commit. The latest version does what the explanation above describes.

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 this pull request may close these issues.

2 participants