urlencoder
urlencoder copied to clipboard
Support Java 8
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.
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.
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 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 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 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.
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.
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 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.
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.
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?
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=8to 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
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=8to 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.
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 😅
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=8to make the compiled Java compatible with Java 8.Isn't that redundant, since the
jvmTargetcompile option is already set? As far as I can tell,kotlincthinks 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...
@rbraeunlich is the snapshot working for you?
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.
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.
Version 1.6.0 is out. Might take a bit to show up on Maven Central.