Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Gradle 8.4 #12655

Closed
risdenk opened this issue Oct 11, 2023 · 34 comments · Fixed by #12662
Closed

Upgrade to Gradle 8.4 #12655

risdenk opened this issue Oct 11, 2023 · 34 comments · Fixed by #12662
Assignees
Milestone

Comments

@risdenk
Copy link
Contributor

risdenk commented Oct 11, 2023

Description

Upgrade Gradle from 7.6 to 8.4 - supports building directly with JDK 21 LTS.

https://docs.gradle.org/8.4/release-notes.html

@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

Handled by #12650

@risdenk risdenk mentioned this issue Oct 11, 2023
@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

Merged to main as:

@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

Merged to branch_9x as:

@uschindler
Copy link
Contributor

There seems to be an issue with compiling the distribution tests:

> Task :lucene:core.tests:compileJava FAILED
error: invalid flag: -XX:+UseParallelGC
Usage: javac <options> <source files>
use --help for a list of possible options

> Task :lucene:distribution.tests:compileTestJava FAILED
error: invalid flag: -XX:+UseParallelGC
Usage: javac <options> <source files>
use --help for a list of possible options

Not sure why the test runner ARGS are used for compilation. Jenkins passes those as Gradle property.

I have no idea why they are only used for compilation of those tests.

@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

Hmmm I checked this with ./gradlew check on both JDK 17 and JDK 21 locally.

