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

Quick fix for spotless #324

Merged
merged 3 commits into from
May 24, 2021
Merged

Quick fix for spotless #324

merged 3 commits into from
May 24, 2021

Conversation

rnett
Copy link
Contributor

@rnett rnett commented May 23, 2021

Removes .mvn/jvm.config since the --add-exports doesn't work on JDK 8. Build won't work on JDK 16 unless you add the noted exports yourself (they are noted in CONTRIBUTING.md). Alternatively, we could use 11 in the CI and rely on profiles to set the target.

Signed-off-by: Ryan Nett <JNett96@gmail.com>
@karllessard
Copy link
Collaborator

karllessard commented May 23, 2021

Please @rnett , can you remove the reformatting changes in the CONTRIBUTING.md of your PR? Thanks

Also since it won't work with JDK16+, I would protect the execution of the spotless plugin only for the lower versions of the JVM, similar to what I did for error-prone, so users compiling on it won't face issues.

We will eventually jump the CI build to use 11 but it is not for now yet.

Signed-off-by: Ryan Nett <JNett96@gmail.com>
@rnett
Copy link
Contributor Author

rnett commented May 23, 2021

Should be fixed.

Also since it won't work with JDK16+, I would protect the execution of the spotless plugin only for the lower versions of the JVM, similar to what I did for error-prone, so users compiling on it won't face issues.

That's what I did for now. Unfortunately, there's no way to do it via profiles since the args need to be set for the maven JVM, but it's noted so you'll just have to add it yourself on 16.

@karllessard
Copy link
Collaborator

But right now the plugin is executed no matter which JDK you use, right? So building on JDK 16 will fail, unless you add the exports manually?

If so, my point is that it would be better to prevent the plugin to be executed on 16+ using Maven profile activation based on the JDK version, at least for now. This plugin is just a nice-to-have anyway and should not block our users

Signed-off-by: Ryan Nett <JNett96@gmail.com>
@rnett
Copy link
Contributor Author

rnett commented May 23, 2021

Ah, I see. It won't auto-run on JDK 16+ unless the format profile is enabled, but the plugin tasks will still be there, I'm assuming that if you're running the tasks manually you know to add the exports (or will when you get the error message).

@karllessard karllessard merged commit 0f7274e into tensorflow:master May 24, 2021
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.

2 participants