Restore an edge case of non-javac toolchain tools being used as a Java compiler #24014

alllex · 2023-02-23T12:37:39Z

Gradle 8 introduced unconditional usage of Java toolchains for Java compilation. However, providing custom tool paths (custom Java home or a compiler executable path) was still supported.

In addition to that, Gradle 8 protect users from doubly-configuring the toolchain via the tool property and a Java home/executable. Validations were added to make sure that toolchain from a custom path agrees with the final resolved toolchain property. In particular, the path to the executable is checked to exactly match the path to javac within the toolchain. This, however, made it impossible to use custom Java compilers that rely on being started as a JVM via java launcher tool.

This PR relaxes the checks to only validate that a custom executable path leads to a tool within the same toolchain without checking which exact tool it is. In particular, a path to the java tool can be specified.

Fixes: #23990

alllex · 2023-02-24T10:32:22Z

...ojects/platform-jvm/src/main/java/org/gradle/jvm/toolchain/internal/JavaExecutableUtils.java

            throw new InvalidUserDataException("The configured executable is a directory (" + executableFile.getAbsolutePath() + ")");
        }
-        return executableFile;
+        return executableAbsoluteFile;


We recently deprecated the usage of relative paths, but we still need to support it until removed.

This path ends up going to the command line executor and breaks it if it's still a relative path. So the options were to either do the absolute path conversion right before execution or here. I think here is better, because we already do a number of checks on the absolute path.

It worked before only because we always used the toolchain paths, relying on exact match between toolchain tool path and forkOptions.executable. But we relax this contraint in this PR.

TheMrMilchmann · 2023-02-24T18:45:07Z

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

+                def customJavaLauncher = javaToolchains.launcherFor {
+                    languageVersion.set(JavaLanguageVersion.of(${jdk.javaVersion.majorVersion}))
+                }.get()


I assume that this would still fail if a different default toolchain is configured at project level. javaCompiler would use javac from the default toolchain and the selected toolchain here might not necessarily match. It would probably be a good idea to also test that situation (which I think should still be supported too).

I guess this could be worked around by retrieving the launcher for the toolchain of the configured JavaCompiler somehow but I believe that getting the toolchain for a tool is not currently possible.

I assume that this would still fail if a different default toolchain is configured at project level.

It should not fail. Since we are configuring forkOptions.executable for this JavaCompile task, it will take priority over any toolchain configured on the project level. Here is how the override is selected:

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

Lines 105 to 108 in c11d14a

Provider<JavaCompiler> javaCompilerConvention = getProviderFactory()

.provider(() -> JavaCompileExecutableUtils.getExecutableOverrideToolchainSpec(this, objectFactory))

.flatMap(javaToolchainService::compilerFor)

.orElse(javaToolchainService.compilerFor(it -> {}));

I'll update the test to show this

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

alllex · 2023-02-27T13:58:18Z

@bot-gradle test this

bot-gradle · 2023-02-27T13:58:25Z

I've triggered the following builds for you:

PullRequestFeedback build

ljacomet

This all makes sense, though I would investigate a fix in the upcoming 8.0.2

jvandort · 2023-02-27T18:47:42Z

...integ-testing/src/main/groovy/org/gradle/integtests/fixtures/jvm/JavaToolchainFixture.groovy

