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

remove google java formatter dependency #11765

Closed
wants to merge 3 commits into from

Conversation

jsvd
Copy link
Member

@jsvd jsvd commented Apr 8, 2020

No description provided.

@jsvd jsvd marked this pull request as ready for review April 8, 2020 11:16
@jsvd
Copy link
Member Author

jsvd commented Apr 13, 2020

@colinsurprenant you've been around the compilation code recently, what do you think here about the need of this dependency? if this is being used to avoid cache invalidation, in what scenarios does it do that?

This dependency was introduced in #8913

@jsvd jsvd requested a review from colinsurprenant April 15, 2020 08:44
@colinsurprenant
Copy link
Contributor

Good question, will investigate.

@colinsurprenant colinsurprenant self-assigned this Apr 28, 2020
@colinsurprenant
Copy link
Contributor

@jsvd This is used to do generated source code formatting and in that respect insures that 2 equivalent code generation will yield the same text+formatting. I see that the note about licensing :

WARNING: DO NOT UPGRADE "google-java-format"
later versions require GPL licensed code in javac-shaded that is
Apache2 incompatible

I just verified that and starting at v1.8 they removed that GPL dependency we should be good to upgrade to latest v1.9 without incompatible license dependency.

I think we can close this and create a new PR to upgrade google-java-format.

@colinsurprenant
Copy link
Contributor

Created #12223 to upgrade to latest versions.

@jsvd
Copy link
Member Author

jsvd commented Sep 3, 2020

@colinsurprenant I believe that version only works with Java >= 9, which is why we didn’t upgrade

@colinsurprenant
Copy link
Contributor

Good catch. I can't find clear indications for the Java version requirements but looking at https://github.com/google/google-java-format/releases all I see is that v1.8 requires JDK 11.

Short of upgrading the library because of minimum JDK requirements we can either keep the current version or remove it altogether as you suggest here but by doing so we also will loose readable formatting on the generated sources which is useful when developing/debugging. I don't think that removing the formatter will break caching but I don't know for sure.

@roaksoax roaksoax assigned jsvd and unassigned colinsurprenant Jan 19, 2022
@andsel
Copy link
Contributor

andsel commented Feb 7, 2022

Here we should upgrade to version 1.13.

Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

Left a comment to update the version

// WARNING: DO NOT UPGRADE "google-java-format"
// later versions require GPL licensed code in javac-shaded that is
// Apache2 incompatible
implementation('com.google.googlejavaformat:google-java-format:1.1') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that it's use for code formatting in debug mode of Janino, we should keep it and update to version 1.13 which still require Java 11 and is In Apache License

@jsvd
Copy link
Member Author

jsvd commented Feb 7, 2022

Closing this in favor of #13700

@jsvd jsvd closed this Feb 7, 2022
@jsvd jsvd deleted the remove_google_java_formatter branch February 7, 2022 11:06
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.

5 participants