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

docs(jdk): recommend JDK 11. JDK18 will not work. JDK17 may not work < 0.68 #3178

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

mikehardy
Copy link
Contributor

I get reports of build errors from users that have chosen JDK18, that JDK does not work yet and should not be included either explicitly or even as part of an acceptable range ("...or newer")

See react-native-device-info/react-native-device-info#1431 (comment)

I will add a second commit here in a moment for linux setup

@mikehardy
Copy link
Contributor Author

I chose 14 as the max because I thought it was a well-maintained release but as I consult the version table it appears that 17 is an LTS, and currently 17 is supported. I'll modify it to recommend 11 or 17 as those are LTS versions so should receive updates through system package managers and thus be reasonable choices

https://en.wikipedia.org/wiki/Java_version_history

@netlify
Copy link

netlify bot commented Jun 24, 2022

Deploy Preview for react-native ready!

Name Link
🔨 Latest commit baed289
🔍 Latest deploy log https://app.netlify.com/sites/react-native/deploys/62b5d82ec6910e00090917cd
😎 Deploy Preview https://deploy-preview-3178--react-native.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Jun 24, 2022

Deploy Preview for react-native ready!

Name Link
🔨 Latest commit e500a78
🔍 Latest deploy log https://app.netlify.com/sites/react-native/deploys/62bc5719a586e80008353467
😎 Deploy Preview https://deploy-preview-3178--react-native.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mikehardy mikehardy changed the title docs(jdk): specify JDK11 or 14. JDK18 will not work docs(jdk): recommend LTS JDKs 11 or 17. JDK18 will not work Jun 24, 2022
Copy link
Contributor

@leotm leotm left a comment

Choose a reason for hiding this comment

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

thx for picking this up, going w my spidey tingles in future to more proactively suggest/pr updates

@@ -28,7 +28,7 @@ brew install --cask zulu11

The Zulu OpenJDK distribution offers JDKs for **both Intel and M1 Macs**. This will make sure your builds are faster on M1 Macs compared to using an Intel-based JDK.

If you have already installed JDK on your system, make sure it is JDK 11 or newer.
If you have already installed JDK on your system, we recommend either of the two currently supported LTS versions, JDK 11 or JDK 17. JDK 18 is not supported yet.
Copy link
Contributor

Choose a reason for hiding this comment

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

JDK 18 is not supported yet.

just chiming in agree on this, not only was it causing 0.69.0-rc.6 java.lang.OutOfMemoryError debug-mode earlier

Copy link
Contributor

Choose a reason for hiding this comment

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

but also separately (my template) slightly (unsupportedly 😅) ahead on Gradle/AGP 7.4.2
debug/release both build fine, debug-mode runs fine, but release has similar runtime error i'm debugging atm

Copy link
Contributor

Choose a reason for hiding this comment

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

@cortinico
Copy link
Contributor

I think we should stringly enforce Java 11 for now. I haven't got the time to look into this specifically, but I believe JDK 17 won't work at all (for the same reason JDK 18 won't work)

@mikehardy
Copy link
Contributor Author

I have specific user feedback that JDK17 is working...at least mostly? -

react-native-device-info/react-native-device-info#1431 (comment)

very good and best !- yes all but some projets were working in JDK17

Let me check

@mikehardy
Copy link
Contributor Author

My react-native-firebase / react-native build qualifier has no trouble with JDK17 🤷 - https://github.com/mikehardy/rnfbdemo/blob/main/make-demo.sh - used brew install --cask temurin-17 and verified PATH etc. Built it debug and release mode, hermes enabled etc.

It is always possible I made a mistake but it appears to work fine, it is an LTS, and another user confirmed it worked as well.

So TL;DR: @cortinico I think JDK17 is good actually, an unexpected result on my part to be honest but happy about it since it's now the most current LTS.

@cortinico
Copy link
Contributor

Built it debug and release mode, hermes enabled etc.

Have you tried to run it also?

@mikehardy
Copy link
Contributor Author

@cortinico

Have you tried to run it also?

Of course! :-) I also just ran my main work project (which has a large pile of dependencies, not all current) and it seemed fine. JDK17 seems fine, with success reports from template and in work projects and no failure reports. Absence of proof (of failure) is not proof of absence (of failure) but with zero reports of failure and success in all the things I normally test I'm not sure how else to qualify it, it seems good to me