So it must be something w/ Gradle property being sent in :( - do you have a Jenkins job url? I can dig if not.

@uschindler
Copy link
Contributor

See this build. Gradle command line and env cars can be found there: https://jenkins.thetaphi.de/job/Lucene-main-Linux/44942/console

@uschindler
Copy link
Contributor

Jenkins uses: RUNTIME_JAVA_HOME and TEST_ARGS environment variables.

@uschindler
Copy link
Contributor

I have no idea why those are injected only for compilation of those tests. Core test compilation works fine. So there seems to be some difference on the distribution tests.

@dweiss ?

@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

RUNTIME_JAVA_HOME=/Library/Java/JavaVirtualMachines/openjdk.jdk/Contents/Home ./gradlew check -x test reproduced the same exception for me locally.

but for

> Task :lucene:core:compileJava FAILED
error: invalid flag: -XX:+UseParallelGC
Usage: javac <options> <source files>
use --help for a list of possible options

I'm still digging.

@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

I locally reverted gradlew changes and that didn't help fix anything. So it looks like its something in the newer gradle version and how RUNTIME_JAVA_HOME is being handled :/

@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

snippet from:

RUNTIME_JAVA_HOME=/Library/Java/JavaVirtualMachines/openjdk.jdk/Contents/Home ./gradlew check -x test -Pvalidation.git.failOnModified=false --info
> Task :lucene:core:compileJava FAILED
Caching disabled for task ':lucene:core:compileJava' because:
  Build cache is disabled
Task ':lucene:core:compileJava' is not up-to-date because:
  Task has failed previously.
The input changes require a full rebuild for incremental task ':lucene:core:compileJava'.
Modular extension, compilation paths, source set=main (module), mode=DEPENDENCIES_ON_MODULE_PATH:
  Module path: [empty]
  Class path:  [empty]
  Patches:     [empty]
Full recompilation is required because no incremental change information is available. This is usually caused by clean builds or changing compiler arguments.
Compiling with toolchain '/Library/Java/JavaVirtualMachines/openjdk.jdk/Contents/Home'.
Compiling with Java command line compiler '/Library/Java/JavaVirtualMachines/openjdk.jdk/Contents/Home/bin/javac'.
Starting process 'command '/Library/Java/JavaVirtualMachines/openjdk.jdk/Contents/Home/bin/javac''. Working directory: /Users/risdenk/repos/apache/lucene/lucene/core Command: /Library/Java/JavaVirtualMachines/openjdk.jdk/Contents/Home/bin/javac -XX:+UseParallelGC -XX:TieredStopAtLevel=1 -XX:ActiveProcessorCount=1 @/Users/risdenk/repos/apache/lucene/lucene/core/build/tmp/compileJava/java-compiler-args.txt
Successfully started process 'command '/Library/Java/JavaVirtualMachines/openjdk.jdk/Contents/Home/bin/javac''
error: invalid flag: -XX:+UseParallelGC
Usage: javac <options> <source files>
use --help for a list of possible options

@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

The cause is gradle/hacks/turbocharge-jvm-opts.gradle

If I comment out apply from: file('gradle/hacks/turbocharge-jvm-opts.gradle') in top level build.gradle things work.

@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

@dweiss already filed gradle/gradle#22746 and it looks like somehow the workaround in gradle/hacks/turbocharge-jvm-opts.gradle is broken in Gradle 8.4 w/ RUNTIME_JAVA_HOME

@uschindler do you want me to revert the changes? I'm happy to do so.

@uschindler
Copy link
Contributor

Hrrrrrm....

Let's see how this affects us us.

@risdenk
Copy link
Contributor Author

risdenk commented Oct 11, 2023

It has to be the interaction with

    // Any javac compilation tasks
    tasks.withType(JavaCompile) {
      dependsOn ":altJvmWarning"
      options.fork = true
      options.forkOptions.javaHome = jvmCurrent.javaHome

in alternative-jdk-support and turbocharge-jvm-opts.gradle trying to look at forking options

@dweiss
Copy link
Contributor

dweiss commented Oct 12, 2023

Let me take a look.

@dweiss
Copy link
Contributor

dweiss commented Oct 12, 2023

So it seems something has changed and the javaCompiler property is now always non-null, causing -X options to be passed to the forked compiler (instead of -J...). I've changed this check to this:

diff --git a/gradle/hacks/turbocharge-jvm-opts.gradle b/gradle/hacks/turbocharge-jvm-opts.gradle
index 0f404e096b8..e730d878080 100644
--- a/gradle/hacks/turbocharge-jvm-opts.gradle
+++ b/gradle/hacks/turbocharge-jvm-opts.gradle
@@ -57,8 +57,8 @@ allprojects {
                 // are not part of up-to-date checks but these are internal JVM flags so we
                 // don't care.
                 //
-                // Pass VM options via -J only with a custom javaHome AND when java toolchains are not used.
-                if (task.options.forkOptions.javaHome != null && task.javaCompiler.getOrNull() == null) {
+                // Pass VM options via -J when a custom javaHome is used and we're in fork mode.
+                if (task.options.fork && task.options.forkOptions.javaHome != null) {
                     return vmOpts.collect {"-J" + it}
                 } else {
                     return vmOpts

and it seems to pass for me. The toolchains condition was about dodging some problem with jdk api regeneration (like generateJdkApiJar19) but I just ran it with and without RUNTIME_JAVA_HOME and it worked fine... Perhaps @uschindler will remember what the problem was. I'll commit the above as a temporary workaround.

@uschindler
Copy link
Contributor

I don't remember any problem there. The toolchain use for generateJdkApiJarXX was added much later and I did not touch that. Maybe check git blame.

@dweiss
Copy link
Contributor

dweiss commented Oct 12, 2023

I did check that file's history - it used an explicit exclusion for compileMain19Java:

        if (task.path == ":lucene:core:compileMain19Java") {
          // uses "uschindler" toolchain method, which erases "dweiss" method, but later at configure time
          task.options.forkOptions.jvmArgs += vmOpts

Not worth spending more time on it - if something fails, we'll fix it.

@uschindler
Copy link
Contributor

Hi, there are some other problem with the regenerate task, toolchains do not automatically download, although enabled. I am digging.

@uschindler
Copy link
Contributor

uschindler commented Oct 12, 2023

Very strange output if I clean the JDK cache in ~/.gradle/jdks:

> Task :lucene:core:generateJdkApiJar19 SKIPPED
Launcher for Java 21 is not available; skipping regeneration of Panama Foreign & Vector API JAR.
Error: No matching toolchains found for requested specification: {languageVersion=19, vendor=any, implementation=vendor-specific} for WINDOWS on x86_64.
Please make sure to point env 'JAVA21_HOME' to exactly JDK version 21 or enable Gradle toolchain auto-download.

> Task :lucene:core:generateJdkApiJar20 SKIPPED
Launcher for Java 21 is not available; skipping regeneration of Panama Foreign & Vector API JAR.
Error: No matching toolchains found for requested specification: {languageVersion=20, vendor=any, implementation=vendor-specific} for WINDOWS on x86_64.
Please make sure to point env 'JAVA21_HOME' to exactly JDK version 21 or enable Gradle toolchain auto-download.

> Task :lucene:core:generateJdkApiJar21 SKIPPED
Launcher for Java 21 is not available; skipping regeneration of Panama Foreign & Vector API JAR.
Error: No matching toolchains found for requested specification: {languageVersion=21, vendor=any, implementation=vendor-specific} for WINDOWS on x86_64.
Please make sure to point env 'JAVA21_HOME' to exactly JDK version 21 or enable Gradle toolchain auto-download.

Please not the wrong version numbers, it looks like the variable name is incorrectly used. But actually also Java 21 should be available on Adoptium.

@uschindler
Copy link
Contributor

Task info by gradlew tasks is correct:

generateJdkApiJar19 - Regenerate the API-only JAR file with public Panama Foreign & Vector API from JDK 19
generateJdkApiJar20 - Regenerate the API-only JAR file with public Panama Foreign & Vector API from JDK 20
generateJdkApiJar21 - Regenerate the API-only JAR file with public Panama Foreign & Vector API from JDK 21

@uschindler
Copy link
Contributor

I did check that file's history - it used an explicit exclusion for compileMain19Java:

        if (task.path == ":lucene:core:compileMain19Java") {
          // uses "uschindler" toolchain method, which erases "dweiss" method, but later at configure time
          task.options.forkOptions.jvmArgs += vmOpts

Not worth spending more time on it - if something fails, we'll fix it.

This is no longer relevant, as we no longer use the toolkit specific compiler. We now generate tha API jars and patch them into java.base module.

Toolkits are only used for launching the API extractor. This is somehow broken since Gradle 8.4, I try to figure out what's wrong.

@uschindler
Copy link
Contributor

OK, I found the bug for the icorrect logger output, but that's not the issue here. The fix is easy. I will provide a PR for the regenerator.

The reason is that it looks like the for (x : list) {} you cannot use the x in a closure, because the loop variable is not final (like in Java). This may be a change in newer Groovy versions.

But it looks like at least for windows,, Gradle toolkits can't autodownload the JDK version. I will dig further.

@uschindler
Copy link
Contributor

This fixes the logging problem:

diff --git a/gradle/generation/extract-jdk-apis.gradle b/gradle/generation/extract-jdk-apis.gradle
index 28f56ea6432..020dc052813 100644
--- a/gradle/generation/extract-jdk-apis.gradle
+++ b/gradle/generation/extract-jdk-apis.gradle
@@ -38,7 +38,7 @@ configure(project(":lucene:core")) {
     apiextractor "org.ow2.asm:asm:${scriptDepVersions['asm']}"
   }
 
-  for (jdkVersion : mrjarJavaVersions) {
+  mrjarJavaVersions.each { jdkVersion ->
     def task = tasks.create(name: "generateJdkApiJar${jdkVersion}", type: JavaExec) {
       description "Regenerate the API-only JAR file with public Panama Foreign & Vector API from JDK ${jdkVersion}"
       group "generation"

@uschindler
Copy link
Contributor

It looks like gradle toolchain download is fully broken in Gradle 8.4. It always says that it cannot find any JDK, no matter which OS (Windows, Linux).

uschindler added a commit to uschindler/lucene that referenced this issue Oct 12, 2023
@uschindler
Copy link
Contributor

uschindler commented Oct 12, 2023

Here is the draft PR, it only fixes the logging, but I have no idea why the toolchains are not downloaded: #12662

If you clean ~/.gradle/jdks and then run gradlew :lucene:core:regenerate it tells you that it found no suitable JDK.

@uschindler
Copy link
Contributor

I found the issue, in Gradle 8+ you need to explicitely configure toolchain repositories:

Using automatic toolchain downloading without having a repository configured
Automatic toolchain downloading without explicitly providing repositories to use is no longer supported. See user manual for more information.

I will add the foojay one in the PR.

@dweiss
Copy link
Contributor

dweiss commented Oct 12, 2023

because the loop variable is not final (like in Java)

In groovy closures do have access to their full context (including local variables outside). That code used gstring, which is a dynamically evaluated string - if you used a string concatenation, it'd be immutable and worked fine.

@uschindler
Copy link
Contributor

OK, PR is ready. The plugin needs to be in settings.gradle (it is a so-called "settings" plugin, which works on gradle toplevel without project context.

PR: #12662 (please review)

@uschindler
Copy link
Contributor

because the loop variable is not final (like in Java)

In groovy closures do have access to their full context (including local variables outside). That code used gstring, which is a dynamically evaluated string - if you used a string concatenation, it'd be immutable and worked fine.

The problem was not the gstring/strings. Problem was that the onlyIf closure is executed at execution time (so delayed) and not at config time. At execution time, the for loop was already on jdkVersion==21 (the last entry because the loop had already executed at config time). By using a each closure the jdkVersion is private to the closure and the build-time evaluation has its own jdkVersion variable (which is a constant).

@risdenk
Copy link
Contributor Author

risdenk commented Oct 12, 2023

Thanks @uschindler and @dweiss sorry for the work here :( I'll try to make sure I test all these cases next time.

@dweiss
Copy link
Contributor

dweiss commented Oct 12, 2023

By using a each closure the jdkVersion is private to the closure and the build-time evaluation has its own jdkVersion variable (which is a constant).

Ah, thanks for clarifying, Uwe. I only looked at the diff and suspected the gstring to blame - overall it's the same thing - the previous code referenced a local variable (not the current loop's value, the variable). Switching to 'each' makes the variable a closure argument, so it's indeed constant (as in: a parameter)... it sometimes gets tricky because if you nest another loop/closure, it can still observe the parameter's value and you can change it... It's a virtue and a pain, depends on when you use it...

For what it's worth, I've tried switching a few builds to Kotlin... but I wasn't convinced. It's nice to have a type checker but there were many things that required more verbose constructs to achieve.

@dweiss
Copy link
Contributor

dweiss commented Oct 12, 2023

No worries, @risdenk - it's a big effort already, there'll always be problems, especially with major version upgrades.

@dweiss dweiss added this to the 9.9.0 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants