Implement py_proto_library

[comius]

PR Checklist

Please check if your PR fulfills the following requirements:

• [ ] Tests for the changes have been added (for bug fixes / features)
• [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• [ ] Bugfix
• [x] Feature (please, look at the "Scope of the project" section in the README.md file)
• [ ] Code style update (formatting, local variables)
• [ ] Refactoring (no functional changes, no api changes)
• [ ] Build related changes
• [ ] CI related changes
• [ ] Documentation content changes
• [ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• [ ] Yes
• [x] No

Other information

[comius]

cc @rickeylev @haberman

[aaliddell]

FWIW transitive proto compilation with an aspect is probably not the right method to use any longer. Both rules_proto and rules_proto_grpc have dropped aspect based compilation in favour of more explicit direct compilation of the named protos only within a target. This is because transitive compilation is both artificially limiting in terms of passable options and more importantly leads to multiple definitions of the same proto message if depended on via differing trees (a serious problem in C++, a nuisance in Python). See here for further details: https://rules-proto-grpc.com/en/latest/transitivity.html

[comius]

FWIW transitive proto compilation with an aspect is probably not the right method to use any longer. Both rules_proto and rules_proto_grpc have dropped aspect based compilation in favour of more explicit direct compilation of the named protos only within a target. This is because transitive compilation is both artificially limiting in terms of passable options and more importantly leads to multiple definitions of the same proto message if depended on via differing trees (a serious problem in C++, a nuisance in Python). See here for further details: https://rules-proto-grpc.com/en/latest/transitivity.html

Compiling only directly is a good choice for gRPC generated libraries.

For non-service proto libraries, it's better to use lang_proto_library with an aspect. It removes redundancies - having to add py_proto_library next to every proto_library in the transitive closure. Aspects take care that you're 1:1 mapping from proto dependency graph to python graph - even if lang_proto_library is duplicated. If there are problems with proto dependency graph, they are not going to be solved by either direct or aspect based solution. Aspect based solution will probably fail, whereas no-aspect/direct solution will potentially hide problems.

[rickeylev]

https://rules-proto-grpc.com/en/latest/transitivity.html says

In hindsight, [recursive aspect] was not the correct behaviour and led to many bugs, since you may end up creating a library that contains compiled proto files from a third party, where you should instead be depending on a proper library for that third party’s protos.

Is there a place to further discuss this (this PR is probably not the best venue)? This situation sounds similar to some of the problems we have with SWIG (and C bindings in general) within Google, and pushing the codegen burden elsewhere has its own set of problems. @aaliddell

[aaliddell]

Is there a place to further discuss this (this PR is probably not the best venue)? This situation sounds similar to some of the problems we have with SWIG (and C bindings in general) within Google, and pushing the codegen burden elsewhere has its own set of problems. @aaliddell

You can email me or put it in a new discussion here; not sure I’ll be much help though with SWIG. Or put it on the bazel-discuss mailing list if you want a wider audience. Or slack. Too many options 😵‍💫

Compiling only directly is a good choice for gRPC generated libraries.

For non-service proto libraries, it's better to use lang_proto_library with an aspect. It removes redundancies - having to add py_proto_library next to every proto_library in the transitive closure. Aspects take care that your 1:1 mapping from proto dependency graph to python graph - even if lang_proto_library is duplicated. If there are problems with proto dependency graph, they are not going to be solved by either direct or aspect based solution. Aspect based solution will probably fail, whereas no-aspect/direct solution will potentially hide problems.

Ok, I thought I’d check your weren’t just blindly copying some of the other language rulesets’ behaviour without knowing the implications; seems like you’re on top of it 👍

[rickeylev]

per chat with Ivo: PR is not yet ready to merge; he's going to refactor things into a single bzlmod module file because deps for modules are fetched lazily (and thus no proto dep will be brought in unless necessary)

[tpudlik]

I just ran into an interesting bug in the unofficial py_proto_library from https://github.com/protocolbuffers/protobuf/: it doesn't respect tags = ["manual"]. No tags are forwarded to the code generation rule, so the code generation runs unconditionally. So I thought I'd confirm: this "official" implementation doesn't suffer from this bug, does it?