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

Add support for gazelle's default_visibility directive #1786

Closed
dougthor42 opened this issue Feb 29, 2024 · 2 comments
Closed

Add support for gazelle's default_visibility directive #1786

dougthor42 opened this issue Feb 29, 2024 · 2 comments

Comments

@dougthor42
Copy link
Collaborator

🚀 feature request

Relevant Rules

  • gazelle

Description

Python's gazelle doesn't currently support the default_visibility directive - it's hard-coded as

visibility := fmt.Sprintf("//%s:__subpackages__", pythonProjectRoot)

If you try and use it, you get this error:

gazelle: /c/dev/bazel-python-src-tests-example/BUILD: unknown directive: gazelle:default_visibility

Support for such should be added.

Describe the solution you'd like

Implement default_visibility 🙃. The same as how build-gazelle defines it:

Comma-separated list of visibility specifications. This directive adds the visibility specifications for this and descendant packages.

There are 4 questions that I've been able to come up with. I elaborate on them below.

  1. How to handle python_root?
  2. Should the directive be specific to just python?
  3. Should the current default be kept?
  4. Should we support "clearing/resetting" visibility?

Open Question 1: How to handle python_root?

rules_python's gazelle supports the python_root directive, which modifies the default visibility:

visibility := fmt.Sprintf("//%s:__subpackages__", pythonProjectRoot)

Do we want to support users injecting python_root or do we want them to hard code things?

Examples:

# Example of users injecting python_root
# gazelle:default_visibility //$python_root:__pkg__,//bar:__pkg__,//$python_root/foo:__subpackages

# assuming python_root = "./src", renders targets as:
py_library(
    ...,
    visibility = [
        "//bar:__pkg__",  # ordered alphabetically
        "//src:__pkg__",
        "//src/foo:__subpackages__",
    ],
    ...,
)

How would we support such injection? The above example is a linux var style, but we could also do %s formatting or {{ python_root }} templating or something else.

Or we could just say "Do it yourself, users!":

# Example of users injecting python_root
# gazelle:default_visibility //src:__pkg__,//bar:__pkg__,//src/foo:__subpackages

Open Question 2: Specific to just python?

I see 3 paths forward:

  1. Add only default_visibility
    • Same as the core bazel-gazelle project, affects all targets.
  2. Add only python_default_visibility
    • Only affects py_* targets.
  3. Add both default_visibility and python_default_visibility.
    • allows more customization and would be beneficial for multi-language projects, but is more complex for both gazelle code and user BUILD files.
    • python_default_visibility overrides default_visibility for py_* targets

Which should we strive for?

Open Question 3: Should the current default be kept?

Should the current default visivility be kept as fmt.Sprintf("//%s:__subpackages__", pythonProjectRoot)?

If not, what should it change to?

Open Question 4: Support for "clearing/resetting" visibility?

Do we want to add the ability to clear/reset visibility? Perhaps by supporting special labels NONE and DEFAULT?

Example:

# No directive set, use the default. Assuming that python_root is unset.
py_library(
    visibility = ["//:__subpackages__"],
)

# Set default vis.
# gazelle:default_visibility //src:__pkg__
py_library(
    visibility = ["//src:__pkg__"],
)

go_library(
    visibility = ["//src:__pkg__"],
)

# python_default_visibility overrides default_visibility for py_* targets only
# gazelle:python_default_visibility //foo:bar,//tests:baz
py_library(
    visibility = [
        "//foo:bar",
        "//tests:baz",
    ],
)

go_library(
    visibility = ["//src:__pkg__"],
)

# Here's the "reset"
# gazelle:python_default_visibility DEFAULT
py_library(
    visibility = ["//:__subpackages__"],
)

# gazelle:python_default_visibility NONE
py_library(
    visibility = [],  # in reality, this attribute would just be missing altogether
)

Describe alternatives you've considered

N/A

@dougthor42
Copy link
Collaborator Author

I've opened #1784 for this, making the following assumptions:

  1. How to handle python_root? Allow users to manually inject it via special string
  2. Should the directive be specific to just python? Just python
  3. Should the current default be kept? Yes
  4. Should we support "clearing/resetting" visibility? Yes

@dougthor42
Copy link
Collaborator Author

Fixed in #1784 and #1787.

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

No branches or pull requests

1 participant