bazel: Imitate native rule behavior with java_grpc_library
#5237
Conversation
Using an aspect allows the implementation to more closely match the user interface and behavior of the 'java_proto_library' native rule. Also noteworthy is this implementation reconfigures itself for protobuf-lite generated messages+runtime and okhttp gRPC transport when consumed by an android_* rule.
|
Soo... I actually had started some work to convert java_grpc_library over to an aspect. I've generally done that work internally in Google and then exported it, because getting it working in Google takes some work (there's lots of users doing weird things, I have to fix everyone, and Blaze has added "gotchas" compared to Bazel). For example, there's some Android integration in Blaze that is a bit different in Bazel and it adds quite a few constraints to the process (yes, dexing, I'm looking at you). My plan was to create two new toolchains: one for full protobuf and one for lite protobuf. Your changes go further than I had been able to get. I did have a "beautiful" workaround for the dexing issue, by using _toolchain (don't even try to get that working yourself; you have to see an example to have any hope of seeing how it plugs together). I'm trying to figure out how to move forward. I think I could push forward my internal work a bit, so that I can then export it. In general, we'll also need to care about providing a clean migration strategy, which I think should be pretty easy. @ceason, do you mind giving me a week to make progress on what I have and at least open a PR of it so you can see it? I'll then need to figure out what pieces of what you've done here should be merged in; there's a lot of changes. |
|
For sure--whatever gets us a gRPC drop-in replacement for I'd like to brain dump the nuances I ran across to a fellow gRPC-proto-aspect implementer, but I'm on east coast time so that will have to wait until tmw. The main one though, was the Android dexer aspect only seeing the native proto library aspect provider, and not the Starlark |
|
Okay. My changes internally haven't been committed, but I'm exporting what I had anyway. This included some things which hadn't made it back in OSS because I've been busy. https://github.com/ejona86/grpc-java/tree/bazel-dex Consider that a "code dump" for the moment. The general organization isn't pretty. But you should be able to find the important parts that you'd be interested in. It does come with build tests! Once the pieces get merged internally, I can turn it into a "real" PR. But you can still see the magic for dexing now. There aren't any aspects in what I'm providing here. I used make_non_strict for the moment to avoid strict_deps. I remember poking at JavaProtoAspect and JavaLiteProtoAspect to try to use them to get dependencies, and I remember I had concern multiple times that things weren't available to skylark... Anyway, this was July when I was working on it last. |
|
I took a look at your code and it looked very familiar--I was definietly in the same place at one point on my java-grpc-proto journey. Here's how that journey led to the code in this PR. I had previously used Eventually I decided to take the plunge and try implementing it myself. My success critera were:
I started with an aspect that generated src and java_common.compile'd it at every transitive proto_library. Then I hooked up that "protocompiler aspect" to a rule which java_common.merge'd the resultant JavaInfos together. This worked great for me and I thought about PRing it back to java-grpc--but my implementation only worked for normal proto/gRPC and I figured "only works for some stuff" doesn't make for a very compelling PR. I took a look at "the other stuff" (ie proto/gRPC lite) and learned quite a bit, being unfamiliar with both proto lite and Android development:
..And off I went to configure my toolchain with
So off I went again:
That about brings us up to date. ..And is also more content than I'd anticipated (sorry about the wall of text) |
|
(When I'm talking about "we" in history, that includes the main author of the LANG_proto_library in Bazel; he and I worked on the design for LANG_grpc_library. We did it in Java first, but the rule appearance applied to all languages.) We don't want java_grpc_library to transitively generate service code. The reason java_proto_library does that is because they are actual dependencies and wiring them together gets tricky and is useless because they must all be java_proto_libraries. But java_grpc_library has no need and it would just add bloat (since due to strict_deps you would still have to define a java_grpc_library rule for the other services). So there should not be a requirement to use aspects. But aspects may be useful to handle things like strict_deps and consuming aspects may be helpful to avoid users providing configuration. The "only one" restriction in java_grpc_library is fake. The code would be happy to handle more. But it was put in place because you really shouldn't be using more than one per proto_library. The "same package" restriction is also fake. It was added because all the rules proto rules should generally be together. But there are times that doesn't work, so... it's never worked well, although it has done its job. The need to specify "srcs" and "deps" is still a bit contentious. It was done, honestly, to be less confusing (as in "less magical"; more concrete). If you happen to forget a java_proto_library for the messages you'd get weird results. And we didn't want java_grpc_library to implicitly generate java_proto_library. And not all languages have LANG_proto_library and others are in the process of migrating. I'd like to set this specific change off to the side as a "revisit"; it is a cross-language decision and it'd need some effort to change (because we were fully aware of the option when we did the initial design, might we just change decisions again in the future?). We want separate rules for full vs lite. This was on purpose, as is the case for java_{lite_,}proto_library. I will mention that at the time, java_proto_library had a "flavor" argument which we copied. I expect at some point to split java_grpc_library in two but it's been low on my list. So I guess we need to start at "what are we wanting to accomplish?" |
The current java_proto_library rules have a great UX--I put proto_library in deps, then plug that into the deps of my {jvmLanguage}_{library,binary}. It JustWorks. Easy UX is what I want to accomplish for the gRPC rules. As a new bazel/proto/etc user, I realize that my java_proto_library generated my messages, but not my service definitions. I find java_grpc_library and I think "great, that looks like it should do gRPC too". But what I find is that although the name is misleadingly similar, the usage semantics are entirely different:
Truth be told, I'm already personally sold on bazel, protobuf/gRPC and the synthesis they have--rough edges and all. I am trying to make converts out of the rest of my workplace, though, and the rough edges make that rather more difficult.
That seems reasonable, and can be accomplished by having the aspect 1. expose the gRPC srcs in the aspect provider instead of compiling them (only compile the proto srcs), 2. also don't propagate transitive gRPC srcs forward (only propagate proto JavaInfos), 3. have the aspected
I can live with that. It even makes some semantic sense as you can't split a service declaration across files.
It's tough when your org is just adopting bazel/protobuf and authors are trying to share protos/gRPC services across polyglot consumers, but the org as a whole hasn't mastered TheOneTrueWay of how to best expose each language's proto artifact. Reducing the dependency surface area (of build rules, by not including lang rules in proto rules' packages) seems like the safest way to avoid consumers having a transitive conflict, and aspect-powered proto library rules go a long, long way to make that possible. Still, I can't complain too much as the current impl is warning rather than failure/enforcement.
I agree and think the ecosystem as a whole benefits from consistent semantics across languages. Personally I think having LANG_proto depend only on proto is more consistent with how bazel rules work overall--each rule depends on the things it depends on. But with LANG_proto via srcs/deps, it's like each successive rule needs to depend not only on the immediate proto, but also that immediate proto's transitive proto's LANG_proto artifact; it feels like superfluous cognitive overhead. In fact I was surprised Go's proto rules work this way, but since gazelle does such an excellent job of generating all of my Go/proto build definitions, I don't mind in that case.
I completely agree; as my understanding of 'lite' has expanded and I see that it actually comes with a different programmer-facing API it definitely feels like it should be a separately named rule rather than a setting on a single rule. Upon reflection I realize this PR's impl of WDYT? Do those concerns/considerations/motivations make sense? I think I can still accomplish everything I'm after with a PR that:
|
Completely agree. I mentioned before that this is an engineering tradeoff: it has advantages and disadvantages. It's also something that can be changed, assuming we get enough agreement and show why we won't just flip/flop the decision again in the future. Also, I'll mention that there are potential practical issues here. The other language devs aren't as skilled in Skylark and some languages like C++ don't even have their own LANG_proto_library rule yet. There's also a manpower issue of where we will find the time for this cross-language work.
But let's say your proto_library (PL1) depends on another proto_library (PL2). To use a message from PL2, you must create a separate java_proto_library. So java_proto_library will generate dependencies, but not let you directly use them. For gRPC, we don't want to
This is true for all users. Sorry, but we don't want to bind the transport dependency with the grpc codegen dependency, just for the sake of convenience. Libraries, for example, commonly have no need to depend on a particular transport. In addition, most tests don't need to depend on either netty nor okhttp; they just need inprocess. It's really only once you start integrating pieces into a final application that the transport is truly necessary. I don't see myself accepting a change that would add a fake transport dependency.
I may be misunderstanding this section. But I wasn't meaning from an implementation standpoint. I was meaning from the user's standpoint. From the UI. Even though with a macro we could easily add a java_proto_library() and then depend on it, that seemed far too magical and made the rules too tightly coupled. We were also worried about future impacts it could have on java_proto_library().
Well, it was meant to encourage users to do it that right way; the warning itself tells you that you aren't doing TheOneTrueWay. So in some ways, that's exactly what it is supposed to do. But there are some cases (with Bazel, multi-repo would be a clear one) that it simply doesn't work to require the java_grpc_library to be along with the proto_library. Thus the crux. This may very well be on the chopping block to remove, although I'd need to have a better list of the issues. (We could just "fix" the multi-repo case by detecting that case and avoiding the warning, for instance.)
So actually it didn't have as much to do with conflicts. It was more about having the java_grpc_library()s in predictable locations and avoiding every user of a proto from re-defining the java_grpc_library() and friends.
I'm generally fine with this and was the direction I was already going. However, I will need to get buy-in internally to rewrite all the users of these in Google. I think I can do that. We will also want to provide users with a migration path (add java_lite_grpc_library and keep flavor around for a bit to give people time to migrate).
I'm a bit confused by this, given this is the current state of the rule.
I'd be a bit concerned as to how you might implement that. How would you detect this case?
It's still a bit unclear what's being proposed here. Would this PR you mention be based on this PR or on my bazel-dex branch? The main deficiency of the bazel-dex branch that I'm aware of is that it defeats strict_deps (via make_non_strict). Using aspects (either our own or the Proto aspects) would seem a solution. I seem to remember that I found a way to just use the Proto aspects, which would seem advantageous. There's also a bit too much involved with running protoc with the dependencies and such. I wanted to use proto.transitive_descriptor_sets/--descriptor_set_in to avoid that, but ran into some problems internally that have to be resolved. (This is a very small PR; basically two lines of code change) |
Aside from the improved ergonomics of only having to specify
Why not export the messages when appropriate?
The "lazy user" problem is real. Could that be solved via
Isn't that a problem anyway? Depending directly on PL2 via a java_proto_library (vs transitively via PL1's
The transport wouldn't actually be bound to codegen, nor to compilation of the generated code, but rather to the returned provider after the generated code had been compiled (is your concern superfluous build actions?). The transport (as a runtime dep) also wouldn't affect how dependee libraries are compiled. I definitely don't want this just for the sake of convenience. I want it so that
The same could be said of any Admittedly, the correct gRPC transport runtime dependency depends on the consuming rule. But fortuitously, the discriminating criteria for "what is the right gRPC transport" lines up with bazel's current (albeit limited and hardcoded) intra-build cross-platform support. This type of support looks like it will be expanded in the future (the Bazel "platforms" roadmap talks about a generic Starlark way to do this stuff).
I don't know what to say about magical..it never seemed that way to me. I'm just proposing the UI be identical to Is it the aspect that seems magical? The aspects documentation lists LANG_proto_library as one of its primary use cases. I've been on the "that seems too magical" side of the table before and a link to some solid documentation is often the missing link in the chain.
One way would be to put a wrapper around the grpc-src generating action (ie the protoc --grpc-plugin_out command) which asserts the presence of a |
|
I've been thinking through how to move this PR forward along a path that gets it merged and I hope that path exists; I'm afraid the changes I want, below, are counter to the (ostensibe?) design goals of
Which of those are non-starters? Which are negotiable with what level of effort/concession? You've given counterpoints to each of the changes I want; in isolation each explanation makes sense, but collectively they point
it gives me pause. Am I approaching the problem (of |
I think those are non-starters. It is on-the-table that we could "remove the srcs list". But we'd still maintain the deps to java_proto_library.
As it stands, there's been some complaints about LANG_proto_library being too arduous. Previously we had a single proto_library rule that generated code for all languages. Now each language needs its own target. And you need such targets for service generation as well. I think Google feels this pain worse than most, because of the monorepo and the scale many protos are used. There's been brief talk that it'd be easy to make a macro that would create all the LANG_proto_libraries and LANG_grpc_libraries. But push hasn't come to shove. The status quo honestly isn't that bad, so it's unclear to me where things will turn out long-term. So I'm just waiting and seeing. If it bothers you a lot, making a macro that defines the proto_library, java_proto_library, and java_grpc_library all-at-once would be easy.
We don't want it exported, as then users would get the messages from the grpc rule. While I'd agree that is convenient, it is something we're strongly against, as we went "down that road" already in the past and it was horrible. Strict_deps is our friend and while, yes, it adds more boilerplate, it is a very good friend and well-worth the costs. We want users of the messages to depend on the messages.
That's a complicated solution for a simple problem. It still also removes the fact that users wouldn't depend on the messages. It is very common to search for "users of target X". That is a bit more complicated with aspects, but I think still possible. We want the users of the messages to depend directly on the messages so we can find them easily.
No. PL1 may be using an option from PL2, for example (or for a "private" message, or the proto was split into two files, etc.). If PL1 removes that option, the dependency on PL2 goes away. That itself is good. But it is prohibitive if you have to add a PL1 dependency to some of PL2's reverse deps.
That's not true. If on Android, you may be using OkHttp or Cronet transports. If on a server, you generally are using Netty, but some users are using OkHttp for its small size. And in both cases you may only need InProcess. I don't have a problem with adding runtime_deps. But it should only be done for true dependencies. I don't think we are in a position to make the choice for our users.
Okay, I didn't realize that. That's a "no-no." We don't want multiple targets generating the same Java classes. The java_proto_library targets go out of their way to avoid that, via aspects. There should only be one target that generates any particular Java class. So to do this "right" would require creating an implicit java_proto_library within java_grpc_library. And that was what was deemed "magical." Aspects do have a sense of magic about them, but most people probably can live life without looking behind the curtain. |
They are generally authored manually, to my knowledge, except for a class of Go users. While it is true that many of the references I see were automatically generated, that was due to migration tools. As I did mention, there has been some discussions over how verbose all the LANG_proto_library stuff is and the repetition of srcs/deps for LANG_grpc_library. So it has been discussed to remove 'srcs' and it has been discussed (less seriously) to have a macro that generates the various pieces. But people are still writing this stuff manually today, and honestly, I'm not bothered too much by it. (These tend to be the simplest rules/targets in a repo; they don't take much time/effort compared to all the other BUILD things we deal with.) |
|
What would you think about a pair of rules ( proto_library(
name = "helloworld_proto",
srcs = ["helloworld.proto"],
)
java_[lite_]proto_library(
name = "helloworld_java_proto",
deps = [":helloworld_proto"],
)
java_[lite_]grpc_library(
name = "helloworld_java_grpc",
deps = [":helloworld_java_proto"],
# (optional) runtime deps to allow user-defined configuration of transport
runtime_deps = [],
)
java_binary(
name = "server",
runtime_deps = [
"@io_grpc_grpc_java//netty",
],
deps = [
":helloworld_java_grpc",
":helloworld_java_proto",
],
...
)
android_binary(
name = "client",
runtime_deps = [
"@io_grpc_grpc_java//okhttp",
],
deps = [
":helloworld_javalite_grpc",
":helloworld_javalite_proto",
],
...
)That would address my main pain points, and hopefully checks your boxes as well? |
|
No to adding If you really want a |
|
Fair enough. So without runtime_deps the rest looks good though? |
|
Yes, the rest looks good to me. But understand:
These rules have been implemented cross-language and have been in use for 4 years. So there is a momentum behind them that must be overcome. |
|
I created tracking issues for the two changes mentioned at the end here: #5348 and #5349. I'm still moving forward with my changes from the bazel-dex branch, but hit some more delays waiting for other teams to make fixes and release them. While we may reuse parts of this PR, it seems it would be reworked pieces, so closing for now. |
This PR reimplements
java_grpc_libraryusing an aspect. Using an aspect allows the implementation to more closely match the user interface and behavior of thejava_proto_librarynative rule. (ie the new impl can directly consume any number of proto_library targets in itsdeps, and no longer depends onjava_{lite_,}proto_library)Also noteworthy is this implementation reconfigures itself to protobuf-lite
generated messages+runtime and okhttp gRPC transport when consumed by an
android_* rule.
Aside from more closely matching the native java_proto_* rules, I think this implementation is more end-user friendly--no need to coordinate between multiple
java_{lite_,}{proto,grpc}_libraryrules' srcs and deps attrs, just plug your desired proto_libraries into thedepsattr and go!