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

Open com.sun.tools.javac modules on JDK 16+ #1224

Merged
merged 10 commits into from
Jun 5, 2022

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Jun 1, 2022

This is an attempt to fix #834.

The solution is pointed out from google/google-java-format#612 (comment).

I'm new to this community and I'm glad to know how to move forward this effort.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 2, 2022

cc @nedtwigg

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

This needs an entry in each of the three CHANGES.md (root and plugin-maven/plugin-gradle).

If I understand correctly, the intent of this change is that users won't have to add the --add-opens commands most of the time? Have you been able to confirm this on your machine in an integration test?

lib/src/main/java/com/diffplug/spotless/Jvm.java Outdated Show resolved Hide resolved
lib/src/main/java/com/diffplug/spotless/ModuleHelper.java Outdated Show resolved Hide resolved
Signed-off-by: tison <wander4096@gmail.com>
…levant

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 2, 2022

@nedtwigg comments addressed. I'd like to know how to apply a local plugin jar so that I can verify with the SNAPSHOT on my projects - I'm working on this issue because I don't want my project contains a JDK16+ workaround gradle.properties file, which will fail JDK8 build.

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 2, 2022

OK. I've checked that this patch work for my project by publishToMavenLocal and running spotlessCheck with JDK17 without the workaround.

I'm glad to add an integration test if I learn how to. Or you may verify by yourself since I think if we workaround a JDK limitation, it should be straightforward and it's hard to me to construct such a test.

@tisonkun tisonkun requested a review from nedtwigg June 2, 2022 16:17
@nedtwigg nedtwigg enabled auto-merge June 5, 2022 16:00
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Thanks!

@nedtwigg nedtwigg merged commit 2cb9043 into diffplug:main Jun 5, 2022
@nedtwigg
Copy link
Member

nedtwigg commented Jun 5, 2022

Released in plugin-gradle 6.7.0 and plugin-maven 2.22.6.

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 5, 2022

@nedtwigg sorry that I miss a local commit that we should get unsafe from reflection otherwise it will throw exceptions.

@catostrophe
Copy link

Does it fix the same issue with palantir-java-format?

@nedtwigg
Copy link
Member

Re: palantir-java-format

  • Not fixed in plugin-gradle 6.7.1 and plugin-maven 2.22.7.
  • Yes fixed in plugin-gradle 6.7.2 and plugin-maven 2.22.8

@davidburstrom
Copy link
Contributor

@nedtwigg This fix has an interesting side-effect that can cause apparent flakiness: Since the modules are opened for the VM as a whole, this means that any other plugin that runs GJF in the same VM piggy-backs on this solution, iff Spotless gets to run first. I've just been scratching my head as to why one invocation of Gradle works but not the other. It could be worth looking into closing the modules after use.

@tisonkun
Copy link
Contributor Author

tisonkun commented Mar 1, 2023

@davidburstrom Can you provide a reproduce here? I'm curious how an extra opened module hurts?

@taofik69
Copy link

taofik69 commented Mar 1, 2023 via email

@davidburstrom
Copy link
Contributor

@tisonkun I don't have a public reproducer at hand, but consider this:

You have a custom Task that invokes GJF, for the sake of it called myGJF.
You have Spotless applied with GJF, which creates a Task called spotlessJavaApply.
With a new Gradle daemon, you execute ./gradlew spotlessJavaApply and then ./gradlew myGJF, which succeeds.
After stopping the Gradle daemon, you execute ./gradlew myGJF which fails, because the workaround in the PR hasn't been applied in the VM yet.

It hurts because hermeticity and reproducibility has been broken.

@tisonkun
Copy link
Contributor Author

tisonkun commented Mar 1, 2023

@davidburstrom I get your point now. Basically, I ever thought of just fixing the issue in GJF upstream and all downstreams need not to do any trick. But it seems the upstream is relatively inactive so I fix my case here.

You may try to pick related changes to the upstream and remove this trick :)

@taofik69
Copy link

taofik69 commented Mar 1, 2023

Thank's boss

@davidburstrom
Copy link
Contributor

Technically speaking, it would be the same issue upstream too: Any other library X that require the same compiler modules to be exported would work if GJF has been executed in the same VM and vice versa. It'd be better if the open was matched with a corresponding close when GJF is done, which obviously requires reference counting and would still be susceptible to race conditions (the library X would work only when GJF is executing).

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.

google-java-format (and removeUnusedImports) broken on JDK 16+ (has workaround)
5 participants