-
-
Notifications
You must be signed in to change notification settings - Fork 616
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
base: main
Are you sure you want to change the base?
Changes from all commits
d07619f
49248b6
083f3ee
2878c0d
9ae7af6
50980b1
706809c
af8ff34
a7c03c6
e9a29f1
88b14ca
8262b7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not always true. Depending on
I think we'll need a test case for
In the above tree, 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 ( |
||
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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Future work: if the proto import is guarded with |
||
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 | ||
|
@@ -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, ".") | ||
} | ||
|
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. |
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"], | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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:
|
||
|
||
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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a test case with One |
||
], | ||
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; | ||
} |
There was a problem hiding this comment.
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?