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
Restore an edge case of non-javac toolchain tools being used as a Java compiler #24014
Changes from 3 commits
6b9f039
ba332d7
c11d14a
74bdc8a
277e222
7ce1d3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -469,6 +469,55 @@ class JavaCompileToolchainIntegrationTest extends AbstractIntegrationSpec implem | |||||||||
JavaVersion.current() | "[deprecation] foo() in Foo has been deprecated" | ||||||||||
} | ||||||||||
|
||||||||||
@Issue("https://github.com/gradle/gradle/issues/23990") | ||||||||||
@Requires(TestPrecondition.JDK11_OR_LATER) | ||||||||||
def "can compile with a custom compiler executable"() { | ||||||||||
def jdk = AvailableJavaHomes.getJdk(JavaVersion.current()) | ||||||||||
|
||||||||||
buildFile << """ | ||||||||||
plugins { | ||||||||||
id("java") | ||||||||||
} | ||||||||||
|
||||||||||
configurations { | ||||||||||
ecj { | ||||||||||
canBeConsumed = false | ||||||||||
canBeResolved = true | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
${mavenCentralRepository()} | ||||||||||
|
||||||||||
dependencies { | ||||||||||
ecj("org.eclipse.jdt:ecj:3.32.0") | ||||||||||
} | ||||||||||
|
||||||||||
compileJava { | ||||||||||
def customJavaLauncher = javaToolchains.launcherFor { | ||||||||||
languageVersion.set(JavaLanguageVersion.of(${jdk.javaVersion.majorVersion})) | ||||||||||
}.get() | ||||||||||
Comment on lines
+521
to
+523
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume that this would still fail if a different default toolchain is configured at project level. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this could be worked around by retrieving the launcher for the toolchain of the configured There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It should not fail. Since we are configuring gradle/subprojects/language-java/src/main/java/org/gradle/api/tasks/compile/JavaCompile.java Lines 105 to 108 in c11d14a
I'll update the test to show this |
||||||||||
|
||||||||||
// ECJ does not support generating JNI headers | ||||||||||
options.headerOutputDirectory.set(provider { null }) | ||||||||||
options.fork = true | ||||||||||
options.forkOptions.executable = customJavaLauncher.executablePath.asFile.absolutePath | ||||||||||
|
||||||||||
options.forkOptions.jvmArgumentProviders.add({ | ||||||||||
wolfs marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
["-cp", configurations.ecj.asPath, "org.eclipse.jdt.internal.compiler.batch.Main"] | ||||||||||
} as CommandLineArgumentProvider) | ||||||||||
} | ||||||||||
""" | ||||||||||
|
||||||||||
when: | ||||||||||
withInstallations(jdk).run(":compileJava", "--info") | ||||||||||
|
||||||||||
then: | ||||||||||
executedAndNotSkipped(":compileJava") | ||||||||||
outputContains("Compiling with toolchain '${jdk.javaHome.absolutePath}'") | ||||||||||
outputContains("Compiling with Java command line compiler '${jdk.javaExecutable.absolutePath}'") | ||||||||||
classJavaVersion(javaClassFile("Foo.class")) == jdk.javaVersion | ||||||||||
} | ||||||||||
|
||||||||||
private TestFile configureForkOptionsExecutable(Jvm jdk) { | ||||||||||
buildFile << """ | ||||||||||
compileJava { | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,32 +39,43 @@ public static File resolveExecutable(String executable) { | |
.undocumented() | ||
.nagUser(); | ||
} | ||
if (!executableFile.getAbsoluteFile().exists()) { | ||
File executableAbsoluteFile = executableFile.getAbsoluteFile(); | ||
if (!executableAbsoluteFile.exists()) { | ||
throw new InvalidUserDataException("The configured executable does not exist (" + executableFile.getAbsolutePath() + ")"); | ||
} | ||
if (executableFile.getAbsoluteFile().isDirectory()) { | ||
if (executableAbsoluteFile.isDirectory()) { | ||
throw new InvalidUserDataException("The configured executable is a directory (" + executableFile.getAbsolutePath() + ")"); | ||
} | ||
return executableFile; | ||
return executableAbsoluteFile; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
public static File resolveJavaHomeOfExecutable(String executable) { | ||
// Relying on the layout of the toolchain distribution: <JAVA HOME>/bin/<executable> | ||
return resolveExecutable(executable).getParentFile().getParentFile(); | ||
} | ||
|
||
|
||
public static void validateExecutable(@Nullable String executable, String executableDescription, File referenceFile, String referenceDescription) { | ||
if (executable == null) { | ||
return; | ||
} | ||
|
||
File executableFile = resolveExecutable(executable); | ||
if (executableFile.equals(referenceFile)) { | ||
validateMatchingFiles(executableFile, executableDescription, referenceFile, referenceDescription); | ||
} | ||
|
||
public static void validateMatchingFiles(File customFile, String customDescription, File referenceFile, String referenceDescription) { | ||
if (customFile.equals(referenceFile)) { | ||
return; | ||
} | ||
|
||
File canonicalExecutableFile = canonicalFile(executableFile); | ||
File canonicalCustomFile = canonicalFile(customFile); | ||
File canonicalReferenceFile = canonicalFile(referenceFile); | ||
if (canonicalExecutableFile.equals(canonicalReferenceFile)) { | ||
if (canonicalCustomFile.equals(canonicalReferenceFile)) { | ||
return; | ||
} | ||
|
||
throw new IllegalStateException(executableDescription + " does not match " + referenceDescription + "."); | ||
throw new IllegalStateException(customDescription + " does not match " + referenceDescription + "."); | ||
} | ||
|
||
private static File canonicalFile(File file) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't assertions typically written with a
:
instead of comma, at least in Java? Does the comma mean something different here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!