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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from Feb 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6b9f039
Support non-javac toolchain tools to be used as a Java compiler
alllex Feb 23, 2023
ba332d7
Resolve java executable to absolute files
alllex Feb 23, 2023
c11d14a
Match Java homes canonically
alllex Feb 23, 2023
74bdc8a
Add up-to-date checks for the custom-compiler test
alllex Feb 27, 2023
277e222
Fix ECJ test by constraining Java versions
alllex Feb 27, 2023
7ce1d3d
Remove unnecessary assert message
alllex Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 86 additions & 0 deletions .../integTest/groovy/org/gradle/api/tasks/compile/JavaCompileToolchainIntegrationTest.groovy
View file
Open in desktop
Expand Up @@ -469,6 +469,92 @@ class JavaCompileToolchainIntegrationTest extends AbstractIntegrationSpec implem
JavaVersion.current() | "[deprecation] foo() in Foo has been deprecated"
}

@Issue("https://github.com/gradle/gradle/issues/23990")
def "can compile with a custom compiler executable"() {
def otherJdk = AvailableJavaHomes.getJdk(JavaVersion.current())
def jdk = AvailableJavaHomes.getDifferentVersion {
def v = it.languageVersion.majorVersion.toInteger()
11 <= v && v <= 18 // Java versions supported by ECJ releases used in the test
}

buildFile << """
plugins {
id("java")
}

java {
toolchain {
languageVersion = JavaLanguageVersion.of(${otherJdk.javaVersion.majorVersion})
}
}

configurations {
ecj {
canBeConsumed = false
canBeResolved = true
}
}

${mavenCentralRepository()}

dependencies {
def changed = providers.gradleProperty("changed").isPresent()
ecj(!changed ? "org.eclipse.jdt:ecj:3.31.0" : "org.eclipse.jdt:ecj:3.32.0")
}

// Make sure the provider is up-to-date only if the ECJ classpath does not change
class EcjClasspathProvider implements CommandLineArgumentProvider {
@Classpath
final FileCollection ecjClasspath

EcjClasspathProvider(FileCollection ecjClasspath) {
this.ecjClasspath = ecjClasspath
}

@Override
List<String> asArguments() {
return ["-cp", ecjClasspath.asPath, "org.eclipse.jdt.internal.compiler.batch.Main"]
}
}

compileJava {
def customJavaLauncher = javaToolchains.launcherFor {
languageVersion.set(JavaLanguageVersion.of(${jdk.javaVersion.majorVersion}))
}.get()
Comment on lines +521 to +523
Copy link
Contributor

Choose a reason for hiding this comment

The 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. 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).

Copy link
Contributor

Choose a reason for hiding this comment

The 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 JavaCompiler somehow but I believe that getting the toolchain for a tool is not currently possible.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

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


// 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(new EcjClasspathProvider(configurations.ecj))
}
"""

when:
withInstallations(jdk, otherJdk).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

// Test up-to-date checks
when:
withInstallations(jdk, otherJdk).run(":compileJava")
then:
skipped(":compileJava")

when:
withInstallations(jdk, otherJdk).run(":compileJava", "-Pchanged")
then:
executedAndNotSkipped(":compileJava")

when:
withInstallations(jdk, otherJdk).run(":compileJava", "-Pchanged")
then:
skipped(":compileJava")
}

