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

Remove deps attribute from go_proto_library by automatically inferring them #3668

Open
udaya2899 opened this issue Aug 22, 2023 · 4 comments · May be fixed by #3706
Open

Remove deps attribute from go_proto_library by automatically inferring them #3668

udaya2899 opened this issue Aug 22, 2023 · 4 comments · May be fixed by #3706

Comments

@udaya2899
Copy link

udaya2899 commented Aug 22, 2023

What version of rules_go are you using?

0.37.0

What version of gazelle are you using?

0.31.0

What version of Bazel are you using?

6.2.0

Does this issue reproduce with the latest releases of all the above?

Checking the release notes, the requested feature is not there yet

What operating system and processor architecture are you using?

NA

Any other potentially useful information about your toolchain?

NA

What did you do?

I wrote a go_proto_library rule for a corresponding proto_library rule.

For example,

proto_library(
    name = "foo_proto",
    deps = [":bar_proto"],
)

go_proto_library(
    name = "foo_go_proto",
    protos = [":foo_proto"],
    deps = [":bar_go_proto"],
)

What did you expect to see?

The deps attribute needs the corresponding bar_go_proto to be included. Since the protos attribute has the corresponding proto rules, it would be nice to see the bar_go_proto to be inferred by the rule automatically. In other words, we'd like to see the deps attribute removed altogether in the go_proto_library rule. This is how it works in blaze, and it is implemented using aspects there.

cc_proto_library from rules_cc and py_proto_library from rules_python already does this too.

Expected:

proto_library(
    name = "foo_proto",
    deps = [":bar_proto"],
)

go_proto_library(
    name = "foo_go_proto",
    protos = [":foo_proto"],
)

What did you see instead?

This feature is not part of the releases yet. I can also support in developing this but I'm not sure about the work needed and where.

@fmeum
Copy link
Collaborator

fmeum commented Aug 22, 2023

This would be a very welcome contribution. I can review PRs and offer support. I haven't worked with proto_common yet, but I would assume that https://github.com/bazelbuild/rules_python/blob/main/python/private/proto/py_proto_library.bzl is a good starting point. cc_proto_library still has some native parts.

CC @linzhp

@faximan
Copy link

faximan commented Aug 24, 2023

Another source of inspiration might be py_proto_library from gRPC: https://github.com/grpc/grpc/blob/master/bazel/python_rules.bzl#L160 as py_proto_library from rules_python is a recent addition.

That said, as someone unfamiliar with the internals of these libraries and aspects in general, I am struggling to reconcile these implementations with go_proto_library. Some comments would not have hurt :)

Very happy if someone more qualified would pick this up.

@udaya2899
Copy link
Author

Bump. @fmeum some specific pointers could helpsus. Like @faximan said, I find it hard to understand the implementations comparing to py_proto_library.

More specifically:

  1. Where in proto/def.bzl should I insert the logic that queries other deps and appends along with deps?
  2. Where in py_proto_library does this happen that I can take as a reference?
  3. If possible, could you outline the logic/steps to do for a first contributor like me?

@mering mering linked a pull request Sep 21, 2023 that will close this issue
@fmeum
Copy link
Collaborator

fmeum commented Sep 25, 2023

Sorry, I didn't get to answer your questions yet. There is now a draft PR: #3706

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 a pull request may close this issue.

3 participants