-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
android-interop-testing: include android interop testing in main build #6829
android-interop-testing: include android interop testing in main build #6829
Conversation
…ode minification due to proguard issue.
a6b2739
to
c9956a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not fully reviewed, sending what i have now
i ran ./gradlew :grpc-android-interop-testing:{test,checkStyleMain}
those tasks don't work.
You should be able to run |
yes, it turns out that it was my local environmental issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@voidzcy My local run of
|
Did you pull all commits in this PR? That's caused by ProGuard and I should have fixed it in commit 61a3d75 |
Yes, I pulled the PR up to f7c160c . However, I tested build in master and it failed similarly. I figured out the actual failure is
I think it is because I was using jdk 11. I believe it's not a problem from your change. |
Yes yes. You need to recompile everything with jdk 8. |
I'm a little confused about the intended support for JDK versions here. Currently I can build grpc-java at master with JDK 11. With this PR, I need to use JDK 8 (JDK 11 fails) and it sounds like this is intentional. I'm not sure why this would be a desired change, but if it is, should this be explained and documented somewhere? |
Java 9 made an ABI incompatible change in
|
I suspect this is due to Android. You cannot use JDK 11 to build some part of the source (i.e., Do you know if there is any good solution to mitigate this? |
My concern was just that a user who tries to build the grpc-java project with JDK 11 will start getting a build error, the cause of which is not very obvious (at least to me, I didn't immediately think JDK incompatibility). Building with |
Alright. This is actually an issue when ProGuard is trying to remove unused code, not really bytecode incompatibility (we have |
…ile against JDK 9+.
Edited ProGuard rules, build ( |
Good point. I'm trying to fix this in general in #6839
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…hen compile against JDK 9+." This reverts commit 46b407f.
…roid_interop_testing_into_main_build
For #6775.
TODO: temporarily disabled code shrinking for release buildType, need to fix proguard rules to before being able to do code shrinking.The problem was caused by pulling in
org.apache.httpcomponents.htttpcore
by google auth2 lib. According to Proguard:TODO: there are tons of errorprone warnings and Proguard notes. Although they do not cause build fail, it would be better to fix/suppress them.Proguard notes seem to be fine. There are some compiler warnings coming from javalite generated code, nothing can be done to fix that. A couple of errorprone warnings left, but unclear if it is necessary to fix them.
After the change, generated code will be checked in. So this PR contains generated code. Reviewers should only need to review
.gradle
,.md
files,proguard-rules.pro
,InteropTask.java
andTesterActivity.java
.