Skip to content

Add multi-release jars to enable Java Modules#8767

Merged
yschimke merged 37 commits into
lysine-dev:masterfrom
yschimke:mrjar
Sep 21, 2025
Merged

Add multi-release jars to enable Java Modules#8767
yschimke merged 37 commits into
lysine-dev:masterfrom
yschimke:mrjar

Conversation

@yschimke

@yschimke yschimke commented May 11, 2025

Copy link
Copy Markdown
Collaborator

Attempting to apply multi-release Jars and use it to add java9/module-info.java files.

This should run a HTTP request against square.com using a jlink image.

./gradle module-tests:imageRun

fixes #4184

Todo

  • Enable OkHttp modules in the test to flush out errors
  • Setup a CI job probably testing against Mockserver

@yschimke
yschimke requested a review from JakeWharton May 11, 2025 14:53
@yschimke yschimke added android Relates to usage specifically on Android providers Non core security providers jdkversions JDK 8, 17, 19 etc. labels May 11, 2025
@yschimke
yschimke marked this pull request as ready for review May 11, 2025 15:53
@yschimke

yschimke commented May 11, 2025

Copy link
Copy Markdown
Collaborator Author

Current status! resolved

Exception in thread "main" java.lang.IllegalAccessError: class okhttp3.internal.concurrent.TaskRunner (in module okhttp3) cannot access class java.util.logging.Logger (in module java.logging) because module okhttp3 does not read module java.logging
	at okhttp3/okhttp3.internal.concurrent.TaskRunner.<clinit>(TaskRunner.kt:362)

@yschimke
yschimke requested a review from swankjesse May 26, 2025 10:56
Comment thread buildSrc/build.gradle.kts Outdated
@yschimke

Copy link
Copy Markdown
Collaborator Author

Is this post 5.0?

yschimke added 4 commits June 21, 2025 11:03
# Conflicts:
#	okhttp/build.gradle.kts
# Conflicts:
#	buildSrc/build.gradle.kts
#	okhttp/build.gradle.kts
yschimke added 2 commits July 12, 2025 16:30
# Conflicts:
#	gradle/libs.versions.toml
@yschimke

yschimke commented Jul 12, 2025

Copy link
Copy Markdown
Collaborator Author

@swankjesse appropriate for 5.2?

@swankjesse swankjesse left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The code looks fine but I don’t much appreciate what it buys us or our users?

Comment thread android-test/build.gradle.kts
Comment thread buildSrc/src/main/kotlin/JavaModules.kt
Comment thread buildSrc/src/main/kotlin/JavaModules.kt
Comment thread mockwebserver-deprecated/src/main/java9/module-info.java
Comment thread mockwebserver-junit4/build.gradle.kts Outdated
Comment thread module-tests/build.gradle.kts
@@ -0,0 +1,6 @@
module okhttp3.modules {
requires okhttp3;
requires okhttp3.logging;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what the heck is okhttp3.logging ?!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

#3743 (comment)

I think I misread your comment.

image

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yep yep, it’s the package name. I missed that we gave that module two different names.

Comment thread module-tests/src/main/java/okhttp3/modules/OkHttpCaller.java Outdated
requires jdk.crypto.ec;
requires org.junit.jupiter.api;
requires okhttp3.modules;
opens okhttp3.modules.test to org.junit.platform.commons;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do any developers do stuff like this in the wild?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I’m still curious about what okhttp3.logging is

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

A mistake from 2018...

Comment thread README.md
Comment on lines +224 to +228
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)

@yschimke yschimke Jul 19, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@swankjesse I guess this is the main point?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Also added a CI test that jlink works, see https://www.baeldung.com/jlink

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@swankjesse do you need more convincing?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@yschimke yschimke mentioned this pull request Jul 19, 2025
@yschimke

Copy link
Copy Markdown
Collaborator Author

@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.

@yschimke
yschimke merged commit b7290e4 into lysine-dev:master Sep 21, 2025
26 checks passed

@swankjesse swankjesse left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

Comment thread README.md
Comment on lines +224 to +228
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Very helpful comment!

@@ -0,0 +1,6 @@
module okhttp3.modules {
requires okhttp3;
requires okhttp3.logging;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yep yep, it’s the package name. I missed that we gave that module two different names.

Comment thread okhttp/build.gradle.kts
// 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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don’t really understand what this when{} is supposed to be doing

Comment thread okhttp/build.gradle.kts
}

// From https://github.com/Kotlin/kotlinx-atomicfu/blob/master/atomicfu/build.gradle.kts
val compileJavaModuleInfo by tasks.registering(JavaCompile::class) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(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?!)

Comment thread README.md
Java Modules
------------

OkHttp (5.3+) implements Java 9 Modules.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What the heck is OkHttp 5.3 ?! We have to ship 5.2 first!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ooops, can fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

android Relates to usage specifically on Android jdkversions JDK 8, 17, 19 etc. providers Non core security providers

Development

Successfully merging this pull request may close these issues.

JPMS support

3 participants