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

java_grpc_library: Add support for protobuf lite #4289

Merged
merged 2 commits into from Apr 3, 2018

Conversation

ejona86
Copy link
Member

@ejona86 ejona86 commented Mar 31, 2018

gRPC's protobuf-lite auto-selects between full and lite protobuf based on the
value of crosstool_top. If the user is specifying their own
--android_crosstool_top, then it will not auto-detect correctly. One day,
platforms will fix problems like this, but for the moment it seems we get to
live with it.

This includes #4288 as the first commit.

Note: I would have added a BUILD for the helloworld Android app, but ran
into unrelated problems ("cannot access TaskStackBuilder").

@ejona86
Copy link
Member Author

ejona86 commented Mar 31, 2018

CC @codesuki

@codesuki
Copy link

Thanks! That looks good. You didn't even need to add a new rule. I will give it a spin when I am back from vacation.
Sorry I couldn't help after all.

@codesuki
Copy link

I am on mobile and didn't dig any deeper, let me ask a stupid question. Why does the 'protobuf-lite' rule even need the full protobuf in the default case.

"//conditions:default": ["@com_google_protobuf//:protobuf_java"],

I am not saying it's not correct just thinking it's unintuitive :)

@ejona86
Copy link
Member Author

ejona86 commented Apr 2, 2018

@codesuki, so... Lite is a subset of full proto. Or rather, it was a subset before the fork. The fork should be temporary, but it will be with us a while longer.

grcp-protobuf-lite uses only the subset of full proto supported by lite. But it can work with both full and lite protobufs. grpc-protobuf actually depends on grpc-protobuf-lite. However, since Lite is a fork now, it's dangerous to depend on both full and lite protobuf as you'll get conflicts in the classpath. Thus we have to select which version we want.

@ejona86
Copy link
Member Author

ejona86 commented Apr 2, 2018

Oh, and since it is dangerous to mix lite and full protobuf, I am sort of imposing the solution we have done internally: use lite if and only if on Android.


# This config is not fully-reliable. If it breaks, it is probably because you
# are changing --android_crosstool_top. Instead of doing that, you can bind
# your own toolchain on top of the default android/closstool, as mentioned at
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/closstool/crosstool

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@ericgribkoff ericgribkoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Coverage for this on Kokoro is blocked by getting the Android examples building with bazel, right? I manually built protobuf-lite with --crosstool_top=//external:android/crosstool which I think demonstrates that this works - is this or another similar command appropriate to add to the bazel build script as a measure?

@ejona86
Copy link
Member Author

ejona86 commented Apr 3, 2018

Coverage is blocked by getting a android_library in the build, yes.

I manually built protobuf-lite with --crosstool_top=//external:android/crosstool which I think demonstrates that this works

It shows that grpc-protobuf-lite can be built against protobuf-lite (which we already do with gradle). It doesn't show the rest of the stuff works. And it doesn't show that the select() is right. And such an argument command could break other targets. So such a test seems pretty limited use at the moment. I'd much rather introduce an android_library of some sort.

gRPC's protobuf-lite auto-selects between full and lite protobuf based on the
value of crosstool_top. If the user is specifying their own
--android_crosstool_top, then it will not auto-detect correctly. One day,
platforms will fix problems like this, but for the moment it seems we get to
live with it.
@ejona86 ejona86 merged commit bace06f into grpc:master Apr 3, 2018
@ejona86 ejona86 deleted the javalite branch April 3, 2018 22:22
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants