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

okhttp: make okhttp dependencies compile only #8971

Merged
merged 4 commits into from
Mar 30, 2022

Conversation

beatrausch
Copy link
Contributor

@beatrausch beatrausch commented Mar 6, 2022

This PR makes okhttp dependencies compile only. The PR is related to #6119 #8732 and is the base to add okhttp3 in the public API. For the implementation the required okhttp2 files had been forked.

@sanjaypujare sanjaypujare added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 16, 2022
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 16, 2022
@ejona86 ejona86 self-requested a review March 18, 2022 15:51
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I'll need to take a closer look, but at first glance this looks perfect. I am sorry, I had been intending to do the third_party copies, but glad you figured it out. Also sorry that it didn't see review earlier.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Very easy to review. Thank you. Code looks good.

I need to figure out what's wrong with the Android CI. It doesn't seem likely to be a flake.

@ejona86
Copy link
Member

ejona86 commented Mar 21, 2022

Error from Android CI. ./gradlew grpc-android:build -PskipAndroid=false -Pandroid.useAndroidX=true

java.lang.NoClassDefFoundError: com/squareup/okhttp/ConnectionSpec
	at java.lang.Class.getDeclaredMethods0(Native Method)
	at java.lang.Class.privateGetDeclaredMethods(Class.java:2701)
	at java.lang.Class.privateGetMethodRecursive(Class.java:3048)
	at java.lang.Class.getMethod0(Class.java:3018)
	at java.lang.Class.getMethod(Class.java:1784)
	at io.grpc.android.AndroidChannelBuilder.<init>(AndroidChannelBuilder.java:123)
	at io.grpc.android.AndroidChannelBuilder.forTarget(AndroidChannelBuilder.java:77)
	at io.grpc.android.AndroidChannelBuilderTest.channelBuilderClassFoundReflectively(AndroidChannelBuilderTest.java:98)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:575)
	at org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$0(SandboxTestRunner.java:263)
	at org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:89)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.ClassNotFoundException: com.squareup.okhttp.ConnectionSpec
	at org.robolectric.internal.bytecode.SandboxClassLoader.getByteCode(SandboxClassLoader.java:163)
	at org.robolectric.internal.bytecode.SandboxClassLoader.maybeInstrumentClass(SandboxClassLoader.java:129)
	at org.robolectric.internal.bytecode.SandboxClassLoader.lambda$loadClass$0(SandboxClassLoader.java:115)
	at org.robolectric.util.PerfStatsCollector.measure(PerfStatsCollector.java:53)
	at org.robolectric.internal.bytecode.SandboxClassLoader.loadClass(SandboxClassLoader.java:115)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
	... 24 more

Hmm... getMethod() calls getDeclaredMethods0(). That means we can't do any reflection on the class?

We could workaround the problem by adding okhttp as a dependency of grpc-android. We could also use a different class for the reflection; OkHttpChannelProvider would be a good candidate.

Let's avoid this problem in this PR, so we can make forward progress. Let's keep okhttp as an API dependency in this PR. Today I'll try changing the reflection to OkHttpChannelProvider.

ejona86 added a commit to ejona86/grpc-java that referenced this pull request Mar 21, 2022
Doing any reflection on OkHttpChannelBuilder requires that all methods
can have their arguments resolved. We'd like to make okhttp an optional
dependency (to support okhttp 2 and 3/4 simultaneously). But making
okhttp optional means we can no longer construct OkHttpChannelBuilder
reflectively. We swap to the Provider that doesn't have this problem.
See grpc#8971.

Note that ManagedChannelProvider itself only exposes its methods as
protected, so they wouldn't be accessible. However OkHttpChannelProvider
has its methods public. It is an open question of whether
ManagedChannelProvider's methods should become public, but in any case
we can hide a public OkHttpChannelProvider inside a package-private
class so it is only accessable via reflection. So this code assuming
public methods doesn't prevent future implementation hiding.
ejona86 added a commit to ejona86/grpc-java that referenced this pull request Mar 21, 2022
Doing any reflection on OkHttpChannelBuilder requires that all methods
can have their arguments resolved. We'd like to make okhttp an optional
dependency (to support okhttp 2 and 3/4 simultaneously). But making
okhttp optional means we can no longer construct OkHttpChannelBuilder
reflectively. We swap to the Provider that doesn't have this problem.
See grpc#8971.

Note that ManagedChannelProvider itself only exposes its methods as
protected, so they wouldn't be accessible. However OkHttpChannelProvider
has its methods public. It is an open question of whether
ManagedChannelProvider's methods should become public, but in any case
we can hide a public OkHttpChannelProvider inside a package-private
class so it is only accessable via reflection. So this code assuming
public methods doesn't prevent future implementation hiding.
ejona86 added a commit to ejona86/grpc-java that referenced this pull request Mar 21, 2022
Doing any reflection on OkHttpChannelBuilder requires that all methods
can have their arguments resolved. We'd like to make okhttp an optional
dependency (to support okhttp 2 and 3/4 simultaneously). But making
okhttp optional means we can no longer construct OkHttpChannelBuilder
reflectively. We swap to the Provider that doesn't have this problem.
See grpc#8971.

