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_visibility" directive that appends additional visibility labels #1784

Merged
merged 10 commits into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 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_visibility label`](#directive-python_visibility) | |
| Appends additional visibility labels to each generated target. This directive can be set multiple times. | |


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


#### Directive: `python_visibility`:

Appends additional `visibility` labels to each generated target.

This directive can be set multiple times. The generated `visibility` attribute
will include the default visibility and all labels defined by this directive.
All labels will be ordered alphabetically.

```starlark
# ./BUILD.bazel
# gazelle:python_visibility //tests:__pkg__
# gazelle:python_visibility //bar:baz

py_library(
...
visibility = [
"//:__subpackages__", # default visibility
"//bar:baz",
"//tests:__pkg__",
],
...
)
```

Child Bazel packages inherit values from parents:

```starlark
# ./bar/BUILD.bazel
# gazelle:python_visibility //tests:__subpackages__

py_library(
...
visibility = [
"//:__subpackages__", # default visibility
"//bar:baz", # defined in ../BUILD.bazel
"//tests:__pkg__", # defined in ../BUILD.bazel
"//tests:__subpackages__", # defined in this ./BUILD.bazel
],
...
)

```


### Libraries

Python source files are those ending in `.py` but not ending in `_test.py`.
Expand Down
3 changes: 3 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.Visibility,
}
}

Expand Down Expand Up @@ -162,6 +163,8 @@ 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.Visibility:
config.AppendVisibility(strings.TrimSpace(d.Value))
}
}

Expand Down
3 changes: 2 additions & 1 deletion gazelle/python/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,8 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
}

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

var result language.GenerateResult
result.Gen = make([]*rule.Rule, 0)
Expand Down
8 changes: 5 additions & 3 deletions gazelle/python/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,11 @@ func (t *targetBuilder) addResolvedDependency(dep string) *targetBuilder {
return t
}

// addVisibility adds a visibility to the target.
func (t *targetBuilder) addVisibility(visibility string) *targetBuilder {
t.visibility.Add(visibility)
// addVisibility adds visibility labels to the target.
func (t *targetBuilder) addVisibility(visibility []string) *targetBuilder {
for _, item := range visibility {
t.visibility.Add(item)
}
Comment on lines +104 to +106
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 thought that I'd be able to do

t.visibility.Add(visibility...)

instead, but I would get this error:

python/target.go:104:19: cannot use visibility (variable of type []string) as type []interface{} in argument to t.visibility.Add

I'm relatively new to go and haven't use the gods.TreeSet package before, so I'm not sure why things don't work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can follow the pattern from above. See addModuleDependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... looks like doing so would mean having to adjust the arg type from []string to *treeset.Set, which would cascade to generate.go.

As-is, the diff is smaller and I'm always a fan of using built-in types when possible.

That said, LMK if you feel strongly that this func should be *treeset.Set and I'll be happy to change it.

return t
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Directives can be added in any order. They will be ordered alphabetically
# when added.
# gazelle:python_visibility //tests:__pkg__
# gazelle:python_visibility //bar:baz
16 changes: 16 additions & 0 deletions gazelle/python/testdata/directive_python_visibility/BUILD.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
load("@rules_python//python:defs.bzl", "py_library")

# Directives can be added in any order. They will be ordered alphabetically
# when added.
# gazelle:python_visibility //tests:__pkg__
# gazelle:python_visibility //bar:baz

py_library(
name = "directive_python_visibility",
srcs = ["foo.py"],
visibility = [
"//:__subpackages__",
"//bar:baz",
"//tests:__pkg__",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Directive: `python_visibility`

This test case asserts that the `# gazelle:python_visibility` directive correctly
appends multiple labels to the target's `visibility` parameter.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# This is a Bazel workspace for the Gazelle test data.
2 changes: 2 additions & 0 deletions gazelle/python/testdata/directive_python_visibility/foo.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,4 @@
# python_visibilty directive applies to all child bazel packages.
# Thus, the generated file for this package will also have vis for
# //tests:__pkg__ and //bar:baz in addition to the default.
# gazelle:python_visibility //tests:__subpackages__
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
load("@rules_python//python:defs.bzl", "py_library")

# python_visibilty directive applies to all child bazel packages.
# Thus, the generated file for this package will also have vis for
# //tests:__pkg__ and //bar:baz in addition to the default.
# gazelle:python_visibility //tests:__subpackages__

py_library(
name = "subdir",
srcs = [
"__init__.py",
"bar.py",
],
visibility = [
"//:__subpackages__",
"//bar:baz",
"//tests:__pkg__",
"//tests:__subpackages__",
],
)
Empty file.
Empty file.
17 changes: 17 additions & 0 deletions gazelle/python/testdata/directive_python_visibility/test.yaml
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
17 changes: 17 additions & 0 deletions gazelle/pythonconfig/pythonconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ const (
// naming convention. See python_library_naming_convention for more info on
// the package name interpolation.
TestNamingConvention = "python_test_naming_convention"
// Visibility represents the directive that controls what additional
// visibility labels are added to generated targets. It mimics the behavior
// of the `go_visibility` directive.
Visibility = "python_visibility"
)

// GenerationModeType represents one of the generation modes for the Python
Expand Down Expand Up @@ -136,6 +140,7 @@ type Config struct {
libraryNamingConvention string
binaryNamingConvention string
testNamingConvention string
visibility []string
}

// New creates a new Config.
Expand All @@ -157,6 +162,7 @@ func New(
libraryNamingConvention: packageNameNamingConventionSubstitution,
binaryNamingConvention: fmt.Sprintf("%s_bin", packageNameNamingConventionSubstitution),
testNamingConvention: fmt.Sprintf("%s_test", packageNameNamingConventionSubstitution),
visibility: []string{},
}
}

Expand All @@ -183,6 +189,7 @@ func (c *Config) NewChild() *Config {
libraryNamingConvention: c.libraryNamingConvention,
binaryNamingConvention: c.binaryNamingConvention,
testNamingConvention: c.testNamingConvention,
visibility: c.visibility,
}
}

Expand Down Expand Up @@ -388,3 +395,13 @@ func (c *Config) SetTestNamingConvention(testNamingConvention string) {
func (c *Config) RenderTestName(packageName string) string {
return strings.ReplaceAll(c.testNamingConvention, packageNameNamingConventionSubstitution, packageName)
}

// AppendVisibility adds additional items to the target's visibility.
func (c *Config) AppendVisibility(visibility string) {
c.visibility = append(c.visibility, visibility)
}

// Visibility returns the target's visibility.
func (c *Config) Visibility() []string {
return c.visibility
}
Loading