Add multi-release jars to enable Java Modules#8767
Conversation
|
|
|
Is this post 5.0? |
# Conflicts: # okhttp/build.gradle.kts
# Conflicts: # buildSrc/build.gradle.kts # okhttp/build.gradle.kts
# Conflicts: # gradle/libs.versions.toml
|
@swankjesse appropriate for 5.2? |
swankjesse
left a comment
There was a problem hiding this comment.
The code looks fine but I don’t much appreciate what it buys us or our users?
| @@ -0,0 +1,6 @@ | |||
| module okhttp3.modules { | |||
| requires okhttp3; | |||
| requires okhttp3.logging; | |||
There was a problem hiding this comment.
what the heck is okhttp3.logging ?!
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
Or maybe favoured Stephen's comment over yours?
Your module name should be the same as the package name in that jar file, unless you have a good reason otherwise. ie. you as a human are deciding that one package is the base, and assigning that as the module name.
There was a problem hiding this comment.
Yep yep, it’s the package name. I missed that we gave that module two different names.
| requires jdk.crypto.ec; | ||
| requires org.junit.jupiter.api; | ||
| requires okhttp3.modules; | ||
| opens okhttp3.modules.test to org.junit.platform.commons; |
There was a problem hiding this comment.
Do any developers do stuff like this in the wild?
There was a problem hiding this comment.
| exports okhttp3; | ||
| exports okhttp3.internal to okhttp3.logging, okhttp3.sse, okhttp3.java.net.cookiejar, okhttp3.dnsoverhttps, mockwebserver3, okhttp3.mockwebserver, okhttp3.coroutines, okhttp3.tls; | ||
| exports okhttp3.internal.platform to okhttp3.logging, okhttp3.java.net.cookiejar, okhttp3.dnsoverhttps, mockwebserver3, okhttp3.mockwebserver, okhttp3.tls; | ||
| exports okhttp3.internal.http to okhttp3.logging, okhttp3.brotli, mockwebserver3; |
There was a problem hiding this comment.
I’m still curious about what okhttp3.logging is
There was a problem hiding this comment.
A mistake from 2018...
| error: package okhttp3.internal.platform is not visible | ||
| okhttp3.internal.platform.Platform.get(); | ||
| ^ | ||
| (package okhttp3.internal.platform is declared in module okhttp3, | ||
| which does not export it to module i.am.bad.and.i.should.feel.bad) |
There was a problem hiding this comment.
@swankjesse I guess this is the main point?
There was a problem hiding this comment.
Also added a CI test that jlink works, see https://www.baeldung.com/jlink
There was a problem hiding this comment.
There was a problem hiding this comment.
@swankjesse do you need more convincing?
There was a problem hiding this comment.
This is good behavior. I wouldn’t use i.am.bad.and.i.should.feel.bad as the example; this is not a place for shame
|
@swankjesse requesting post review. Seems like a net good to support something some users are adopting. Happy to revert if you are completely against it. |
| error: package okhttp3.internal.platform is not visible | ||
| okhttp3.internal.platform.Platform.get(); | ||
| ^ | ||
| (package okhttp3.internal.platform is declared in module okhttp3, | ||
| which does not export it to module i.am.bad.and.i.should.feel.bad) |
There was a problem hiding this comment.
This is good behavior. I wouldn’t use i.am.bad.and.i.should.feel.bad as the example; this is not a place for shame
| // not needed when compiling with recent JDKs, e.g. 17 | ||
| options.compilerArgs.add("-Xlint:-requires-transitive-automatic") | ||
|
|
||
| // Patch the compileKotlinJvm output classes into the compilation so exporting packages works correctly. |
| @@ -0,0 +1,6 @@ | |||
| module okhttp3.modules { | |||
| requires okhttp3; | |||
| requires okhttp3.logging; | |||
There was a problem hiding this comment.
Yep yep, it’s the package name. I missed that we gave that module two different names.
| // Note that module checking only works on JDK 9+, | ||
| // because the JDK built-in base modules are not available in earlier versions. | ||
| val javaVersion = compileKotlinTask.kotlinJavaToolchain.javaVersion.getOrNull() | ||
| when { |
There was a problem hiding this comment.
I don’t really understand what this when{} is supposed to be doing
| } | ||
|
|
||
| // From https://github.com/Kotlin/kotlinx-atomicfu/blob/master/atomicfu/build.gradle.kts | ||
| val compileJavaModuleInfo by tasks.registering(JavaCompile::class) { |
There was a problem hiding this comment.
Please refactor this so it’s the same applyJavaModules() call we have in all of our other modules? Having two copy-pastes of this configuration will confuse me for all time
There was a problem hiding this comment.
(You could definitely have a parameter to toggle the options that only the okhttp module needs? Why doesn’t this module use me.champeau.mrjar?!)
| Java Modules | ||
| ------------ | ||
|
|
||
| OkHttp (5.3+) implements Java 9 Modules. |
There was a problem hiding this comment.
What the heck is OkHttp 5.3 ?! We have to ship 5.2 first!

Attempting to apply multi-release Jars and use it to add
java9/module-info.javafiles.This should run a HTTP request against square.com using a jlink image.
fixes #4184
Todo