-
-
Notifications
You must be signed in to change notification settings - Fork 615
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?
feat(gazelle): Gazelle resolves module-level imports of py_proto_library
#3109
Conversation
…ts generated for it
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: LF@EO, here and elsewhere.
@@ -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 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
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 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
.
@@ -42,6 +42,8 @@ const ( | |||
pyTestEntrypointTargetname = "__test__" | |||
conftestFilename = "conftest.py" | |||
conftestTargetname = "conftest" | |||
protoRelKey = "_proto_rel" |
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?
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 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.
|
||
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 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.
Closes #1703. (I think this is only asking for module-level imports.)
This adds support in Gazelle for resolving module-level imports of
py_proto_library
rules. Note that this does not support:See the tests for examples of how this works. I would ideally like to implement the first one as well, but I think the "right" way to do that is to use something that's only present in Gazelle 0.38. So that'll have to wait on an upgrade of Gazelle in this repo; in the meanwhile, I think this is worth landing.