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

[Gazelle][proto] How to automatically resolve protobuf target #1703

Open
cyrilhuang opened this issue Jan 18, 2024 · 8 comments · Fixed by #1933
Open

[Gazelle][proto] How to automatically resolve protobuf target #1703

cyrilhuang opened this issue Jan 18, 2024 · 8 comments · Fixed by #1933
Labels
gazelle Gazelle plugin related issues type: feature request

Comments

@cyrilhuang
Copy link

🐞 bug report

Affected Rule

gazelle resolve

Is this a regression?

No

Description

I have a python file with a proto definition imported. When I try to use gazelle to auto generate py_library target, it fails to validate dependency. I look into some code in bazel_gazelle/resolve/index.go, no proto target defined in there.

🔬 Minimal Reproduction

  
import a.b.c_pb2
  

and then run bazel run :py_gazelle

🔥 Exception or Error

  
gazelle: ERROR: failed to validate dependencies for target "//:check": "a.b.c_pb2" at line 11 from "check.py" is an invalid dependency: possible solutions:
	1. Add it as a dependency in the requirements.txt file.
	2. Instruct Gazelle to resolve to a known dependency using the gazelle:resolve directive.
	3. Ignore it with a comment '# gazelle:ignore a.b.c_pb2' in the Python file.

"a.b" at line 11 from "check.py" is an invalid dependency: possible solutions:
	1. Add it as a dependency in the requirements.txt file.
	2. Instruct Gazelle to resolve to a known dependency using the gazelle:resolve directive.
	3. Ignore it with a comment '# gazelle:ignore a.b' in the Python file.

"a" at line 11 from "check.py" is an invalid dependency: possible solutions:
	1. Add it as a dependency in the requirements.txt file.
	2. Instruct Gazelle to resolve to a known dependency using the gazelle:resolve directive.
	3. Ignore it with a comment '# gazelle:ignore a' in the Python file.


  

🌍 Your Environment

Operating System:

  
ubuntu 20.04
  

Output of bazel version:

  
Bazelisk version: v1.16.0
Build label: 6.2.1
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri Jun 2 16:59:58 2023 (1685725198)
Build timestamp: 1685725198
Build timestamp as int: 1685725198
  

Rules_python version:

  
0.25
  

Anything else relevant?
rules_python_gazelle_plugin version
0.28

@aignas aignas added type: feature request gazelle Gazelle plugin related issues labels Jan 18, 2024
@aignas
Copy link
Collaborator

aignas commented Jan 18, 2024

Right now you need to use the gazelle directive to tell gazelle how to resolve a particular proto import, it does not happen automatically.

@michael-christen
Copy link
Contributor

Just spitballing here, but would a reasonable solution be to create a script that queries all the protobuf targets from the workspace, associates them with their import path, and then updates a notated section of the top-level BUILD to define these gazelle directives?

Side note: It'd be nice if there was a mechanism to define gazelle directives outside of comments, eg) like stackb/rules_proto allows specification of a different configuration file: https://github.com/stackb/rules_proto?tab=readme-ov-file#yaml-configuration. Just checking that that's not a pattern that's more widely used around the gazelle infrastructure?

I'm curious what y'all think, and hopeful that my questions aren't too newbish, I'm just starting to familiarize myself with gazelle.

michael-christen added a commit to michael-christen/toolbox that referenced this issue Jan 22, 2024
Changes:
- Establish a mechanism for defining external python requirements via a
`requirements.in`, generating updates to a lock file with `bazel run
//:requirements.update`
- Configure gazelle python  and protobufs
- Configures via gazelle directives to use a file based approach, stick
with the BUILD configuration, disable go, default visibility to private
and resolve py imports
- Update dev_setup to install openjdk (java), tree, ranger, ag

New Commands:
```
bazel run //:requirements.update
bazel run //:gazelle_python_manifest.update
bazel run //:gazelle
```

New Tests:
```
//:requirements_test
//::gazelle_python_manifest.test
```