@leotm
Copy link
Contributor

leotm commented Jun 25, 2022

my results btw from earlier

Capture

@cortinico
Copy link
Contributor

my results btw from earlier

Thanks @leotm This clarifies a lot. I'm wondering if before we update this doc, I'd like to investigate what is the underlying issue with JDK 18 so we can just recommend to use any JDK >= 11

@mikehardy
Copy link
Contributor Author

Having an open upper end is always dangerous IMHO
Here's the underlying JDK18 issue (it was JDK16 and I guess 17 too, but now it's JDK18?) https://issuetracker.google.com/issues/186806275

@mikehardy
Copy link
Contributor Author

Feedback from previous user that prompted this PR for me is that JDK17 will work with RN68ish but rn67 is problematic for JDK17.

if (big if? is it?) the docs are for new installs, then that tells me current state of PR (which recommends 11-17) is okay. If we want the docs to also be backwards compatible then it should be "use 11"

I continue to assert that open-ended upper range is dangerous as current android studio tooling is not compatible with JDK18

react-native-device-info/react-native-device-info#1431 (comment)

That sounds all concrete and like an unmove-able stance but really I'm just trying to work with the data I've got and be helpful. Hopefully that comes through. Cheers

@cortinico
Copy link
Contributor

Having an open upper end is always dangerous IMHO

Agree. Yet having to test the whole range is also a pain.
Also JDK 18 is soon to be superseeded by JDK 19 (18 was a short term support version).
I understand that users wants to update every tool they install, but we should probably pay more attention to the doctor command and make sure the required tools are aligned.

Feedback from previous user that prompted this PR for me is that JDK17 will work with RN68ish but rn67 is problematic for JDK17.

Why was it problematic for RN67?

@mikehardy
Copy link
Contributor Author

Why was it problematic for RN67?

Unsure exactly, it was logged in the google issue tracker which smells (warning: assumption ahead) like a gradle version compatibility thing between gradle / android gradle plugin. Perhaps it couldn't handle the new JDK17 ASTs or something I dunno

I understand that users wants to update every tool they install, but we should probably pay more attention to the doctor command and make sure the required tools are aligned.

+1 on that! You know I've got a stack of doctor enhancements I'd like to work through but then firebase-ios-sdk threw the use_frameworks! bomb at my available time 😅 . I did think the docs update would help as something quick though.

So Perhaps just say "JDK11" and stick with it, sometimes being opinionated and simple is better than complicated. I don't feel strongly but I like fewer issues in my repo. If saying "JDK11" and done works here, that works for me

@mikehardy
Copy link
Contributor Author

Wow, apparently using JDK18 can result in ABI breaks in distributed apps on Android 8/8.1 - invertase/react-native-firebase#6338 - so, not sure what to do about that but I definitely would not include JDK18 in any range we recommend at the moment

- JDK17 seems to work with > 0.68+ but may have problems < 0.68
- JDK18 has known problems with all versions at the moment
@mikehardy
Copy link
Contributor Author

That caused me to come look at this again - @cortinico I see you ❤️ed my comment about just saying "JDK11" so I altered all suggestions to just say 11 and warn about incompatibilities in higher versions with more specifics in the commit message

I squashed all the commits to a single one and rebased against current main here

should be good?

@mikehardy mikehardy changed the title docs(jdk): recommend LTS JDKs 11 or 17. JDK18 will not work docs(jdk): recommend JDK 11. JDK18 will not work. JDK17 may not work < 0.68 Jun 29, 2022
@cortinico
Copy link
Contributor

should be good?

Yes that's good. We can merge this.
FYI @mikehardy @leotm, I've opened this for the sake of visiblity:

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

Successfully merging this pull request may close these issues.

4 participants