search

square / okio

A modern I/O library for Android, Java, and Kotlin Multiplatform.
https://square.github.io/okio/
Apache License 2.0
8.72k stars 1.17k forks source link

Add module-info.java #1486

Open Sineaggi opened 1 month ago

Sineaggi commented 1 month ago

Adds a new java compile task (depends on jvm dependencies and kotlin class files) to compile a module-info.java file, and puts it into the jvm jar.

I've never done this for a kotlin-multiplatform project, so I'm sure there are improvements. I looked into using multi-platforms 'withJvm' feature, but that dropped the files that were compiled with java 8 directly into the resulting jar. At least with this custom sourceset approach, we can compile the file with java 9 and put the module-info.class file where we want (META-INF/versions/9).

Sineaggi commented 1 month ago

The fixupmessage is necessary as okio is not yet https://github.com/square/okio/pull/1363 on bnd 7.0.0 https://github.com/bndtools/bnd/issues/2227.

EDIT: Fixed in https://github.com/square/okio/pull/1487

Sineaggi commented 4 weeks ago

We should be able to simplify the build once base-line increases to java > 8. We'd be able to remove the multi-release jar, and just use multiplatform's java source folder directly. We wouldn't need the META-INF/versions directory, nor the Multi-Release: true entry in the manifest.

Sineaggi commented 4 weeks ago

I was able to simplify module compilation via the docs here https://kotlinlang.org/docs/gradle-configure-project.html#configure-with-java-modules-jpms-enabled.

JakeWharton commented 4 weeks ago

Unfortunately I suspect it will be a long while before we drop Java 8 support completely.

Sineaggi commented 4 weeks ago

Unfortunately I suspect it will be a long while before we drop Java 8 support completely.

That's fine if you're alright with the mild additional complexity that an mr-jar brings along with it.

JakeWharton commented 4 weeks ago

It's probably worth it, yeah. I'll talk to Jesse about it soon and see if he has any concerns.

I have a similar setup (with some different complexities) in Retrofit. I should add a module descriptor there, too, although I have about 20 modules to add it to there...

Sineaggi commented 4 weeks ago

It might be possible to remove the dependency on java.logging too, if on versions > 8 we use System.Logger instead. Afaik it's only used for one utility class, and even then only for warning.

JakeWharton commented 4 weeks ago

Yes, that would be great, too!

Sineaggi commented 4 weeks ago

The logger classes have been added. I converted them to kotlin and made them internal to not trigger the api validator. Speaking of, I don't think the api-validator currently checks that the api is identical between two classes in an mr-jar.

JakeWharton commented 4 weeks ago

Yeah I saw your issue on BCV. Thanks for thinking of it. I'm not sure whether it's BCV's problem or not. From what I remember MR jars only replace classes at runtime and have no effect on compile-time. So even if you were to expose new API in one, it cannot be linked against in code (except through reflection at runtime).

Sineaggi commented 4 weeks ago

Code should be ready for review.

Sineaggi commented 3 weeks ago

There's an issue where the new Logger class isn't being included in the jar.

Sineaggi commented 3 weeks ago

Ok so if you don't exclude the META-INF folder that the java9 kotlin compile task generated, you'll get strange errors on java 11

Error occurred during initialization of boot layer
java.lang.NullPointerException
Sineaggi commented 3 weeks ago

Is there anything else I've missed and should add? Any further simplifications to the build process?