Skip to content

feat(gazelle): Gazelle resolves module-level imports of py_proto_library #3109

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ END_UNRELEASED_TEMPLATE
Gazelle generates `py_proto_library` rules for `proto_library`. `false` by default.
* (gazelle) New directive `gazelle:python_proto_naming_convention`; controls
naming of `py_proto_library` rules.
* (gazelle) Gazelle now resolves dependencies for `py_proto_library`
module-level imports, i.e. `import some.package.foo_pb2`. Imports of
messages/enums/services inside modules are not yet supported.

{#v0-0-0-removed}
### Removed
Expand Down
2 changes: 1 addition & 1 deletion gazelle/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ py_proto_library(

The default naming convention is `$proto_name$_pb2_py`, so by default in the above example Gazelle would generate `foo_pb2_py`. Any pre-existing rules are left in place and not renamed.

Note that the Python library will always be imported as `foo_pb2` in Python code, regardless of the naming convention. Also note that Gazelle is currently not able to map said imports, e.g. `import foo_pb2`, to fill in `py_proto_library` targets as dependencies of other rules. See [this issue](https://github.com/bazel-contrib/rules_python/issues/1703).
Note that the Python library will always be imported as `foo_pb2` in Python code, regardless of the naming convention.

#### Directive: `python_default_visibility`:

Expand Down
10 changes: 10 additions & 0 deletions gazelle/python/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ const (
pyTestEntrypointTargetname = "__test__"
conftestFilename = "conftest.py"
conftestTargetname = "conftest"
protoRelKey = "_proto_rel"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's Rel in this context?

protoSrcsKey = "_proto_srcs"
)

var (
Expand Down Expand Up @@ -572,11 +574,16 @@ func ensureNoCollision(file *rule.File, targetName, kind string) error {
func generateProtoLibraries(args language.GenerateArgs, cfg *pythonconfig.Config, pythonProjectRoot string, visibility []string, res *language.GenerateResult) {
// First, enumerate all the proto_library in this package.
var protoRuleNames []string
protoRel := map[string]string{}
protoSrcs := map[string][]string{}

for _, r := range args.OtherGen {
if r.Kind() != "proto_library" {
continue
}
protoRuleNames = append(protoRuleNames, r.Name())
protoRel[r.Name()] = args.Rel
protoSrcs[r.Name()] = r.AttrStrings("srcs")
}
sort.Strings(protoRuleNames)

Expand Down Expand Up @@ -610,6 +617,9 @@ func generateProtoLibraries(args language.GenerateArgs, cfg *pythonconfig.Config
addResolvedDependency(":" + protoRuleName).
generateImportsAttribute().build()

pyProtoLibrary.SetPrivateAttr(protoRelKey, protoRel[protoRuleName])
pyProtoLibrary.SetPrivateAttr(protoSrcsKey, protoSrcs[protoRuleName])

res.Gen = append(res.Gen, pyProtoLibrary)
res.Imports = append(res.Imports, pyProtoLibrary.PrivateAttr(config.GazelleImportsKey))
pyProtoRules[pyProtoLibrary.Name()] = true
Expand Down
41 changes: 38 additions & 3 deletions gazelle/python/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,18 @@ func (*Resolver) Name() string { return languageName }
func (py *Resolver) Imports(c *config.Config, r *rule.Rule, f *rule.File) []resolve.ImportSpec {
cfgs := c.Exts[languageName].(pythonconfig.Configs)
cfg := cfgs[f.Pkg]

srcs := r.AttrStrings("srcs")
if srcs != nil {
return importsSrcLibrary(cfg, srcs, f)
} else if isProtoLibrary(r) {
return importsProtoLibrary(cfg, r)
}

return nil
}

func importsSrcLibrary(cfg *pythonconfig.Config, srcs []string, f *rule.File) []resolve.ImportSpec {
provides := make([]resolve.ImportSpec, 0, len(srcs)+1)
for _, src := range srcs {
ext := filepath.Ext(src)
Expand Down Expand Up @@ -114,6 +125,30 @@ func importSpecFromSrc(pythonProjectRoot, bzlPkg, src string) resolve.ImportSpec
}
}

func isProtoLibrary(r *rule.Rule) bool {
return r.Kind() == pyProtoLibraryKind
}

func importsProtoLibrary(cfg *pythonconfig.Config, r *rule.Rule) []resolve.ImportSpec {
specs := []resolve.ImportSpec{}

// Determine the root module and emit an import for that,
// i.e. for //foo:foo_py_pb2, we'd get foo.foo_pb2
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not always true. Depending on # gazelle:python_root, any one of these is possible:

//some/path/foo:foo_py_pb2
foo.foo_pb2                     # python_root is //some/path
path.foo.foo_pb2                # python_root is //some
some.path.foo.foo_pb2           # python_root is project root, default case.

I think we'll need a test case for # gazelle:python_root.

.
+ src/
  + BUILD.bazel        # gazelle:python_root
  + my_pkg/
    + protos/
      + foo.proto
      + BUILD.bazel
    + subpkg/
    + __init__.py
    + main.py
    + foo.py
    + BUILD.bazel
pyproject.toml
deploy.py
BUILD.bazel

In the above tree, src/my_pkg/main.py will have:

import my_pkg.foo as foo
import my_pkg.subpkg.bar as bar
import my_pkg.proto.foo_pb2 as foo_pb2

While the target will be:

# src/my_pkg/BUILD.bazel
py_library(
    name = "main",
    srcs = ["main.py"],
    imports = "..",
    deps = [
        "//src/my_pkg:foo",
        "//src/my_pkg/subpkg:bar",
        "//src/my_pkg/proto:foo_py_pb2",
    ],
)

Specifically: the Bazel target will include information (src) not found in the python import string.

protoRelAttr := r.PrivateAttr(protoRelKey)
protoSrcsAttr := r.PrivateAttr(protoSrcsKey)
if protoRelAttr == nil || protoSrcsAttr == nil {
return nil
}

protoRel := protoRelAttr.(string)
for _, protoSrc := range protoSrcsAttr.([]string) {
generatedPbFileName := strings.TrimSuffix(protoSrc, ".proto") + "_pb2.py"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Future work: if the proto import is guarded with if TYPE_CHECKING, then only include the _pb2.pyi file in the pyi_deps target attribute.

specs = append(specs, importSpecFromSrc(cfg.PythonProjectRoot(), protoRel, generatedPbFileName))
}

return specs
}

// Embeds returns a list of labels of rules that the given rule embeds. If
// a rule is embedded by another importable rule of the same language, only
// the embedding rule will be indexed. The embedding rule will inherit
Expand Down Expand Up @@ -210,9 +245,9 @@ func (py *Resolver) Resolve(
baseParts = pkgParts[:len(pkgParts)-(relativeDepth-1)]
}
// Build absolute module path
absParts := append([]string{}, baseParts...) // base path
absParts = append(absParts, fromParts...) // subpath from 'from'
absParts = append(absParts, imported) // actual imported symbol
absParts := append([]string{}, baseParts...) // base path
absParts = append(absParts, fromParts...) // subpath from 'from'
absParts = append(absParts, imported) // actual imported symbol

moduleName = strings.Join(absParts, ".")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ correctly:
1. Has no effect on pre-existing `py_proto_library` when `gazelle:python_generate_proto` is disabled.
2. Uses the default value when proto generation is on and `python_proto_naming_convention` is not set.
3. Uses the provided naming convention when proto generation is on and `python_proto_naming_convention` is set.
4. With a pre-existing `py_proto_library` not following a given naming convention, keeps it intact and does not rename it.
4. With a pre-existing `py_proto_library` not following a given naming convention, keeps it intact and does not rename it.
Empty file.
Empty file.
8 changes: 8 additions & 0 deletions gazelle/python/testdata/resolves_proto_imports/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Resolves proto imports

This test asserts that Gazelle can resolve imports from `py_proto_library` targets:

1. Generates a dependency in the default case.
2. Uses `gazelle:resolve` to generate dependencies.
3. Uses `python_proto_naming_convention` to generate dependencies.
4. Generates a correct dependency for a proto_library with multiple srcs.
Empty file.
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
load("@rules_python//python:defs.bzl", "py_library")

py_library(
name = "test1_generates_dependency",
srcs = ["bar.py"],
visibility = ["//:__subpackages__"],
deps = ["//test1_generates_dependency/foo:test1_generates_dependency_foo_py_pb2"],
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: LF@EO, here and elsewhere.

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import test1_generates_dependency.foo.foo_pb2

x = foo_pb2.Foo()
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_generate_proto true
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
load("@rules_proto//proto:defs.bzl", "proto_library")
load("@com_google_protobuf//bazel:py_proto_library.bzl", "py_proto_library")

# gazelle:python_generate_proto true

proto_library(
name = "test1_generates_dependency_foo_proto",
srcs = ["foo.proto"],
visibility = ["//visibility:public"],
)

py_proto_library(
name = "test1_generates_dependency_foo_py_pb2",
visibility = ["//:__subpackages__"],
deps = [":test1_generates_dependency_foo_proto"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
syntax = "proto3";

package test1_generates_dependency.foo;

message Foo {
bool bar = 1;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:resolve py test2_generates_using_resolve.bar.bar_pb2 //test2_generates_using_resolve/bar:bar_py_pb2
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
load("@rules_python//python:defs.bzl", "py_library")

# gazelle:resolve py test2_generates_using_resolve.bar.bar_pb2 //test2_generates_using_resolve/bar:bar_py_pb2
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand this test case - can you elaborate? Unless I'm blind, it looks like you're resolving the same thing that gazelle would resolve by default. So... what's that testing?

I would assume that using a resolve directive would typically be for cases where, say, the bar_py_pb2 py_proto_library doesn't exist but we want Gazelle to resolve import test2_generates_using_resolve.bar.bar_pb2 to something else.

Yes, I'm quite blind I guess.

Could you adjust the resolve directive to be easier to ID that it's resolving to a target that doesn't exist? Maybe:

# gazelle:resolve py test2_generates_using_resolve.bar.bar_pb2 //some:target


py_library(
name = "test2_generates_using_resolve",
srcs = ["baz.py"],
visibility = ["//:__subpackages__"],
deps = ["//test2_generates_using_resolve/bar:bar_py_pb2"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_generate_proto true
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
load("@rules_proto//proto:defs.bzl", "proto_library")
load("@com_google_protobuf//bazel:py_proto_library.bzl", "py_proto_library")

# gazelle:python_generate_proto true

proto_library(
name = "test2_generates_using_resolve_bar_proto",
srcs = ["bar.proto"],
visibility = ["//visibility:public"],
)

py_proto_library(
name = "test2_generates_using_resolve_bar_py_pb2",
visibility = ["//:__subpackages__"],
deps = [":test2_generates_using_resolve_bar_proto"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
syntax = "proto3";

package test2_generates_using_resolve.bar;

message Bar {
bool bar = 1;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import test2_generates_using_resolve.bar.bar_pb2

x = bar_pb2.Bar()
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_proto_naming_convention some_$proto_name$_value
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
load("@rules_python//python:defs.bzl", "py_library")

# gazelle:python_proto_naming_convention some_$proto_name$_value

py_library(
name = "test3_uses_naming_convention",
srcs = ["baz.py"],
visibility = ["//:__subpackages__"],
deps = ["//test3_uses_naming_convention/bar:some_test3_uses_naming_convention_bar_value"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_generate_proto true
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
load("@rules_proto//proto:defs.bzl", "proto_library")
load("@com_google_protobuf//bazel:py_proto_library.bzl", "py_proto_library")

# gazelle:python_generate_proto true

proto_library(
name = "test3_uses_naming_convention_bar_proto",
srcs = ["bar.proto"],
visibility = ["//visibility:public"],
)

py_proto_library(
name = "some_test3_uses_naming_convention_bar_value",
visibility = ["//:__subpackages__"],
deps = [":test3_uses_naming_convention_bar_proto"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
syntax = "proto3";

package test3_uses_naming_convention.bar;

message Bar {
bool bar = 1;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import test3_uses_naming_convention.bar.bar_pb2

x = bar_pb2.Bar()
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
load("@rules_python//python:defs.bzl", "py_library")

py_library(
name = "test4_generates_imports_for_multiple_proto_srcs",
srcs = ["bar.py"],
visibility = ["//:__subpackages__"],
deps = ["//test4_generates_imports_for_multiple_proto_srcs/foo:test4_generates_imports_for_multiple_proto_srcs_foo_py_pb2"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import test4_generates_imports_for_multiple_proto_srcs.foo.foo_pb2
import test4_generates_imports_for_multiple_proto_srcs.foo.bar_pb2

x = foo_pb2.Foo()
y = bar_pb2.Bar()
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# gazelle:python_generate_proto true
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
load("@rules_proto//proto:defs.bzl", "proto_library")
load("@com_google_protobuf//bazel:py_proto_library.bzl", "py_proto_library")

# gazelle:python_generate_proto true

proto_library(
name = "test4_generates_imports_for_multiple_proto_srcs_foo_proto",
srcs = [
"bar.proto",
"foo.proto",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test case with # gazelle:proto file mode?

One proto_library per .proto --> one py_proto_library per proto_library --> the final py_library will have multiple deps.

],
visibility = ["//visibility:public"],
)

py_proto_library(
name = "test4_generates_imports_for_multiple_proto_srcs_foo_py_pb2",
visibility = ["//:__subpackages__"],
deps = [":test4_generates_imports_for_multiple_proto_srcs_foo_proto"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
syntax = "proto3";

package test4_generates_imports_for_multiple_proto_srcs.foo;

message Bar {
bool bar = 1;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
syntax = "proto3";

package test4_generates_imports_for_multiple_proto_srcs.foo;

message Foo {
bool bar = 1;
}