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

Work around reflective access errors on JVM 16+ #634

Merged
merged 3 commits into from
Mar 3, 2023

Conversation

tadfisher
Copy link
Contributor

Fixes #629


// Work around ktlint triggering reflective access errors from the embedded Kotlin
// compiler. See https://youtrack.jetbrains.com/issue/KT-43704 for details.
if (JavaVersion.current() >= JavaVersion.VERSION_16) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not that familiar with the Worker API yet. are we sure this is the correct way to check the java versions?
ie. is it always inherited from the java version of the gradle process? or is there a toolchain/fork option we should be looking at?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I don't think that, at this point, we are executing in the worker process. At this point we are still in the Gradle process, so it will be the JVM version of that process/classloader.

The workers are running in their own process/classloader, and static variables are unique-per-classloader hierarchy IIRC. As such, you can safely assume that any JavaVersion.current() calls will be accurate within the JVM classloader they are executing inside of.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at Gradle's exec code, the default JavaForkOptions is set up here:

@Override
public JavaForkOptionsInternal newDecoratedJavaForkOptions() {
    JavaDebugOptions javaDebugOptions = objectFactory.newInstance(DefaultJavaDebugOptions.class, objectFactory);
    final DefaultJavaForkOptions forkOptions = instantiator.newInstance(DefaultJavaForkOptions.class, fileResolver, fileCollectionFactory, javaDebugOptions);
    forkOptions.setExecutable(Jvm.current().getJavaExecutable());
    return forkOptions;
}

So by default, Gradle appears to set the java executable for the child process to the same executable as the Gradle daemon. To override this, I believe we would need to call setExecutable ourselves with a path to some other Java toolchain's javac. Since we don't do that, JavaVersion.current() should be accurate for both the Gradle daemon process and the forked worker process.

@wakingrufus
Copy link
Collaborator

As for testing, if this does fix #629 can you increase the maximum gradle version to the latest 8.x?
If not, this seems like a worthwhile fix, even if it is not the only thing needed for gradle 8 support

@tadfisher
Copy link
Contributor Author

Sure. I've been using this patch on Gradle 8, and I haven't seen any other showstoppers for Gradle 8 support.

@IvanPizhenko
Copy link

@wakingrufus @JLLeitschuh Hi, is there a chance for this to be merged and released anytime soon? We are also using ktlint and suffering from this issue.

@IvanPizhenko IvanPizhenko mentioned this pull request Feb 28, 2023
@JLLeitschuh
Copy link
Owner

CI currently fails for this PR which is currently a non-starter for merging this PR at this moment

@tadfisher
Copy link
Contributor Author

@JLLeitschuh Looks like CI is good to go. Let me know if the change to TestVersions.maxSupportedKotlinVersion makes sense; these are the versions listed in the Kotlin Gradle plugin documentation here. Without that change, our minimum Gradle version (currently 6.8) will fail with Kotlin 1.8.10, which requires Gradle 6.8.3.

@wakingrufus wakingrufus self-requested a review March 1, 2023 23:48
@JLLeitschuh
Copy link
Owner

I'd like to wait for this if your fine with that

#642

@wakingrufus
Copy link
Collaborator

LGTM. ill merge my other PR, then we can do this one

@JLLeitschuh
Copy link
Owner

Just rebased the changes

@JLLeitschuh JLLeitschuh merged commit 01df7fe into JLLeitschuh:main Mar 3, 2023
@JLLeitschuh
Copy link
Owner

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gradle 8 support
6 participants