Properly fix Java 8 API compatibility (#2218) #2350

Vampire · 2023-06-26T19:26:31Z

There are 7 methods in ByteBuffer that have the problem described in #2218 and was attempted to be fixed in #2219.
Unfortunately, the fix in #2219 was incomplete as another of these 7 methods is used in the same class.
Also it could happen anytime and with similar cases, that this happens again and is only recognized very late.

With this PR, the JVM toolchains feature is used to ensure the proper API being compiled against.
As a side-effect I also improved the module-info.java compilation that was done "sub-optimally"
by changing configuration of the task during its execution phase, and compiling into the same
output directory as the main compile task and then moving it out in a doLast action,
and having a manual task dependency, ...

With the new setup, no explicit task dependencies are necessary, which are bad-practice,
the output directories are right from the start separated, and no manual task dependencies
are necessary anymore as the jar configuration and the compile task arguments both have the
necessary implicit task dependencies automatically and by using a CommandLineArgumentProvider
which also has the necessary implicit dependencies, there is also no configuration change at
execution time necessary anymore.

buildSrc/src/main/kotlin/Java9Modularity.kt

ALikhachev · 2023-07-11T18:22:28Z

buildSrc/src/main/kotlin/Java9Modularity.kt

-            // Also add the module-info.java source file to the Kotlin compile task;
-            // the Kotlin compiler will parse and check module dependencies,
-            // but it currently won't compile to a module-info.class file.
-            compileTask.source(sourceFile)


If you remove this line, please provide an alternative way. It was here to perform validation of the sources in accordance to the module-info file, and now nothing performs this validation.

Can you tell me what exactly that did / checked?
What kind of bad change did that capture, so what would make the build fail with it?

I'm sorry we lack documentation about this. Actually, Kotlin compiler is able to read module-info.java file and verify it (the comment says 'the Kotlin compiler will parse and check module dependencies'). It verifies that all package names in exports are correct and all requires dependencies are present (not so sure about latter, @ALikhachev ?).
What happens here is we add module-info.java location to the inputs of KotlinCompile task so Kotlin compiler can perform this verification.

Hm, I see.
But I'm not sure whether there is a good solution, besides having the normal compile task compile with Java 8 toolchain and having a second set of compile tasks with Java 11 toolchain and jdk-release 9 / jvm-target 9 that is hooked into the check task dependencies. :-/

If I just add -Xjdk-release=1.8 as free compiler arg into the existing (pre-my-PR) setup, there come a looot of errors about classes being in modules that are not depended upon.

If with the new (with-my-PR) setup I use JVM toolchain 17 (to reproduce the problematic bytecode eventually) and add jdk-release 1.8 / jvm-target 1.8, the correct bytecode is produced but still those errors are not verified as the module infos seem to be ignored.

If I additionally add the module-info.java to the KotlinCompile task again, I get the same wall of errors I get if I just add "jdk-release 1.8" to the existing setup.

If I remove the "jdk-release" and only keep the "jvm-target", the verifications work again, but the wrong byte-code is produced. grml

Let me try to sum this up: the problem is, when module-info.java is in the KotlinCompile task inputs AND jdk-release=1.8 specified to use correct Java API, then there are a lot of errors. What this errors are, exactly? I think this scenario is popular for multi-release JARs and should be somewhat supported without much setup, but I may be wrong.

However, after giving it a second thought, I'm not so sure we need this verification at all. We have JavaCompile task for module-info anyway — it will tell us if some of dependencies are missing and/or exported incorrectly, won't it?

No, it wont actually.

The JavaCompile task just compiles the module-info.java file.
It will complain if there is an error within, like requiring a module that is not present, or exporting a package that does not exist.
But the intended verification happens when compiling the Kotlin files JPMS aware.
So if you for example remove exports kotlinx.serialization; from core, formats/json compilation will fail with the verification for using classes in non-exported packages.
And if you for example remove requires transitive kotlinx.serialization.core; from formats/json, its compilation will fail with the verification for using classes without requiring the module.

This verification still has some bugs like https://youtrack.jetbrains.com/issue/KT-60583 and https://youtrack.jetbrains.com/issue/KT-60582 for example.

I've added the alternative means for the validation that @ALikhachev requested now.
It needed some work-arounds due to shortcomings in the Kotlin Gradle Plugin and in the module-info handling.
But all are reported and documented with code comments.
And from my tests hopefully the same verifications done before are now again done.
Thank you very much @Tapchicoma for your thorough help in getting this set up.

I see. Thank you for you careful study.

Can you please also file this bug

when module-info.java is in the KotlinCompile task inputs AND jdk-release=1.8 specified to use correct Java API, then there are a lot of errors.

so a compiler team can look properly at it (cc @udalov )

https://youtrack.jetbrains.com/issue/KT-60868

sandwwraith · 2023-07-24T14:39:16Z

buildSrc/src/main/kotlin/Java9Modularity.kt

-                        // the default source set depends on this new source set
-                        defaultSourceSet.dependsOn(this)
+                        // register and wire a task to verify module-info.java content
+                        val verifyModuleTask = registerVerifyModuleTask(


Am I right that this actually will cause the full compilation to happen twice (you've mentioned it somewhere)?
I believe it should be mentioned in the comments then

Yes, when running with check this is correct.
During pure building it should not happen.
I added that info to the comment now.

ALikhachev

LGTM, thank you.

Would you mind making a minor adjustment by adding a description (the description property of the task) to the verification task in addition to setting the group?

Vampire · 2023-08-01T13:24:51Z

Would you mind making a minor adjustment by adding a description

Not at all, done :-)

sandwwraith · 2023-08-01T15:02:01Z

@Vampire Thanks for all your efforts! I'm terribly sorry for not noticing it sooner, but your PR is based against master, while development is usually done in dev. At this point, dev is already updated to Kotlin 1.9.0. I've tried to put your commit on top of dev and I got a failed build:

> Task :kotlinx-serialization-core:compileKotlinJs FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':kotlinx-serialization-core:compileKotlinJs'.
> Failed to query the value of task ':kotlinx-serialization-core:compileKotlinJs' property 'incrementalModuleInfoProvider'.
   > Could not isolate value org.jetbrains.kotlin.gradle.incremental.IncrementalModuleInfoBuildService$Parameters_Decorated@757090b2 of type IncrementalModuleInfoBuildService.Parameters
      > Cannot query the value of task ':kotlinx-serialization-cbor:verifyKotlinJvmModule' property 'ownModuleName' because it has no value available.
        The value of this property is derived from:
          - task ':kotlinx-serialization-cbor:compileKotlinJvm' property 'moduleName'

I'm not sure why it happens, as you're setting ownModuleName explicitly. Can you rebase your branch on dev, please? Maybe @Tapchicoma could help us again.

Vampire · 2023-08-01T15:46:14Z

No problem, fixed.
I set ownModuleName explicitly, yes.
But I set it to compileTask.moduleName which is now deprecated and replaced by compileTask.kotlinOptions.moduleName.
The deprecated one seems to stay unset and so the ownModuleName also stayed unset.
I replaced that deprecated usage in both places and verified the verification still works as intended, so should be fine now hopefully.

sandwwraith · 2023-08-02T12:03:32Z

Thanks again!

includes fix in Kotlin/kotlinx.serialization#2328 or Kotlin/kotlinx.serialization#2350

sandwwraith requested a review from ALikhachev July 11, 2023 14:20

ALikhachev requested changes Jul 11, 2023

View reviewed changes

Vampire force-pushed the issue-2218 branch 2 times, most recently from ed9fcf0 to ba682a6 Compare July 16, 2023 13:33

sandwwraith requested a review from ALikhachev July 17, 2023 10:21

Vampire force-pushed the issue-2218 branch from ba682a6 to 07398b1 Compare July 20, 2023 13:32

Vampire requested a review from sandwwraith July 20, 2023 13:53

sandwwraith reviewed Jul 24, 2023

View reviewed changes

ALikhachev approved these changes Jul 31, 2023

View reviewed changes

Vampire force-pushed the issue-2218 branch from 07398b1 to 93a5a03 Compare August 1, 2023 13:23

Vampire requested review from ALikhachev and sandwwraith August 1, 2023 13:25

Properly fix Java 8 API compatibility (Kotlin#2218)

eced301

Vampire force-pushed the issue-2218 branch from 93a5a03 to eced301 Compare August 1, 2023 15:42

Vampire changed the base branch from master to dev August 1, 2023 15:43

sandwwraith approved these changes Aug 2, 2023

View reviewed changes

sandwwraith merged commit 8b231ff into Kotlin:dev Aug 2, 2023

Vampire deleted the issue-2218 branch August 2, 2023 13:21

My-Name-Is-Jeff added a commit to Skytils/SkytilsMod that referenced this pull request Jan 31, 2024

build: force kotlinx.serialization at >= 1.5.1

a65c24f

includes fix in Kotlin/kotlinx.serialization#2328 or Kotlin/kotlinx.serialization#2350

My-Name-Is-Jeff added a commit to Skytils/SkytilsMod that referenced this pull request Jan 31, 2024

build: force kotlinx.serialization at >= 1.5.1

c303ff5

includes fix in Kotlin/kotlinx.serialization#2328 or Kotlin/kotlinx.serialization#2350

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly fix Java 8 API compatibility (#2218) #2350

Properly fix Java 8 API compatibility (#2218) #2350

Vampire commented Jun 26, 2023

ALikhachev Jul 11, 2023

Vampire Jul 16, 2023

sandwwraith Jul 17, 2023

Vampire Jul 17, 2023

sandwwraith Jul 19, 2023 •

edited

Loading

sandwwraith Jul 19, 2023

Vampire Jul 20, 2023

sandwwraith Jul 24, 2023

Vampire Aug 1, 2023

sandwwraith Jul 24, 2023

Vampire Aug 1, 2023

ALikhachev left a comment •

edited

Loading

Vampire commented Aug 1, 2023

sandwwraith commented Aug 1, 2023

Vampire commented Aug 1, 2023

sandwwraith commented Aug 2, 2023

Properly fix Java 8 API compatibility (#2218) #2350

Properly fix Java 8 API compatibility (#2218) #2350

Conversation

Vampire commented Jun 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandwwraith Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ALikhachev left a comment • edited Loading

Choose a reason for hiding this comment

Vampire commented Aug 1, 2023

sandwwraith commented Aug 1, 2023

Vampire commented Aug 1, 2023

sandwwraith commented Aug 2, 2023

sandwwraith Jul 19, 2023 •

edited

Loading

ALikhachev left a comment •

edited

Loading