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

Relation (merge?) with bazelbuild/rules_proto #15

Closed
qzmfranklin opened this issue Aug 13, 2019 · 7 comments
Closed

Relation (merge?) with bazelbuild/rules_proto #15

qzmfranklin opened this issue Aug 13, 2019 · 7 comments

Comments

@qzmfranklin
Copy link
Contributor

Google started a repo: https://github.com/bazelbuild/rules_proto

How much sense does it make to merge this repo with that?

@aaliddell
Copy link
Member

That repo contains (or will contain) replacements for the presently native proto_library rule and associated providers etc, so serves a somewhat different purpose than this repo. I did check if there was desire to merge prior to forking this repo, but there was not.

@aaliddell
Copy link
Member

Regarding the question you posted in stackb/rules_proto#116, the context can be found at stackb/rules_proto#99

The reason this is not a GitHub ‘fork’ is that you cannot have the issues page etc enabled when on a forked repo. So it is still a fork, just limitations on GitHub prevent it from being marked as such. I’ve tried to reference back as much as possible in the readme etc to indicate this.

@Yannic
Copy link

Yannic commented Aug 13, 2019

As Adam said and I mentioned in bazelbuild/rules_proto#16, the two repos have very different goal.

@aaliddell I'm currently working on a (concrete) migration plan for the native Protobuf rules, which involves rewriting the native {cc,java,javalite}_proto_library in Starlark. You mentioned in the original design doc that you'd be interested in merging, would you also be interested in merging just these two or three rules so we can hopefully get to a point where theres just one canonical _proto_library` per language?

@aaliddell
Copy link
Member

aaliddell commented Aug 13, 2019

Regarding merging:
I am interested to hear what others have to say regarding usability etc, as I'm probably viewing things from a wonky perspective. Personally, having thought about it a bit more since that design doc: as a user I would expect the rules_proto repo contains the stable core set of rules to get going with Protobuf in Bazel, without dragging in lots of dependencies (much like rules_cc etc). Unfortunately, this repo is necessarily quite heavy-weight due to supporting so many languages, so ends up pulling in a lot of dependencies that a regular protobuf user may not want (e.g. gRPC). So merging this into rules_proto would probably be detrimental to most rules_proto users, without much foreseeable gain to users that could otherwise import this repo if they need it.

However, from the perspective of maintaining this repo, it's worth considering how we ensure continued support where not just one person is looking after these rules. I set this up on a separate org so that it is its own entity that isn't tied to me and can add maintainers without renaming. In terms of attracting PRs and support: the rulegen code in Go is probably quite a large barrier to entry for a lot of people. Removing it would be realistically be untenable, as it automates something that'd be very tedious manually. However, I have considered moving it to Python to be more accessible to people who know Starlark (and a more familiar language for me).

Regarding {cc,java,javalite}_proto_library:
The aspect implementation is pretty self-contained, so could be used in other repos pretty easily and this repo could then depend on the canonical version where available. We'd then need to sync bug fixes to the aspect, but I'd hope they're not too frequent 🙂. Also, the aspect presently relies on this repo's protoc toolchain, but I hope that it can move to just using the rules_proto toolchain as part of #13?

@Yannic
Copy link

Yannic commented Aug 13, 2019

Personally, I strongly believe that foo_proto_library should be defined in rules_foo so users don't have to pull in lots of dependencies for languages they don't use (except for some core rules like C++ and Java, maybe Python). I understand that there are different views on this :).

From the maintaining perspective, I have written some proto lang rules in the past and it involved a lot of copy-pasting code and fixing bugs in literally thousands of places. Generally I'd say that Protobuf in Bazel is a huge mess. Cleaning this up and making it simpler is actually the second goal of bazelbuild/rules_proto. See bazelbuild/rules_proto#6.

Regarding {cc,java,javalite}_proto_library: That sounds good. Since they're ment to replace the native versions, they need to have the exact same API (in fact, if we encounter things that aren't possbile in Starlark, we'll modify the native versions to match the Starlark behaviour). Breaking changes will occur similar to Bazel's incompatible policy.

Regarding proto_toolchain: That's not a thing yet, but will be defined very soon. The only reason we're telling people to add rules_proto_toolchains() in their WORKSPACE is to make this process easier :).

If you're intersted, I can add you to the draft of the design doc for this. I don't want to make it public just yet to avoid comments on things I haven't gotten to. You can find my email on GitHub.

@aaliddell
Copy link
Member

Personally, I strongly believe that foo_proto_library should be defined in rules_foo so users don't have to pull in lots of dependencies for languages they don't use (except for some core rules like C++ and Java, maybe Python). I understand that there are different views on this :).

I'm somewhat on the fence about this, as it would also require that every rules_lang have Protobuf and rules_proto as a dependency. I think Bazel will be forced to pull this dependency regardless of if the user uses the protobuf rules, as the rules_lang defs.bzl must load the proto_library or ProtoInfo for its lang_proto_library rule.

On the other hand, I agree it would be good for rules_lang specialists in their language to support their lang_proto_library, rather than myself and others trying to support langauges we barely know. But thus far it seems to be a bit hit and miss which languages provide support, and with varying implementations when they do.

From the maintaining perspective, I have written some proto lang rules in the past and it involved a lot of copy-pasting code and fixing bugs in literally thousands of places. Generally I'd say that Protobuf in Bazel is a huge mess.

Definitely agree! The rulegen somewhat solves this repetitive work thankfully.

Perhaps the aspect should live only in rules_proto and this repo just provide wrappers for various languages that don't yet provide it in rules_lang. Also, this repo provides the gRPC rules that mostly don't exist elsewhere and I don't think should be in rules_lang, but arguably could be the gRPC repo instead (and some are).

If you're intersted, I can add you to the draft of the design doc for this. I don't want to make it public just yet to avoid comments on things I haven't gotten to. You can find my email on GitHub.

Sure. I'll send you an email.

@qzmfranklin
Copy link
Contributor Author

qzmfranklin commented Aug 21, 2019

After reading the code in //:aspect.bzl, I believe that this is the right way of doing proto and grpc rules.

I was only thinking of doing it. But you actually did it. Good job and thanks!

@aaliddell aaliddell mentioned this issue Oct 18, 2019
29 tasks
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

No branches or pull requests

3 participants