Note that ManagedChannelProvider itself only exposes its methods as
protected, so they wouldn't be accessible. However OkHttpChannelProvider
has its methods public. It is an open question of whether
ManagedChannelProvider's methods should become public, but in any case
we can hide a public OkHttpChannelProvider inside a package-private
class so it is only accessable via reflection. So this code assuming
public methods doesn't prevent future implementation hiding.
ejona86 added a commit to ejona86/grpc-java that referenced this pull request Mar 21, 2022
Doing any reflection on OkHttpChannelBuilder requires that all methods
can have their arguments resolved. We'd like to make okhttp an optional
dependency (to support okhttp 2 and 3/4 simultaneously). But making
okhttp optional means we can no longer construct OkHttpChannelBuilder
reflectively. We swap to the Provider that doesn't have this problem.
See grpc#8971.

Note that ManagedChannelProvider itself only exposes its methods as
protected, so they wouldn't be accessible. However OkHttpChannelProvider
has its methods public. It is an open question of whether
ManagedChannelProvider's methods should become public, but in any case
we can hide a public OkHttpChannelProvider inside a package-private
class so it is only accessable via reflection. So this code assuming
public methods doesn't prevent future implementation hiding.
@ejona86
Copy link
Member

ejona86 commented Mar 21, 2022

CC @YifeiZhuang, @temawi

@beatrausch
Copy link
Contributor Author

@ejona86 I will update the PR to API dependency and just note it in an comment and reference your issue #9003.

@ejona86
Copy link
Member

ejona86 commented Mar 21, 2022

@beatrausch, actually. You can hold-off. We can just make sure my PR goes in first. Changing this PR now would mean changing the PR title and some other stuff. It turned out I made my changes quickly, so this is probably fine as-is.

@beatrausch
Copy link
Contributor Author

@beatrausch, actually. You can hold-off. We can just make sure my PR goes in first. Changing this PR now would mean changing the PR title and some other stuff. It turned out I made my changes quickly, so this is probably fine as-is.

Ok

ejona86 added a commit that referenced this pull request Mar 24, 2022
Doing any reflection on OkHttpChannelBuilder requires that all methods
can have their arguments resolved. We'd like to make okhttp an optional
dependency (to support okhttp 2 and 3/4 simultaneously). But making
okhttp optional means we can no longer construct OkHttpChannelBuilder
reflectively. We swap to the Provider that doesn't have this problem.
See #8971.

Note that ManagedChannelProvider itself only exposes its methods as
protected, so they wouldn't be accessible. However OkHttpChannelProvider
has its methods public. It is an open question of whether
ManagedChannelProvider's methods should become public, but in any case
we can hide a public OkHttpChannelProvider inside a package-private
class so it is only accessable via reflection. So this code assuming
public methods doesn't prevent future implementation hiding.
@ejona86
Copy link
Member

ejona86 commented Mar 24, 2022

#9003 is merged. Re-running the Android CI.

@ejona86
Copy link
Member

ejona86 commented Mar 30, 2022

@YifeiZhuang, could you take a look?

@YifeiZhuang YifeiZhuang merged commit b20ce17 into grpc:master Mar 30, 2022
Copy link
Contributor

@YifeiZhuang YifeiZhuang left a comment

Choose a reason for hiding this comment

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

LGTM

ejona86 added a commit to ejona86/grpc-java that referenced this pull request Mar 31, 2022
…)"

This partly reverts b20ce17 because it is dependent on cda0e9d which is
being reverted as it is incompatible with Proguard without configuration
(e.g., isAvailable() may not be available and would need a -keep).

While the code no longer uses compileOnly, the compatibility still
remains, so it would be possible for users to exclude the okhttp
dependency if they don't use AndroidChannelBuilder.
temawi pushed a commit to temawi/grpc-java that referenced this pull request Apr 8, 2022
Doing any reflection on OkHttpChannelBuilder requires that all methods
can have their arguments resolved. We'd like to make okhttp an optional
dependency (to support okhttp 2 and 3/4 simultaneously). But making
okhttp optional means we can no longer construct OkHttpChannelBuilder
reflectively. We swap to the Provider that doesn't have this problem.
See grpc#8971.

Note that ManagedChannelProvider itself only exposes its methods as
protected, so they wouldn't be accessible. However OkHttpChannelProvider
has its methods public. It is an open question of whether
ManagedChannelProvider's methods should become public, but in any case
we can hide a public OkHttpChannelProvider inside a package-private
class so it is only accessable via reflection. So this code assuming
public methods doesn't prevent future implementation hiding.
temawi pushed a commit to temawi/grpc-java that referenced this pull request Apr 8, 2022
* okhttp: forked required files to make okhttp dep compile only

* okhttp: forked missing file to make okhttp dep compile only

* okhttp: moved url and request files to proxy packge

* okhttp: removed unused methods from forked files; fixed build
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 29, 2022
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.

5 participants