urlencoder icon indicating copy to clipboard operation
urlencoder copied to clipboard

Support Java 8

Open krzema12 opened this issue 1 year ago • 6 comments

Hi! We use the library in https://github.com/krzema12/snakeyaml-engine-kmp. There's a request to support Java 8 (https://github.com/krzema12/snakeyaml-engine-kmp/issues/202), however urlencoder is compatible with Java 11+:

https://github.com/ethauvin/urlencoder/blob/dd3d500496b8768a89ff94d1cd0343e739a72940/buildSrc/src/main/kotlin/buildsrc/conventions/lang/kotlin-multiplatform-base.gradle.kts#L46-L50

Would it be possible to support Java 8 as well? Technically it's an LTS release that is still supported by some major vendors.

krzema12 avatar Jul 16 '24 20:07 krzema12

Hi @krzema12!

I don't recall specifically, but I believe we couldn't support Java 8 because of some KMP restrictions.

@aSemy might be able to clarify some more.

ethauvin avatar Jul 16 '24 21:07 ethauvin

The config specifying Java 11 was already defined before I started updating the project to KMP :)

https://github.com/ethauvin/urlencoder/blob/34b69a7d1f3570aa056285253376ed7a7bde03d8/lib/build.gradle.kts#L50-L54

aSemy avatar Jul 17 '24 07:07 aSemy

@aSemy Ah. Thanks for looking into it.

It's up to you if you want to support Java 8. It's over 10 years old and no-longer receiving public updates.

ethauvin avatar Jul 17 '24 12:07 ethauvin

@ethauvin so here's the full story.

The dependency chain looks as follows: kaml -> snakeyaml-engine-kmp -> urlencoder. Some time ago, kaml was converted to a Kotlin Multiplatform library, previously only the JVM was supported. This conversion involved switching from snakeyaml-engine to snakeyaml-engine-kmp. Apparently as a result, a regression was introduced and kaml no longer supports Java 8: https://github.com/charleskorn/kaml/discussions/580. We asked the user why he needs Java 8, and it's a limitation of some tool related to Minecraft: https://github.com/krzema12/snakeyaml-engine-kmp/issues/202#issuecomment-2211733386

I'd say, if supporting Java 8 is as trivial as adjusting the two values mentioned by @aSemy, I think we can do it for the sake of fixing the regression in the upstream project.

krzema12 avatar Jul 17 '24 13:07 krzema12

@krzema12 It's a little more complicated than that.

If @aSemy is willing to make the changes and test them, I'll publish a new version. I just don't have the bandwidth for much more than that right now.

ethauvin avatar Jul 17 '24 14:07 ethauvin

It'd be nice to be able to build with Java 21, but set the Java target to 8, and then use Java 8 to run the tests.

I've been trying to learn how to do that with KGP, but I've always struggled. So this would be a good opportunity to learn...

But yes, I also think that Java 8 is too old and should die 😄

I'll see if I have time this week to make a PR.

aSemy avatar Jul 18 '24 07:07 aSemy

Hi @ethauvin and @krzema12, I arrived here backtracking from KAML like you initially did. Since I need KAML for a Kotest PR in Java 8 compatibility I thought I'd try to get it working. I created a PR which looks good locally. Strangely, the GitHub Action won't start in my repo. Nevertheless, I hope my change works and is fine for you. If anything happens in the pipeline I'll fix it. Cheers, Ronny

rbraeunlich avatar Oct 15 '24 15:10 rbraeunlich

@rbraeunlich thanks for picking it up! I see CI for 8 us failing on not having the Java's HTTP client: https://github.com/ethauvin/urlencoder/blob/dd3d500496b8768a89ff94d1cd0343e739a72940/buildSrc/src/main/kotlin/buildsrc/utils/Rife2TestListener.kt#L46

I think what we ideally want is building with newest possible JDK, but targeting desired Java versions - what @aSemy suggested.

krzema12 avatar Oct 15 '24 16:10 krzema12

Sorry, I didn't see this in the IDE. I've replaced the HttpClient with Apache Http. Hope it works now. Or maybe we could use a different Java version for buildSrc, since it shouldn't end up in the final JAR.

rbraeunlich avatar Oct 15 '24 17:10 rbraeunlich

