run formatter in fork - run on java 16+ without --add-exports flags #140
Conversation
|
In the light of #141, do you want to make any changes to this @danielnorberg ? |
|
Also, suggest we add a config to enable forking, setting it to false by default (and possibly to true for JDK 16+ ?) |
|
Yeah, I think the way this sets up the classpath when forking will not actually work when running as a maven plugin. |
|
I think I've now managed to resolve the classpath problem, but would want integration tests that run the plugin as a real maven plugin. |
|
Fwiw I've tested a snapshot build of this PR locally and it seems to work as expected. |
|
fwiw the integration tests pass locally with corretto 11 & 17 (x86) when rebasing this on #145 |
| final ProcessBuilder processBuilder = | ||
| new ProcessBuilder(java.toString(), "-cp", classPathArg).directory(workdir.toFile()); | ||
|
|
||
| // Propagate -Xmx and -D. |
There was a problem hiding this comment.
Good question. That’s a remnant from the flo use case. If this was invoking google-java-format as a cli then I’d be naively leaning towards not propagating any flags at all.
But maybe it’d be useful to propagate all flags? Not sure 🤔
There was a problem hiding this comment.
Propagating Xmx as-is might even be problematic if it’s high.
There was a problem hiding this comment.
I wonder what surefire etc does.
There was a problem hiding this comment.
The surefire docs are confusing on this point but having stepped through the surefire execution of forks with a debugger and having inspected the precise command line and serialized property files etc I feel confident in saying that surefire does not automatically propagate any properties, not even -Xmx etc. One has to explicitly allow-list specific properties to propagate.
Maybe we should consider conservatively not propagating properties here unless we see a specific need for it in google-java-format.
|
Perhaps we should also test this on windows. Does GitHub actions support that? |
Have no idea, but hopefully? https://blog.devgenius.io/write-your-github-actions-workflow-for-build-windows-application-94e5a989f477 |
|
Re windows, here goes nothing #146 |
Maven plugin dependencies are loaded using a custom classloader and so
the classpath necessary for forking must be resolved by querying maven
for the plugin dependency artifacts, which can be done using the
`${plugin.pluginArtifactMap}` property.
|
It blows my mind a bit that this just works on windows on the first try. Java is amazing. |
Remove the need to add a
.mvn/jvm.configwith--add-exportsflags when using Java 16+This is achieved by running the formatter in a java subprocess with the necessary
--add-exportsspecified. The forking logic was adapted from https://github.com/spotify/flo/blob/master/flo-runner%2Fsrc%2Fmain%2Fjava%2Fcom%2Fspotify%2Fflo%2Fcontext%2FForkingExecutor.java. We could also consider putting the forking logic in a separate library for easier reuse. Presumably we might encounter the same problem again.Alternative to #139