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

Use aspect to infer proto deps automatically #3706

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mering
Copy link
Contributor

@mering mering commented Sep 21, 2023

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This automatically infers transitive proto deps with an aspect to avoid manually maintaining two parallel dependency trees (proto deps and go_proto deps).

Which issues(s) does this PR fix?

Fixes #3668

Other notes for review

Testing repository: https://github.com/mering/test-rules-go

@mering
Copy link
Contributor Author

mering commented Sep 27, 2023

@fmeum Could I have your input on this? I am quite new to Bazel and therefore any input is highly appreciated!

The approach in this PR is to generate the go proto bindings within an aspect on the proto_library rules instead of on a go_proto_library rule. One problem I am facing because of this is that it is now no longer possible to pass down attributes to the aspect. So one could either overwrite defaults (like importpath) on the proto_library rule instead of the go_proto_library one and access it somehow from there. Another alternative could be to use something like an importpath_prefix on a package level or similar. Same problem for compilers which are essential (e.g. for gRPC). I have seen some TODO comments regarding gRPC, so it probably makes sense to align with your plans as this will probably be a breaking change anyways.

So any input to my general approach or ideas on how to move forward is highly appreciated!

@mering
Copy link
Contributor Author

mering commented Oct 2, 2023

One idea which I just had is to use separate aspects for compilers (might still share the same implementation but need to set the _compiler attribute differently). Then the go_proto_library rule would just attach the proto aspect while the go_grpc_library would attach both the proto and the grpc aspect. Could this work?

@mering
Copy link
Contributor Author

mering commented Oct 9, 2023

@fmeum I would really appreciate your help here. Thanks in advance!

@fmeum
Copy link
Collaborator

fmeum commented Oct 9, 2023

@fmeum I would really appreciate your help here. Thanks in advance!

Sorry for the delay, I do really appreciate your work on this. I can't promise that I will get to this before BazelCon as I'm somewhat busy preparing for it. I am not that knowledgeable about gRPC, so I will have to delve into that a bit before I can provide meaningful input.

CC @linzhp just in case you already have some ideas in mind

@mering
Copy link
Contributor Author

mering commented Oct 9, 2023

@fmeum I would really appreciate your help here. Thanks in advance!

Sorry for the delay, I do really appreciate your work on this. I can't promise that I will get to this before BazelCon as I'm somewhat busy preparing for it. I am not that knowledgeable about gRPC, so I will have to delve into that a bit before I can provide meaningful input.

Thanks for your answer. Take your time. Looking forward for your comments.

@linzhp
Copy link
Contributor

linzhp commented Oct 9, 2023

The approach in this PR is to generate the go proto bindings within an aspect on the proto_library rules instead of on a go_proto_library rule

proto_library rule should be language agnostic. You should pass any information related to Go in go_proto_library and its aspect. One implementation you can refer to is the py_proto_library in rules_python.

@mering
Copy link
Contributor Author

mering commented Oct 9, 2023

The approach in this PR is to generate the go proto bindings within an aspect on the proto_library rules instead of on a go_proto_library rule

proto_library rule should be language agnostic. You should pass any information related to Go in go_proto_library and its aspect. One implementation you can refer to is the py_proto_library in rules_python.

Isn't this exactly what I am doing? I tried to base my implementation on the Python one... Please correct me if I mix things up or chose the wrong words as I am still quite new to Bazel and especially aspects.

proto/def.bzl Outdated Show resolved Hide resolved
proto/def.bzl Outdated Show resolved Hide resolved
@mering mering force-pushed the aspect branch 8 times, most recently from e9cb3cd to b51cfe9 Compare November 23, 2023 13:15
to org_golang_google_protobuf and org_golang_google_grpc.
This is best practice in order to avoid clashing with other aspects who also want to generate go libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove deps attribute from go_proto_library by automatically inferring them
3 participants