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 3 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
48 changes: 48 additions & 0 deletions gazelle/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,54 @@ 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) | |
| Append additional visibility labels to each generated target. This directive can be set multiple times. | |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| Append additional visibility labels to each generated target. This directive can be set multiple times. | |
| Appends additional visibility labels to each generated target. This directive can be set multiple times. | |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍



#### Directive: `python_visibility`:

Append additional `visibility` labels to each generated target.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Append additional `visibility` labels to each generated target.
Appends additional `visibility` labels to each generated target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


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

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.ExtraVisibility,
}
}

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.ExtraVisibility:
config.AppendExtraVisibility(strings.TrimSpace(d.Value))
}
}

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

parser := newPython3Parser(args.Config.RepoRoot, args.Rel, cfg.IgnoresDependency)
visibility := fmt.Sprintf("//%s:__subpackages__", pythonProjectRoot)
// TODO(gh-1682): Add support for default_visibility directive and replace
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, remove this TODO item since there's an issue already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

// the initil visibility value.
dougthor42 marked this conversation as resolved.
Show resolved Hide resolved
visibility := []string{fmt.Sprintf("//%s:__subpackages__", pythonProjectRoot)}
visibility = append(visibility, cfg.ExtraVisibility()...)

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"
// ExtraVisibility represents the directive that controls what additional
// visibility labels are added to generated targets. It mimics the behavior
// of the `go_visibility` directive.
ExtraVisibility = "python_visibility"
Copy link
Collaborator

Choose a reason for hiding this comment

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

To follow the pattern:

Suggested change
ExtraVisibility = "python_visibility"
Visibility = "python_visibility"

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, updated..

)

// 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
extraVisibility []string
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hoped you'd remove the extra everywhere.

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 could have sworn I did... Sorry about that, I definitely meant to.

...
...

Ah, looks like I stashed the changes instead of committing them. /facepalm. Anyway, updated 👍.

}

// 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),
extraVisibility: []string{},
}
}

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

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)
}

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

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