Fix incremental compilation with JDK16 using toolchains #16066

rieske · 2021-02-08T12:23:44Z

add a test to compile java using toolchains with a range of JDKs
remove uses of private Java compiler APIs in incremental compiler plugin

bmuskalla

LGTM. The one workaround with the --add-opens is fine but it would be good to look into my comment and see if we can continue without it

bmuskalla · 2021-02-08T14:04:07Z

subprojects/language-java/src/main/java/org/gradle/api/tasks/compile/JavaCompile.java

@@ -349,6 +353,10 @@ private DefaultJavaCompileSpec createBaseSpec() {
    private void applyToolchain(ForkOptions forkOptions) {
        final JavaInstallationMetadata metadata = getToolchain();
        forkOptions.setJavaHome(metadata.getInstallationPath().getAsFile());
+        if (isToolchainCompatibleWithJava16()) {
+            // used by the incremental compiler plugin, see org.gradle.internal.compiler.java.ClassNameCollector
+            forkOptions.getJvmArgs().addAll(Arrays.asList("--add-opens", "jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED"));


I might be worthing looking to actually fixing this without the add opens. I pushed a small WIP here that I played around with, we might not need this workaround for this. WDYT?

2fc4346

So that they are compatible with Java 8. On Java 8, calling ((JavacTask) delegate).getElements() in the IncrementalCompileTask of the incremental compiler plugin, somehow makes the the deprecation messages print the class names with their fully qualified names, in this case - com.example.Foo.

rieske · 2021-02-10T10:12:07Z

...-compiler-plugin/src/main/java/org/gradle/internal/compiler/java/IncrementalCompileTask.java

@@ -68,7 +68,7 @@ public void setLocale(Locale locale) {
    @Override
    public Boolean call() {
        if (delegate instanceof JavacTask) {
-            ClassNameCollector collector = new ClassNameCollector(relativize);
+            ClassNameCollector collector = new ClassNameCollector(relativize, ((JavacTask) delegate).getElements());


Calling javacTask.getElements() here has a rather weird side effect on deprecation messages in Java 8 - see this commit: b04b5ab
Not sure if there can be more implications here.

I think the side effect is not too bad but would be good to capture in the tests instead of ignoring it. Initially, I thought they dropped the class name completely which is not great.

And would be nice to pull the cast out as we do the same cast in the next line. Time to switch to Java 14 with pattern matching :)

I wonder also if the instanceof-or-throw can be pulled out to the constructor so that the delegate field is always a JavacTask here. Let's see what CI thinks.

bmuskalla

Minor nits and some questions. Generally good to go. Let me know if I should review the remaining changes

bmuskalla · 2021-02-10T14:25:40Z

...java-compiler-plugin/src/main/java/org/gradle/internal/compiler/java/ClassNameCollector.java

        if (symbol.endsWith("module-info")) {
            symbol = "module-info";
+        } else if (typeElement.getNestingKind().isNested()) {
+            symbol = elements.getBinaryName(typeElement).toString();


bmuskalla · 2021-02-10T14:27:36Z

...-compiler-plugin/src/main/java/org/gradle/internal/compiler/java/IncrementalCompileTask.java

@@ -68,7 +68,7 @@ public void setLocale(Locale locale) {
    @Override
    public Boolean call() {
        if (delegate instanceof JavacTask) {
-            ClassNameCollector collector = new ClassNameCollector(relativize);
+            ClassNameCollector collector = new ClassNameCollector(relativize, ((JavacTask) delegate).getElements());


I think the side effect is not too bad but would be good to capture in the tests instead of ignoring it. Initially, I thought they dropped the class name completely which is not great.

bmuskalla · 2021-02-10T14:29:40Z

...-compiler-plugin/src/main/java/org/gradle/internal/compiler/java/IncrementalCompileTask.java

@@ -68,7 +68,7 @@ public void setLocale(Locale locale) {
    @Override
    public Boolean call() {
        if (delegate instanceof JavacTask) {
-            ClassNameCollector collector = new ClassNameCollector(relativize);
+            ClassNameCollector collector = new ClassNameCollector(relativize, ((JavacTask) delegate).getElements());


And would be nice to pull the cast out as we do the same cast in the next line. Time to switch to Java 14 with pattern matching :)

bmuskalla · 2021-02-10T14:30:23Z

...src/integTest/groovy/org/gradle/api/tasks/compile/JavaCompileToolchainIntegrationTest.groovy

+
+    }
+
+    def "can compile Java using different JDKs"() {


Maybe use #javaVersion to see which iterations are skipped?

We get this information without explicitly embedding the variable in the test header (since Spock 2 I believe): https://ge.gradle.org/s/vhyeam6vqgtqi/tests

bmuskalla · 2021-02-10T14:33:16Z

...g/gradle/java/compile/incremental/AbstractSourceIncrementalCompilationIntegrationTest.groovy

+                        };
+                    }
+                }
+            """


Do we need to check for anonymous classes using method-level nested classes?

class Anonymous2 { SomeInterface getAnonymous() { class Foobar implements SomeInterface { } return new Foobar() { public int foo() { return 42; } }; } }

Also, for our use case, do lambdas behave the same as anonymous classes for this use case?

…ctor

…t class

dreis2211 · 2021-02-11T14:19:24Z

Is this maybe possible to backport to 6.x?

rieske added 5 commits February 5, 2021 15:33

Add a test compiling Java using a range of JDKs

b525a17

Merge branch 'master' into vv/jdk16/toolchains

627d7ee

Bust the cache to rerun test on CI

6788132

Fix class visibility for incremental compiler using JDK16 toolchain

56c89f3

Extend toolchain compilation test coverage to JDK17

06f3afb

rieske added from:member in:jvm-ecosystem @core Issue owned by GBT Core labels Feb 8, 2021

rieske self-assigned this Feb 8, 2021

rieske requested review from bmuskalla and ljacomet February 8, 2021 12:24

rieske added this to the 7.0 RC1 milestone Feb 8, 2021

bmuskalla approved these changes Feb 8, 2021

View reviewed changes

rieske added 6 commits February 8, 2021 18:51

Do not use private compiler APIs in incremental compile task

fb8f8d7

Add integration test for incremental compilation with anonymous classes

997da0b

Merge branch 'master' into vv/jdk16/toolchains

5d1ac35

Normalize all nested type elements to binary names

f6e9975

Merge branch 'master' into vv/jdk16/toolchains

ef5a115

rieske commented Feb 10, 2021

View reviewed changes

rieske marked this pull request as ready for review February 10, 2021 14:05

rieske requested a review from bmuskalla February 10, 2021 14:05

bmuskalla reviewed Feb 10, 2021

View reviewed changes

rieske added 3 commits February 10, 2021 17:14

Pull out javac task type check to incremental compiler plugin constru…

34e5a64

…ctor

Expand test coverage for Java incremental compiler plugin

4bd0230

Move anonymous classes incremental compilation tests to Java base tes…

03def01

…t class

rieske merged commit f8f9f6f into master Feb 11, 2021

rieske deleted the vv/jdk16/toolchains branch February 11, 2021 14:10

rieske removed this from the 7.0 RC1 milestone Feb 12, 2021

rieske modified the milestones: 6.8.x, 7.0 RC1 Feb 12, 2021

rieske mentioned this pull request Feb 12, 2021

[Backport] Support building with JDK16 using Java toolchains #16126

Closed

rieske mentioned this pull request Apr 20, 2021

Backport JDK16 support using toolchains to 6.9 #16890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incremental compilation with JDK16 using toolchains #16066

Fix incremental compilation with JDK16 using toolchains #16066

rieske commented Feb 8, 2021 •

edited

bmuskalla left a comment

bmuskalla Feb 8, 2021

rieske Feb 10, 2021

bmuskalla Feb 10, 2021

bmuskalla Feb 10, 2021

rieske Feb 10, 2021

bmuskalla left a comment

bmuskalla Feb 10, 2021

bmuskalla Feb 10, 2021

bmuskalla Feb 10, 2021

bmuskalla Feb 10, 2021

rieske Feb 10, 2021

bmuskalla Feb 10, 2021

bmuskalla Feb 10, 2021

dreis2211 commented Feb 11, 2021

Fix incremental compilation with JDK16 using toolchains #16066

Fix incremental compilation with JDK16 using toolchains #16066

Conversation

rieske commented Feb 8, 2021 • edited

bmuskalla left a 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

bmuskalla left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreis2211 commented Feb 11, 2021

rieske commented Feb 8, 2021 •

edited