Thanks @rbraeunlich and @krzema12 for doing this.

I've just published a new snapshot with your changes and some additional updated dependencies. Unfortunately, I do not have the bandwidth to fully test it at this time, so please report any issues.

@aSemy any thoughts on this?

ethauvin avatar Oct 15 '24 20:10 ethauvin

Looks good to me 👍

If you will allow me to be a perfectionist though, I would recommend the following:

  • Hardcode Gradle to use a specific JDK for the Daemon. https://docs.gradle.org/8.10/userguide/gradle_daemon.html#sec:daemon_jvm_criteria This will permit using the HttpClient (requiring JDK 11+) in buildSrc. (This can be done in a lazy way)
  • Use kotlin { jvmToolchain(21) } to make Kotlin use JDK 21 to compile Kotlin. (Newer JDKs are faster/have more fixes, though I will concede the impacts will be minor.)
  • Set -Xjdk-release=8 to make the compiled Java compatible with Java 8.
  • Add new separate JVM test runs that use a specific Java launcher to test the code runs using JDKs 8, 11, 17, 21 (probably don't need all of these versions, maybe only 8 and 21).
  • Add the Foojay toolchain resolver so Gradle can automatically get the required JDKs.

But this can be implemented in the future, especially as some of the features as still incubating. The current solution is good enough!

aSemy avatar Oct 16 '24 10:10 aSemy

@aSemy

Looks good to me 👍

If you will allow me to be a perfectionist though, I would recommend the following:

No worries, greatly appreciated. This is definitely more your cup of tea than mine.

  • Hardcode Gradle to use a specific JDK for the Daemon. https://docs.gradle.org/8.10/userguide/gradle_daemon.html#sec:daemon_jvm_criteria This will permit using the HttpClient (requiring JDK 11+) in buildSrc. (This can be done in a lazy way)

That, frankly, I don't care about. If curl was an option, I'd use it.

  • Use kotlin { jvmToolchain(21) } to make Kotlin use JDK 21 to compile Kotlin. (Newer JDKs are faster/have more fixes, though I will concede the impacts will be minor.)

I changed the GH publish action to use 21. Thanks for pointing it out.

  • Set -Xjdk-release=8 to make the compiled Java compatible with Java 8.

Isn't that redundant, since the jvmTarget compile option is already set? As far as I can tell, kotlinc thinks it's not needed and simply ignores it.

ethauvin avatar Oct 17 '24 03:10 ethauvin

Hi @ethauvin, is there a chance for a 1.6.0 anytime soon? I've got two more hops to make Java 8 compatible before I arrive at what I originally wanted to do 😅

rbraeunlich avatar Oct 18 '24 07:10 rbraeunlich

That, frankly, I don't care about. If curl was an option, I'd use it.

Completely reasonable. My reasoning is because I prefer to use a few external dependencies as possible. Also, using a consistent JDK for Gradle also increases build cache hits, although again. But I'll fully admit I'm nitpicking: the impact is negligible for a small library without remote cache.

  • Set -Xjdk-release=8 to make the compiled Java compatible with Java 8.

Isn't that redundant, since the jvmTarget compile option is already set? As far as I can tell, kotlinc thinks it's not needed and simply ignores it.

I thought the jvmTarget and -Xjdk-release flags were incompatible with each other, but maybe that's changed...

aSemy avatar Oct 18 '24 08:10 aSemy

@rbraeunlich is the snapshot working for you?

ethauvin avatar Oct 18 '24 20:10 ethauvin

That's a tough one @ethauvin What I can say is that all SnakeYaml Engine KMP tests pass with target Java 8 and the 1.6.0-SNAPSHOT. You can check here: https://github.com/rbraeunlich/snakeyaml-engine-kmp/actions/runs/11416460433 But I cannot tell how extensive the tests in that project cover the UrlEncoder.

rbraeunlich avatar Oct 19 '24 10:10 rbraeunlich

If the SnakeKMP tests pass with the snapshot version, then that's a very good indication 👍. I say go ahead with the 1.6.0 release.

aSemy avatar Oct 19 '24 10:10 aSemy

Version 1.6.0 is out. Might take a bit to show up on Maven Central.

ethauvin avatar Oct 19 '24 16:10 ethauvin