Notes:
- One of the recurring issues I have every time I deal with updating
WORKSPACE is slight incompatibilities with various repos that result in
incomprehensible output errors.
- stackb/rules_proto is one of the only things that seems to support
what I'm looking for, and while it is getting steady contributions
hasn't pubilshed a release in about a year.
https://github.com/rules-proto-grpc/rules_proto_grpc. Additionally, it
could be handy to look into what it would take to make my own "shitty"
version of these rules to learn more about what's going on in gazelle's
/ bazel's internals
- bzlmod migration didn't go well for me, most tutorials out there still
use WORKSPACE, might be good to look into in the future / figure out how
to migrate piecemeal, but bypassed this and 7.0.1 bazel upgrade for now

Related Issues:
- Noting the issue with requiring a gazelle directive for proto
libraries: bazelbuild/rules_python#1703
- bazelbuild/rules_python#1664 fixes the reason
I had to "disable" my py_binary, wait for that to release and then
update. I could probably patch this in, but don't quite know how 🤷
- Sounds like there's some talk around gazelle c++
bazel-contrib/bazel-gazelle#910
- I'm helping ;) bazelbuild/rules_python#1712
(doc update)

References:
- https://github.com/bazelbuild/bazel-gazelle
- https://github.com/stackb/rules_proto
- https://github.com/bazelbuild/rules_python/tree/main/gazelle

Future Things to Look Into:
- Setup a gazelle test that checks output of `//:gazelle -- -mode diff`
is zero
- Configure rust with gazelle: https://github.com/Calsign/gazelle_rust
- A really cool / general parser / generator (what github uses for
"semantic" tool): https://tree-sitter.github.io/tree-sitter/ Could use
to make small code generator / modification? Or maybe more general
utilities?
- General compile command completion:
https://github.com/hedronvision/bazel-compile-commands-extractor
- Maybe play with https://github.com/rules-proto-grpc/rules_proto_grpc
as an alternative to stackb
- If I can't use gazelle for some things, mess around with
https://github.com/bazelbuild/buildtools/tree/master/buildozer
@aignas
Copy link
Collaborator

aignas commented Jan 22, 2024

If I were starting to look into this I would first

  • look at what bazel-gazelle does for go proto library management (if I remember correctly there is an if statement somewhere that handles creation of proto targets).
  • copy that to rules python gazelle with modifications to use py_proto_library.

The second bullet point is where the difficulty lies and I don't know the details of how one would achieve that.

@cyrilhuang
Copy link
Author

@michael-christen
That's the only way I can think of to default enable py_proto_library auto resolution. But for a monorepo with abundant proto definitions, lots of directives will be in the top-level BUILD file.

As @aignas noted, gazelle supports go_proto_library resolution natively, users do not have to manually do query and update of proto denition. I think they achivev this through import path, but not sure about that. Hopefully this part of logic is integrated in python gazelle.

@michael-christen
Copy link
Contributor

Thanks y'all, if I find some time I'll look around there.

@vihangm
Copy link

vihangm commented Feb 14, 2024

We use stackb/rules_proto to generate our proto_py_library rules.

This patch vihangm@96c9aa0 maps the import to what's indexed by rules_proto and works (however I don't know if this is a great solution since it's leaking rules_proto implementation details).

github-merge-queue bot pushed a commit that referenced this issue Jun 5, 2024
Protobuf team is taking ownership of `py_proto_library` and the
implementation was moved to protobuf repository.

Remove py_proto_library from rules_python, to prevent divergent
implementations.

Make a redirect with a deprecation warning, so that this doesn't break
any users.

Expected side effect of this change is also that the protobuf version is
sufficiently updated that there is no more use of legacy struct
providers.

Closes #1935 
Closes #1924 
Closes #1925 
Closes #1703 
Closes #1707 
Closes #1597 
Closes #1293 
Closes #1080
Fixes #1438
@michael-christen
Copy link
Contributor

@aignas, just to clarify, does #1933 fix this or bypass how we'd handle this to the protobuf library?

@aignas aignas reopened this Jun 6, 2024
@aignas
Copy link
Collaborator

aignas commented Jun 6, 2024

I was closing all proto related issues with the move of the py_proto_library out from the rules_python. Might have include an issue too many...

So far no automatic discovery of proto rules is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gazelle Gazelle plugin related issues type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants