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

Bump room to 2.4.2, allowing arm64 jdk compile #773

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

danilobuerger
Copy link
Contributor

Summary

Starting with room 2.4.0-alpha03 arm64 is supported allowing native compile with a arm64 jdk.

Test Plan

  1. Get yourself an m1
  2. brew tap mdogan/zulu
  3. brew install --cask zulu-jdk11
  4. java -version (make sure you are running zulu)
  5. any repo with rn and async-storage included
  6. react-native run android (build will fail)
  7. apply patch
  8. react-native run android (build will succeed)

@danilobuerger
Copy link
Contributor Author

I don't really know why it would still include jetified-kotlin-reflect-1.5.31.jar in the test. Maybe a cache problem?

@tido64
Copy link
Member

tido64 commented Mar 17, 2022

I don't really know why it would still include jetified-kotlin-reflect-1.5.31.jar in the test. Maybe a cache problem?

I think this is because our test app is still on 1.5. I'm looking into bumping to 1.6 now:

@tido64
Copy link
Member

tido64 commented Mar 17, 2022

A new version of the test app with Kotlin 1.6.10 is out. Can you try bumping react-native-test-app to 1.2.1?

"react-native-test-app": "^1.2.0",

@danilobuerger
Copy link
Contributor Author

Bumped and test passes now. Thanks for the help

@tido64
Copy link
Member

tido64 commented Mar 18, 2022

@krizzu: Can you please review this? I think you had some objections to bumping Room in the past, but it currently looks like it will break on react-native 0.68 + M1.

Also, I can't test this because step 1 fails for me 😛

@danilobuerger
Copy link
Contributor Author

Side note: It will only fail on m1 if using an arm64 jdk.

@krizzu
Copy link
Member

krizzu commented Mar 18, 2022

@tido64 Great, I hoped to get an Kotlin update in test-app soon 🙏

@danilobuerger This looks great. My only concern in bumping Room, was because the 2.4.0+ requires compileSdk 31 and I did not want to force anyone (even if it's recommended to use latest one). That's why I gave an option to set Room version through properties.

But I think it might be good to go up now, especially with RN 0.68 💪

@krizzu
Copy link
Member

krizzu commented Mar 18, 2022

Btw, @danilobuerger , any particular reason for Zulu dist? Or it's just personal preference?

Starting with room 2.4.0-alpha03 arm64 is supported allowing native compile with a arm64 jdk.
@danilobuerger
Copy link
Contributor Author

danilobuerger commented Mar 18, 2022

Btw, @danilobuerger , any particular reason for Zulu dist? Or it's just personal preference?

@krizzu Just to get my hands on an arm64 jdk 11 build. For example temurin 11 does not have an arm64 build yet.

Copy link
Member

@krizzu krizzu left a comment

Choose a reason for hiding this comment

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

This is great, thank you

@krizzu
Copy link
Member

krizzu commented Mar 18, 2022

Btw, @danilobuerger , any particular reason for Zulu dist? Or it's just personal preference?

@krizzu Just to get my hands on an arm64 jdk 11 build. For example temurin 11 does not have an arm64 build yet.

Make sense, I see temurin has arm64 available for jdk17 only. Thanks!

@danilobuerger
Copy link
Contributor Author

adoptium/adoptium#96 for reference on temurin 11 arm64

@krizzu
Copy link
Member

krizzu commented Mar 18, 2022

@tido64 If you don't have anything else, feel free to merge

@tido64
Copy link
Member

tido64 commented Mar 18, 2022

@tido64 If you don't have anything else, feel free to merge

Sure, only question I have is whether this should be a minor or patch bump. It feels like it should be minor since we bumped the SDK. What do you think?

@krizzu
Copy link
Member

krizzu commented Mar 18, 2022

@tido64 Yes, I also think this is minor. I'll add annotation to release notes, that we're using higher version of SDKs and require compileSdk 31

Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to investigate and fix this ❤️

@tido64 tido64 merged commit 774fb78 into react-native-async-storage:master Mar 18, 2022
@krizzu
Copy link
Member

krizzu commented Mar 18, 2022

🎉 This PR is included in version 1.17.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mikehardy
Copy link
Contributor

I mentioned this to @tido64 on discord - but in the release notes you should definitely note that compileSdkVersion 31 strictly requires JDK11. JDK8 will suffer internal compiler errors when confronted with the 31 SDK

given the compileSdkVersion bump required and JDK11 bump required, might be a major version but that's a philosophical question :-)

@krizzu
Copy link
Member

krizzu commented Mar 21, 2022

thanks @mikehardy , I updated release notes to also include a workaround, to use old version of Room/Kotlin, if one is not ready to upgrade

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.

4 participants