introduced RoboVM platform. Goal to allow use AndroidSocketAdapter with RoboVM

[dkimitsa]

RoboVM is android4 based runtime and it shares amount of classes from Android4.
Its important to check for RoboVM platform before Android as RoboVM gets classified as Android and then okhttp will crash with java.lang.NoClassDefFoundError: android/os/Build$VERSION.
RoboVMPlatform class is based on Android one. Unsupported entities were removed.

[swankjesse]

I don’t like the PR description... “support” is an overstatement, we just aren’t breaking RoboVM in ways we currently know about. If you’d like to contribute configuration to regression test on CI, that’d be rad.

[yschimke]

I don’t like the PR description... “support” is an overstatement, we just aren’t breaking RoboVM in ways we currently know about. If you’d like to contribute configuration to regression test on CI, that’d be rad.

This was also something I was looking into, we don't intend to break RoboVM, is just isn't a normal platform that we commit to supporting. A regression test on CI is great if we intend to actively support it.

How do RoboVM users choose an OkHttp version. How does the cipher workarounds you've posted about get applied? Does the post suggest maybe you should be providing a module that provides OkHttp 3.12.12 and Google Conscrypt to get back a secure well maintained set of Ciphers?

https://dkimitsa.github.io/2020/06/16/broken-ssl-handshake-okhttp4/

[yschimke]

Its important to check for RoboVM platform before Android as RoboVM gets classified as Android

I don't think this is the case since Android and RoboVM both do similar checks "Dalvik" vs "RoboVM". Both can't be true.

[yschimke]

RoboVM is android4 based runtime and it shares amount of classes from Android4

Why master then? Should you be pinning to OkHttp 3.12.x and get it working there? Should we attempt to support RoboVM with OkHttp 4 since we assume Android 5+ for OkHttp 4.x

[dkimitsa]

thank you for quick review, please find responses inlined in comment bellow:

@yschimke

I don’t like the PR description... “support” is an overstatement, we just aren’t breaking RoboVM in ways we currently know about. If you’d like to contribute configuration to regression test on CI, that’d be rad.

Would CI test similar to ones done in square/retrofit#3424 work here as well (can prepare and push)?

How do RoboVM users choose an OkHttp version. How does the cipher workarounds you've posted about get applied? Does the post suggest maybe you should be providing a module that provides OkHttp 3.12.12 and Google Conscrypt to get back a secure well maintained set of Ciphers?

https://dkimitsa.github.io/2020/06/16/broken-ssl-handshake-okhttp4/

If users use OkHttp directly in code there are plenty options for versions and workarounds. But its real problem when OkHttp is used by third party library where user have no control. Thats the goal to have it compatible without workarounds. Once changes described in post applied there will be no need to have cipher workaround.

I don't think this is the case since Android and RoboVM both do similar checks "Dalvik" vs "RoboVM". Both can't be true.

Sorry my bad, the statement was based on what I saw in okhttp 4.4, in 4.8 the order doesn't matter.

Why master then? Should you be pinning to OkHttp 3.12.x and get it working there? Should we attempt to support RoboVM with OkHttp 4 since we assume Android 5+ for OkHttp 4.x

Its about what user uses directly or through third party dependencies. Why not have better compatibility if it doesn't cost much ?
Basically 4.8 works out of box with RoboVM but generic Platform got elected which means setUseSessionTickets, setHostname and setAlpnProtocols APIs will not be used and okhttp will be not able to connect in some cases.

[yschimke]

@dkimitsa Thanks, no objection from me to this PR. But I think we should get a test in place that leaves you happy that each incremental release we do is still working. Can be a separate PR if that means we can get a full end to end test with RoboVM for example.

[dkimitsa]

@yschimke thanks, will prepare another PR with test, will use approach similar to RoboVM test integration from square/retrofit#3424

[dkimitsa]

@yschimke
have added WIP #6134 , as okhttp requires modern java to build its not possible to use release version of RoboVM yet (as 2.3.9 can be run only on jdk8 host) . will update PR once release version is out. Meanwhile we can start review/discussion there.
Thanks.

[yschimke]

Looks pretty clean, minimal impact won't even be seen on JDK 9+ or Android.

[yschimke]

@swankjesse @JakeWharton any objection if I merge? I think the 180° alternative would making Platform API public-ish and allowing clients in unsupported environments to control. This seems less worse.

[JakeWharton]

I don't mind. But without any test I have no confidence in this actually working as-is or staying as working in the future. The Retrofit test doesn't exercise any real codepaths so I'm not even convinced that it works there either.

[yschimke]

I've tried running this in the other PR, and I've got a few errors so far, finally stuck on

> Task :robovm-test:robovmTest FAILED
Loading class 'java.util.logging.ConsoleHandler' failed
java.lang.ClassNotFoundException: java.util.logging.ConsoleHandler

Which I'm not real motivated to work around. Not going to land as is.

Other diff required

--- a/okhttp/src/main/kotlin/okhttp3/internal/platform/Platform.kt
+++ b/okhttp/src/main/kotlin/okhttp3/internal/platform/Platform.kt
@@ -191,22 +191,24 @@ open class Platform {

     private val isConscryptPreferred: Boolean
       get() {
-        val preferredProvider = Security.getProviders()[0].name
+        val preferredProvider = providerName()
         return "Conscrypt" == preferredProvider
       }

     private val isOpenJSSEPreferred: Boolean
       get() {
-        val preferredProvider = Security.getProviders()[0].name
+        val preferredProvider = providerName()
         return "OpenJSSE" == preferredProvider
       }

     private val isBouncyCastlePreferred: Boolean
       get() {
-        val preferredProvider = Security.getProviders()[0].name
+        val preferredProvider = providerName()
         return "BC" == preferredProvider
       }

+    private fun providerName() = Security.getProviders().firstOrNull()?.name
+

[yschimke]

@dkimitsa Is this and the other PR still something you are keen on? I think we'd need a github actions build showing this working. Not sure who is doing that, who can and is spending the time.

[dkimitsa]

@yschimke probably actions similar from retrofit project can be used here, sadly have no experience setting it up. Following pr shows changes were done to retrofit square/retrofit#3424

[yschimke]

@swankjesse @JakeWharton I'd be ok landing this and asking RoboVM to setup a test in their CI that shows it working with our latest release. It's probably better than what we currently have.

I suspect I might be in the minority here. The CI tests PR wasn't going anywhere #6134

We could start with a 4.10-RC2 and see it work downstream before landing?

[yschimke]

Discussed offline, too much work to support easily here without someone setting up and maintaining the tests.

[dkimitsa]

@yschimke why not use GitHub actions for test as its done for similar case in square/retrofit#3424
Confusing moment here is only robovm related test to be done these while primary ones on Circle CI.
Also I would be appreciated for info of setting test externally at robovm side. If particular the process of integration with okhttp CI.
Thank you

[yschimke]

Should we take a look again after a future migration to GitHub CI. From other PR we were blocked in circle ci without paid account and no-one is investing time on migration yet.