gazelle: Visibilities are incorrect for projects with multiple `# gazelle:python_root` directives

[aryamccarthy]

🐞 bug report

Affected Rule

The issue is caused by the rule: https://github.com/bazelbuild/rules_python/blob/b106f91c9da7e31d73a7293e88ac78eedcad2057/gazelle/python/generate.go#L215

Is this a regression?

No

Description

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
    proj_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__. Bazel will fail to build targets in proj_1 because of this.

🔥 Exception or Error

🌍 Your Environment

Operating System:

  
macOS 14.2
  

Output of bazel version:

  
Build label: 7.0.0
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Mon Dec 11 16:52:42 2023 (1702313562)
Build timestamp: 1702313562
Build timestamp as int: 1702313562
  

Rules_python version:

  
0.27.1
  

Anything else relevant?

The desired implementation is not to set everything to //visibility:public, rather, it's to resolve cross-project dependencies in a monorepo the same way as they are within one directory.

Consider a working example here: main...aryamccarthy:rules_python:main

[aignas]

I think this is something that you may want only if the gazelle roots are using the same requirements files, so this ask is not necessarily something that is useful for all scenarios and I am not sure it should be the default.

[eaplatanios]

@aignas I posted a reply here: #1683 (comment). Regarding the requirements files, that's true regarding external dependencies. Our main issue has to do with internal dependencies between the different Python projects in our Bazel workspace themselves.

[aignas]

Continuing the discussion her.

I think the best way would be to support the default visibility directive that gazelle has to support your usecase.

Just to ensure that we are not missing anything here - if you manually modify the visibility of a talget you want to expose, does it get overwritten in the consecutive gazelle runs?

[aryamccarthy]

I agree that supporting the # gazelle:default_visibility directive would be ideal. (I mentioned that in #1681.) Until that's implemented (and I would have to learn Go to do it), this gets monorepos with shared requirements files working. And for repos that want the existing behavior, it's a one-line addition to their BUILD file (which @eaplatanios noted):

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

If you manually modify the visibility, it's not overwritten. That's true (and good!). But this saves a lot of manual effort the first time the monorepo gets BUILD files generated by Gazelle, in making those updates. The problem is that the Gazelle-inserted visibility can be too restrictive, but it overrules any package-level visibility declarations. Every single target would need to have the visibility altered after Gazelle is run. And any targets that are added later also need to be corrected.

The calculus that motivated this was: a one-line default_visibility attribute once is much cheaper than N visibility updates and having to perennially make fixes as the codebase grows.

[f0rmiga]

I think the best way forward is to support the gazelle:default_visibility directive. I don't have the bandwidth to do this right now. @aignas, do you? Otherwise, @aryamccarthy, Go is really easy to pick.

[aignas]

I don't have bandwidth to work on this either.

[aryamccarthy]

It seems the current behavior is erroneous, and either path would ameliorate this. The one in PR #1683 fixes the bug, and processing # gazelle:default_visibility directives could be seen as an additional feature later on.

The Gazelle source code (in a comment) says that if there's a package(default_visibility=...) stanza, then no visibility declaration should be generated. This helps build the case that the current behavior is wrong. Even if we don't "fix" it by respecting default_visibility directives, our PR resolves that.

[f0rmiga]

This code was added 6 years ago: bazel-contrib/bazel-gazelle@cdc77d0. This is when only the Go generation existed. This is not a rule. If it were, it would be enforced by the framework.

[eaplatanios]

@f0rmiga @aignas we're currently blocked from using gazelle python in our codebase due to this issue. What is your suggestion for moving forward assuming we do not have the bandwidth to learn Go and your codebase to make the ideal changes? Is there any chance you'd approve the current PR or a variation of it or should we look into other solutions that do not involve gazelle-python?

And to be clear, the main difference between the # gazelle:default_visibility solution and what the current PR enables afaiu is that you have to type this line in your BUILD file:

# gazelle:default_visibility ...

instead of this one (which the current PR enables):

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

Could you please explain why this is a bad solution? Or at least why you think it's worse than what gazelle-python currently does which is enforcing a stronger opinion that is not configurable at all for the rule visibilities?

[f0rmiga]

I appreciate the time you've put into this discussion and raising a point that makes your adoption of the Python extension for Gazelle difficult.

As an open-source project, we have to be careful with changes to the behaviour of this extension, especially one that affects everyone else, in order to satisfy your use case. I'm not saying that your use case is wrong or that it doesn't deserve attention, but the optimal solution has been laid out, and you are not committed to helping improve the code generator (even though you are blocked on it). A few companies that provide Bazel consulting would be happy to contribute to this issue if you hire them.

What is your suggestion for moving forward assuming we do not have the bandwidth to learn Go and your codebase to make the ideal changes?

Find someone who has the bandwidth to contribute (whatever it means - including learning Go).
There's also the possibility of writing targets by hand, which lots of people do. Seems to be working fine for them.

Could you please explain why this is a bad solution? Or at least why you think it's worse than what gazelle-python currently does which is enforcing a stronger opinion that is not configurable at all for the rule visibilities?

You are asking for everyone in the community to fit your workflow and not the other way around. The Gazelle code generator is optional and it is opinionated.

[f0rmiga]

It also occurred to me that you could probably just use https://github.com/aspect-build/plugin-fix-visibility to fix your visibility issues.