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

Java 8 instead of 11 #117

Closed
vorburger opened this issue May 14, 2023 · 10 comments
Closed

Java 8 instead of 11 #117

vorburger opened this issue May 14, 2023 · 10 comments

Comments

@vorburger
Copy link
Owner

@mosesn raised (here) that he uses this library in an environment that is still on on Java 8 ("likely for the next year").

This project is currently targeting Java 11. The README does not explicitly state that (but it should); more imporantly the CI build is on Java here which de facto permits using Java 11 in contributions.

I'm personally very open to contributions to "downgrade" that back to Java 8 again, specifically:

  1. Rewrite the Java 9+ AtomicReference#compareAndExchange() introduced in Replace DefaultExecuteResultHandler with AtomicExecuteResultHandler #110 to Java 8
  2. Change the CI build from Java 11 to 8 (here), and add a line about it to the README
  3. Raise a PR with 2 commits for 1. & 2. and see if that passes - fix additional problems, if any

Would someone reading this like to make such a contribution? @mosesn perhaps?

@vorburger
Copy link
Owner Author

An alternative approach here, particularly interesting if some Maven plugin or something doesn't work well under Java 8 anymore for CI (on MariaDB4j there was some PITA about this; I can't remember the details), may be to keep Java 11 on CI but contribute something like the https://www.mojohaus.org/animal-sniffer/animal-sniffer-maven-plugin/ (or perhaps https://openjdk.org/jeps/247; I did not even know that existed, but stumbled upon it from the link on https://github.com/mojohaus/animal-sniffer).

@vorburger
Copy link
Owner Author

There is also a bit of an inconsistency with <maven.compiler.source> and <maven.compiler.target> in pom.xml set to 1.8 but CI on Java 11; this should be aligned (and have a cross-reference inline comment in both places).

@vorburger
Copy link
Owner Author

if some Maven plugin or something doesn't work well under Java 8 anymore

Actually, if it isn't already, this will become the case with the merge of #119, note https://errorprone.info/docs/installation stating that the current ErrorProne version 2.19.1 > 2.10.0 was the latest version to support running on JDK 8.

So the way forward here is actually NOT to change the change the CI build from Java 11 to 8 (here), but instead to add <release>8</release> (after <source> and <target> in the pom.xml) - after a PR which changes the AtomicExecuteResultHandler.

@vorburger
Copy link
Owner Author

I just learnt that the following ugly warning which I never looked more into:

[INFO] --- compiler:3.11.0:compile (default-compile) @ exec ---
[INFO] Changes detected - recompiling the module! :source
[INFO] Compiling 21 source files with javac [debug target 1.8] to target/classes
[INFO] -------------------------------------------------------------
[WARNING] COMPILATION WARNING :
[INFO] -------------------------------------------------------------
[WARNING] bootstrap class path not set in conjunction with -source 8
[INFO] 1 warning

is actually related to this topic. To fix this, in order to be able to do -Werror for #119, I am going to set it to 11 as that's what it currently is - but again I'm very open to a PR e.g. from @mosesn which reduces that to 8 again.

@vorburger
Copy link
Owner Author

vorburger commented May 14, 2023

https://errorprone.info/bugpattern/Java8ApiChecker does something similar as well. I'm going to use -Xep:Java8ApiChecker:OFF in #119 for now, but this could be removed again in a contributor PR.

@mosesn
Copy link
Contributor

mosesn commented May 14, 2023

I'll discuss with my team, but I think what's more likely is that we'll focus our energy on getting to JDK11+ so we don't have to keep worrying about this issue!

@RenatBl
Copy link

RenatBl commented May 14, 2023

@vorburger Hello! Do you mind if I contribute to this issue? I have some idea how this can be implemented

@vorburger
Copy link
Owner Author

vorburger commented May 14, 2023

@RenatBl hello, welcome to the project, and thank you very much for your interest!

FYI I just merged #125. Can you think of anything else that may be required here?

From my side, I think this is actually done with that, and I was thinking of closing this.

@RenatBl
Copy link

RenatBl commented May 14, 2023

@vorburger Oh, yes, I see that you have already made changes to this issue, my thought was to add a utility method that repeats the functionality of AtomicReference.compareAndExchange based on methods supported by Java 8. I thought the main problem was calling this method in the AtomicExecuteResultHandler class

@vorburger
Copy link
Owner Author

@RenatBl we (with @mosesn) FYI just got rid of that AtomicReference.compareAndExchange in #121. (And in #126 we're discussing more related changes which may interest you?) Let me close this as Done. Please comment if you / anyone else reading along can hink of anything else that may be required here re. supported Java version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants