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

Upgrade Google Java Format 1.14.0 -> 1.15.0 #141

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Upgrade Google Java Format 1.14.0 -> 1.15.0 #141

merged 1 commit into from
Mar 29, 2022

Conversation

rickie
Copy link
Contributor

@rickie rickie commented Mar 8, 2022

rickie
rickie commented Mar 8, 2022
View reviewed changes
README.md Outdated
@@ -199,14 +199,6 @@ You can pass parameters via standard `-D` syntax.

`-Dfmt.skip` is whether the plugin should skip the operation.

### Using with Java 16+ and Maven

Since the JDK is more restrictive since version 16 you need to pass some [parameters](https://github.com/google/google-java-format#jdk-16) to the JVM to run the Google Java Formatter. To do so add the file `.mvn/jvm.config` under your project's root directory with the following contents:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section is also removed upstream: google/google-java-format@13ca73e.

@klaraward
Copy link
Contributor

Seems this (and in particular google/google-java-format#759) might make #140 less necessary
Great news 👍

klaraward
klaraward reviewed Mar 8, 2022
View reviewed changes
pom.xml Outdated
@@ -221,7 +221,7 @@
<jdk>[17,)</jdk>
</activation>
<properties>
<!-- TODO: Remove this once google-java-fmt supports 17 or the plugin supports forking and run with these args built-in -->
<!-- TODO: Remove this once google-java-fmt supports 17 or the plugin supports forking -->
<argLine> --add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</argLine>
Copy link
Contributor

Choose a reason for hiding this comment

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

If the fix in google-java-format works, it should be possible to remove these flags right?

@klaraward
Copy link
Contributor

I would say that the Add-Exports sections added to the manifest of the google-java-format does not have any effect in this plugin since we don't launch the jar.
We most likely still need to fork the plugin to be able to add the JVM args, even if we add the same args to our manifest, the plugin is run as part of the maven process and will not pick them up.

@klaraward klaraward mentioned this pull request Mar 8, 2022
Merged
@rickie rickie force-pushed the rossendrijver/upgrade_gjf branch from 67de601 to 4cf984e Compare March 14, 2022 15:43
@rickie
Copy link
Contributor Author

rickie commented Mar 14, 2022

Rebased the PR.

@klaraward
Copy link
Contributor

This wouldn't really have any effects for the plugin I believe? Can still merge ofc.

@rickie
Copy link
Contributor Author

rickie commented Mar 15, 2022

Yes you are right. Fine with both merging and closing the PR.

@klaraward
Copy link
Contributor

It's good to be on the latest version after all :)

@klaraward klaraward merged commit 595bf5a into spotify:main Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klaraward klaraward klaraward approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rickie @klaraward