@@ -97,7 +97,7 @@ trait JavaToolchainFixture {
     * Returns the Java version from the compiled class bytecode.
     */
    JavaVersion classJavaVersion(File classFile) {
-        assert classFile.exists()
+        assert classFile.exists(), "Class file ${classFile} does not exist"


Aren't assertions typically written with a : instead of comma, at least in Java? Does the comma mean something different here?

You are correct. The comma syntax also works, but I completely forgot that Spock, in case of an error, provides all the info for expressions anyway. So this message here is redundant. Thanks for noticing!

alllex · 2023-02-28T09:33:02Z

@bot-gradle test and merge

bot-gradle · 2023-02-28T09:33:10Z

Your PR is queued. See the queue page for details.

bot-gradle · 2023-02-28T11:19:14Z

I've triggered a build for you.

… tools being used as a Java compiler (Gradle 8.0.x) Backporting of a fix from #24014 Co-authored-by: Alex Semin <asemin@gradle.com>

alllex added 3 commits February 23, 2023 13:37

Support non-javac toolchain tools to be used as a Java compiler

6b9f039

Resolve java executable to absolute files

ba332d7

Match Java homes canonically

c11d14a

alllex self-assigned this Feb 23, 2023

alllex changed the title ~~Support non-javac toolchain tools to be used as a Java compiler~~ Restore an edge case of non-javac toolchain tools being used as a Java compiler Feb 23, 2023

alllex marked this pull request as ready for review February 24, 2023 10:07

alllex requested a review from a team as a code owner February 24, 2023 10:07

alllex requested review from jvandort, wolfs and big-guy February 24, 2023 10:07

alllex mentioned this pull request Feb 24, 2023

Gradle 8.0.+ silently dropped support for custom compilers in JavaCompile #23990

Closed

alllex commented Feb 24, 2023

View reviewed changes

TheMrMilchmann reviewed Feb 24, 2023

View reviewed changes

wolfs approved these changes Feb 27, 2023

View reviewed changes

...src/integTest/groovy/org/gradle/api/tasks/compile/JavaCompileToolchainIntegrationTest.groovy Outdated Show resolved Hide resolved

Add up-to-date checks for the custom-compiler test

74bdc8a

alllex requested a review from wolfs February 27, 2023 11:40

wolfs approved these changes Feb 27, 2023

View reviewed changes

Fix ECJ test by constraining Java versions

277e222

ljacomet approved these changes Feb 27, 2023

View reviewed changes

jvandort approved these changes Feb 27, 2023

View reviewed changes

wolfs added this to the 8.1 RC1 milestone Feb 28, 2023

Remove unnecessary assert message

7ce1d3d

gradle deleted a comment from alllex Feb 28, 2023

alllex mentioned this pull request Feb 28, 2023

Restore an edge case of non-javac toolchain tools being used as a Java compiler (Gradle 8.0.x) #24072

Merged

bot-gradle merged commit e87ed79 into master Feb 28, 2023

bot-gradle deleted the alllex/jvm/toolchain-relax-javac branch February 28, 2023 12:44

bot-gradle added a commit that referenced this pull request Feb 28, 2023

Merge pull request #24072 Restore an edge case of non-javac toolchain…

a21ec6d

… tools being used as a Java compiler (Gradle 8.0.x) Backporting of a fix from #24014 Co-authored-by: Alex Semin <asemin@gradle.com>

ljacomet mentioned this pull request Jun 12, 2023

No longer possible to specify a executable which is not part of a Java toolchain for a JavaExec task #25345

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore an edge case of non-javac toolchain tools being used as a Java compiler #24014

Restore an edge case of non-javac toolchain tools being used as a Java compiler #24014

alllex commented Feb 23, 2023 •

edited

alllex Feb 24, 2023

TheMrMilchmann Feb 24, 2023 •

edited

TheMrMilchmann Feb 24, 2023

alllex Feb 27, 2023

alllex commented Feb 27, 2023

bot-gradle commented Feb 27, 2023

ljacomet left a comment

jvandort Feb 27, 2023

alllex Feb 28, 2023

alllex commented Feb 28, 2023

bot-gradle commented Feb 28, 2023

bot-gradle commented Feb 28, 2023

	Provider<JavaCompiler> javaCompilerConvention = getProviderFactory()
	.provider(() -> JavaCompileExecutableUtils.getExecutableOverrideToolchainSpec(this, objectFactory))
	.flatMap(javaToolchainService::compilerFor)
	.orElse(javaToolchainService.compilerFor(it -> {}));

Restore an edge case of non-javac toolchain tools being used as a Java compiler #24014

Restore an edge case of non-javac toolchain tools being used as a Java compiler #24014

Conversation

alllex commented Feb 23, 2023 • edited

alllex Feb 24, 2023

Choose a reason for hiding this comment

TheMrMilchmann Feb 24, 2023 • edited

Choose a reason for hiding this comment

TheMrMilchmann Feb 24, 2023

Choose a reason for hiding this comment

alllex Feb 27, 2023

Choose a reason for hiding this comment

alllex commented Feb 27, 2023

bot-gradle commented Feb 27, 2023

ljacomet left a comment

Choose a reason for hiding this comment

jvandort Feb 27, 2023

Choose a reason for hiding this comment

alllex Feb 28, 2023

Choose a reason for hiding this comment

alllex commented Feb 28, 2023

bot-gradle commented Feb 28, 2023

bot-gradle commented Feb 28, 2023

alllex commented Feb 23, 2023 •

edited

TheMrMilchmann Feb 24, 2023 •

edited