private TestFile configureForkOptionsExecutable(Jvm jdk) {
buildFile << """
compileJava {
Expand Down
34 changes: 16 additions & 18 deletions ...a/src/main/java/org/gradle/api/internal/tasks/compile/AbstractJavaCompileSpecFactory.java
View file
Open in desktop
Expand Up @@ -42,17 +42,7 @@ public T create() {
}

if (compileOptions.isFork()) {
File customJavaHome = compileOptions.getForkOptions().getJavaHome();
if (customJavaHome != null) {
return getCommandLineSpec(Jvm.forHome(customJavaHome).getJavacExecutable());
}

String customExecutable = compileOptions.getForkOptions().getExecutable();
if (customExecutable != null) {
return getCommandLineSpec(JavaExecutableUtils.resolveExecutable(customExecutable));
}

return getForkingSpec(Jvm.current().getJavaHome());
return chooseSpecFromCompileOptions(Jvm.current().getJavaHome());
}

return getDefaultSpec();
Expand All @@ -65,13 +55,7 @@ private T chooseSpecForToolchain() {
}

if (compileOptions.isFork()) {
// Presence of the fork options means that the user has explicitly requested a command-line compiler
if (compileOptions.getForkOptions().getJavaHome() != null || compileOptions.getForkOptions().getExecutable() != null) {
// We use the toolchain path because the fork options must agree with the selected toolchain
return getCommandLineSpec(Jvm.forHome(toolchainJavaHome).getJavacExecutable());
}

return getForkingSpec(toolchainJavaHome);
return chooseSpecFromCompileOptions(toolchainJavaHome);
}

if (!toolchain.isCurrentJvm()) {
Expand All @@ -81,6 +65,20 @@ private T chooseSpecForToolchain() {
return getDefaultSpec();
}

private T chooseSpecFromCompileOptions(File fallbackJavaHome) {
File forkJavaHome = compileOptions.getForkOptions().getJavaHome();
if (forkJavaHome != null) {
return getCommandLineSpec(Jvm.forHome(forkJavaHome).getJavacExecutable());
}

String forkExecutable = compileOptions.getForkOptions().getExecutable();
if (forkExecutable != null) {
return getCommandLineSpec(JavaExecutableUtils.resolveExecutable(forkExecutable));
}

return getForkingSpec(fallbackJavaHome);
}

abstract protected T getCommandLineSpec(File executable);

abstract protected T getForkingSpec(File javaHome);
Expand Down
25 changes: 15 additions & 10 deletions subprojects/language-java/src/main/java/org/gradle/api/tasks/compile/JavaCompile.java
View file
Open in desktop
Expand Up @@ -75,8 +75,6 @@
import java.util.List;
import java.util.concurrent.Callable;

import static com.google.common.base.Preconditions.checkState;

/**
* Compiles Java source files.
*
Expand Down Expand Up @@ -258,19 +256,26 @@ private void validateForkOptionsMatchToolchain() {

JavaCompiler javaCompilerTool = getJavaCompiler().get();
File toolchainJavaHome = javaCompilerTool.getMetadata().getInstallationPath().getAsFile();
File toolchainExecutable = javaCompilerTool.getExecutablePath().getAsFile();

ForkOptions forkOptions = getOptions().getForkOptions();
File customJavaHome = forkOptions.getJavaHome();
checkState(
customJavaHome == null || customJavaHome.equals(toolchainJavaHome),
"Toolchain from `javaHome` property on `ForkOptions` does not match toolchain from `javaCompiler` property"
);
if (customJavaHome != null) {
JavaExecutableUtils.validateMatchingFiles(
customJavaHome, "Toolchain from `javaHome` property on `ForkOptions`",
toolchainJavaHome, "toolchain from `javaCompiler` property"
);
}

String customExecutablePath = forkOptions.getExecutable();
JavaExecutableUtils.validateExecutable(
customExecutablePath, "Toolchain from `executable` property on `ForkOptions`",
toolchainExecutable, "toolchain from `javaCompiler` property");
if (customExecutablePath != null) {
// We do not match the custom executable against the compiler executable from the toolchain (javac),
// because the custom executable can be set to the path of another tool in the toolchain such as a launcher (java).
File customExecutableJavaHome = JavaExecutableUtils.resolveJavaHomeOfExecutable(customExecutablePath);
JavaExecutableUtils.validateMatchingFiles(
customExecutableJavaHome, "Toolchain from `executable` property on `ForkOptions`",
toolchainJavaHome, "toolchain from `javaCompiler` property"
);
}
}

private boolean isToolchainCompatibleWithJava8() {
Expand Down
25 changes: 18 additions & 7 deletions ...cts/platform-jvm/src/main/java/org/gradle/jvm/toolchain/internal/JavaExecutableUtils.java
View file
Open in desktop
Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

The 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 forkOptions.executable. But we relax this contraint in this PR.

}

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) {
Expand Down
5 changes: 1 addition & 4 deletions ...vm/src/main/java/org/gradle/jvm/toolchain/internal/SpecificInstallationToolchainSpec.java
View file
Open in desktop
Expand Up @@ -74,10 +74,7 @@ public static SpecificInstallationToolchainSpec fromJavaHome(ObjectFactory objec
}

public static SpecificInstallationToolchainSpec fromJavaExecutable(ObjectFactory objectFactory, String executable) {
File executableFile = JavaExecutableUtils.resolveExecutable(executable);
// Relying on the layout of the toolchain distribution: <JAVA HOME>/bin/<executable>
final File parentJavaHome = executableFile.getParentFile().getParentFile();
return new SpecificInstallationToolchainSpec(objectFactory, parentJavaHome);
return new SpecificInstallationToolchainSpec(objectFactory, JavaExecutableUtils.resolveJavaHomeOfExecutable(executable));
}

@Override
Expand Down