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

feat(gazelle): Add "python_default_visibility" directive #1787

Merged
merged 14 commits into from
Mar 17, 2024
80 changes: 80 additions & 0 deletions gazelle/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ Python-specific directives are as follows:
| Controls the `py_test` naming convention. Follows the same interpolation rules as `python_library_naming_convention`. | |
| `# gazelle:resolve py ...` | n/a |
| Instructs the plugin what target to add as a dependency to satisfy a given import statement. The syntax is `# gazelle:resolve py import-string label` where `import-string` is the symbol in the python `import` statement, and `label` is the Bazel label that Gazelle should write in `deps`. | |
| [`# gazelle:python_default_visibility labels`](#directive-python_default_visibility) | |
| Instructs gazelle to use these visibility labels on all python targets. `labels` is a comma-separated list of labels (without spaces). | `//$python_root:__subpackages__` |
| [`# gazelle:python_visibility label`](#directive-python_visibility) | |
| Appends additional visibility labels to each generated target. This directive can be set multiple times. | |

Expand Down Expand Up @@ -238,6 +240,84 @@ py_libary(
[python-packaging-user-guide]: https://github.com/pypa/packaging.python.org/blob/4c86169a/source/tutorials/packaging-projects.rst


#### Directive: `python_default_visibility`:

Instructs gazelle to use these visibility labels on all _python_ targets
(typically `py_*`, but can be modified via the `map_kind` directive). The arg
to this directive is a a comma-separated list (without spaces) of labels.

For example:

```starlark
# gazelle:python_default_visibility //:__subpackages__,//tests:__subpackages__
```

produces the following visibility attribute:

```starlark
py_library(
...,
visibility = [
"//:__subpackages__",
"//tests:__subpackages__",
],
...,
)
```

You can also inject the `python_root` value by using the exact string
`$python_root`:

```starlark
# an absurd example
dougthor42 marked this conversation as resolved.
Show resolved Hide resolved
# gazelle:python_default_visibility //$python_root:__pkg__,//foo/$python_root/tests:__subpackages__,//$python_root/$python_root/foo:bar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more, //$python_root/$python_root/foo:bar will probably confuse users. Let's keep this more straightforward and remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

The purpose was to highlight that all instances of $python_root would get replaced and that it doesn't have to be at the start of the target. It's probably clearer to just say that explicitly.


# assuming the "# gazelle:python_root" directive is set in ./py/src/BUILD.bazel
# results in:
dougthor42 marked this conversation as resolved.
Show resolved Hide resolved
py_library(
...,
visibility = [
"//foo/py/src/tests:__subpackages__", # sorted alphabetically
"//py/src/py/src/foo:bar",
"//py/src:__pkg__",
],
...,
)
```

Two special values are also accepted as an arg to the directive:
dougthor42 marked this conversation as resolved.
Show resolved Hide resolved

+ `NONE`: This removes all default visibility. Labels added by the
`python_visibility` directive are still included.
+ `DEFAULT`: This resets the default visibility.

For example:

```starlark
# gazelle:python_default_visibility NONE

# results in no `visibility` attribute being set:
dougthor42 marked this conversation as resolved.
Show resolved Hide resolved
py_library(
name = "...",
srcs = [...],
)
```

```starlark
# gazelle:python_default_visibility //foo:bar
# gazelle:python_default_visibility DEFAULT

# results in:
dougthor42 marked this conversation as resolved.
Show resolved Hide resolved
py_library(
...,
visibility = ["//:__subpackages__"],
...,
)
```

These special values can be useful for descendant `BAZEL.build` packages.
dougthor42 marked this conversation as resolved.
Show resolved Hide resolved


#### Directive: `python_visibility`:

Appends additional `visibility` labels to each generated target.
Expand Down
18 changes: 18 additions & 0 deletions gazelle/python/configure.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func (py *Configurer) KnownDirectives() []string {
pythonconfig.LibraryNamingConvention,
pythonconfig.BinaryNamingConvention,
pythonconfig.TestNamingConvention,
pythonconfig.DefaultVisibilty,
pythonconfig.Visibility,
}
}
Expand Down Expand Up @@ -99,6 +100,8 @@ func (py *Configurer) Configure(c *config.Config, rel string, f *rule.File) {
}

gazelleManifestFilename := "gazelle_python.yaml"
// TODO: figure out how to keep this in sync with pythonconfig.go New *Config
defaultVisibilityFmtString := "//%s:__subpackages__"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this an exported const in the pythonconfig.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.


for _, d := range f.Directives {
switch d.Key {
Expand All @@ -119,6 +122,7 @@ func (py *Configurer) Configure(c *config.Config, rel string, f *rule.File) {
}
case pythonconfig.PythonRootDirective:
config.SetPythonProjectRoot(rel)
config.SetDefaultVisibility([]string{fmt.Sprintf(defaultVisibilityFmtString, rel)})
case pythonconfig.PythonManifestFileNameDirective:
gazelleManifestFilename = strings.TrimSpace(d.Value)
case pythonconfig.IgnoreFilesDirective:
Expand Down Expand Up @@ -163,6 +167,20 @@ func (py *Configurer) Configure(c *config.Config, rel string, f *rule.File) {
config.SetBinaryNamingConvention(strings.TrimSpace(d.Value))
case pythonconfig.TestNamingConvention:
config.SetTestNamingConvention(strings.TrimSpace(d.Value))
case pythonconfig.DefaultVisibilty:
switch directiveArg := strings.TrimSpace(d.Value); directiveArg {
case "NONE":
config.SetDefaultVisibility([]string{})
case "DEFAULT":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought of "RESET" instead.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think DEFAULT is more self-descriptive.

pythonProjectRoot := config.PythonProjectRoot()
defaultVisibility := fmt.Sprintf(defaultVisibilityFmtString, pythonProjectRoot)
config.SetDefaultVisibility([]string{defaultVisibility})
default:
// Handle injecting the python root. Assume that the user used the
// exact string "$python_root".
labels := strings.ReplaceAll(directiveArg, "$python_root", config.PythonProjectRoot())
config.SetDefaultVisibility(strings.Split(labels, ","))
}
case pythonconfig.Visibility:
config.AppendVisibility(strings.TrimSpace(d.Value))
}
Expand Down
3 changes: 1 addition & 2 deletions gazelle/python/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
}

parser := newPython3Parser(args.Config.RepoRoot, args.Rel, cfg.IgnoresDependency)
visibility := []string{fmt.Sprintf("//%s:__subpackages__", pythonProjectRoot)}
visibility = append(visibility, cfg.Visibility()...)
visibility := cfg.Visibility()

var result language.GenerateResult
result.Gen = make([]*rule.Rule, 0)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Directive: `python_default_visibility`

This test case asserts that the `# gazelle:python_default_visibility` directive
correctly:

1. Uses the default value when `python_default_visibility` is not set.
2. Uses the correct default value when `python_root` is set and
`python_default_visibility` is not set.
3. Supports injecting `python_root`
4. Supports multiple labels
5. Setting the label to "NONE" removes all visibility attibutes.
6. Setting the label to "DEFAULT" reverts to using the default.
7. Adding `python_visibility` directive with `python_default_visibility NONE`
only adds the items listed by `python_visibility`.
8. Multiple `python_root` dirs [GH #1682](gh-1682) uses correct value when
injecting `python_root`.


[gh-1682]: https://github.com/bazelbuild/rules_python/issues/1682
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# This is a Bazel workspace for the Gazelle test data.
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Copyright 2023 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

---
expect:
exit_code: 0
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# python_default_visibility is not set.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
load("@rules_python//python:defs.bzl", "py_library")

# python_default_visibility is not set.

py_library(
name = "test1_default",
srcs = ["test1.py"],
visibility = ["//:__subpackages__"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def func():
print("library_func")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_root
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
load("@rules_python//python:defs.bzl", "py_library")

# gazelle:python_root

py_library(
name = "test2_default_with_python_root",
srcs = [
"__init__.py",
"test2.py",
],
visibility = ["//test2_default_with_python_root:__subpackages__"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def func():
print("library_func")
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# gazelle:python_root
# gazelle:python_default_visibility //foo/$python_root/bar:__pkg__
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
load("@rules_python//python:defs.bzl", "py_library")

# gazelle:python_root
# gazelle:python_default_visibility //foo/$python_root/bar:__pkg__

py_library(
name = "test3_injection",
srcs = [
"__init__.py",
"test3.py",
],
visibility = ["//foo/test3_injection/bar:__pkg__"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def func():
print("library_func")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_default_visibility //foo/bar:__pkg__,//tests:__subpackages__,//a:b
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
load("@rules_python//python:defs.bzl", "py_library")

# gazelle:python_default_visibility //foo/bar:__pkg__,//tests:__subpackages__,//a:b

py_library(
name = "test4_multiple_labels",
srcs = ["test4.py"],
visibility = [
"//a:b",
"//foo/bar:__pkg__",
"//tests:__subpackages__",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def func():
print("library_func")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_default_visibility NONE
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
load("@rules_python//python:defs.bzl", "py_library")

# gazelle:python_default_visibility NONE

py_library(
name = "test5_none_label",
srcs = ["test5.py"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def func():
print("library_func")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_default_visibility //foo:bar
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
load("@rules_python//python:defs.bzl", "py_library")

# gazelle:python_default_visibility //foo:bar

py_library(
name = "test6_default_label",
srcs = ["test6.py"],
visibility = ["//foo:bar"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Reset the default visibility to the default for all child packages.
# gazelle:python_default_visibility DEFAULT
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
load("@rules_python//python:defs.bzl", "py_library")

# Reset the default visibility to the default for all child packages.
# gazelle:python_default_visibility DEFAULT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we set # gazelle:python_visibility together with # gazelle:python_default_visibility?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a test case that uses # gazelle:python_visibility on a sub-package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test8 sets # gazelle:python_visibility on the root and then # gazelle:python_default_visibility on a sub-package.

Added the inverse of that as test9.


py_library(
name = "subpkg",
srcs = ["test6_sub.py"],
visibility = ["//:__subpackages__"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def func():
print("library_func")
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def func():
print("library_func")
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# python_visibility directives that happen either before _or_ after the
# NONE reset both get applied.
# gazelle:python_visibility //foo:bar
# gazelle:python_default_visibility NONE
# gazelle:python_visibility //bar:baz
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
load("@rules_python//python:defs.bzl", "py_library")

# python_visibility directives that happen either before _or_ after the
# NONE reset both get applied.
# gazelle:python_visibility //foo:bar
# gazelle:python_default_visibility NONE
# gazelle:python_visibility //bar:baz

py_library(
name = "test7_none_label_with_extra_vis",
srcs = ["test7.py"],
visibility = [
"//bar:baz",
"//foo:bar",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def func():
print("library_func")
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# For funzies, also throw in some additional visibility.
# gazelle:python_visibility //tests:__pkg__
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# For funzies, also throw in some additional visibility.
# gazelle:python_visibility //tests:__pkg__
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_root
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_root
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# proj1 depends on proj2
# We can leave the default visibility.
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
load("@rules_python//python:defs.bzl", "py_library")

# proj1 depends on proj2
# We can leave the default visibility.

py_library(
name = "pkg1",
srcs = ["file1.py"],
imports = [".."],
visibility = [
"//test8_multiple_python_root_dirs/proj1/src:__subpackages__",
"//tests:__pkg__",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_root
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_root
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# proj1 depends on proj2
# So we have to make sure that proj2 is visible by proj1
# gazelle:python_default_visibility //$python_root:__subpackages__,//test8_multiple_python_root_dirs/proj1/src:__subpackages__
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
load("@rules_python//python:defs.bzl", "py_library")

# proj1 depends on proj2
# So we have to make sure that proj2 is visible by proj1
# gazelle:python_default_visibility //$python_root:__subpackages__,//test8_multiple_python_root_dirs/proj1/src:__subpackages__

py_library(
name = "pkg2",
srcs = ["file2.py"],
imports = [".."],
visibility = [
"//test8_multiple_python_root_dirs/proj1/src:__subpackages__",
"//test8_multiple_python_root_dirs/proj2/src:__subpackages__",
"//tests:__pkg__",
],
)
Loading
Loading