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

fix(android): make template / rn-tester Android 12 compatible #32540

Closed
wants to merge 1 commit into from

Conversation

mikehardy
Copy link
Contributor

Summary

The stock new app template is not Android-12 compatible, and I did not notice any issues or PRs opened for it when I searched, though I may have missed it? So I'm proposing what I think are the necessary changes so new apps work on Android 12 out of the box

Changelog

[Android] [Fixed] - updated new app template and rn-tester for Android 12 support

Test Plan

  1. Check out my branch
  2. Make a temporary edit to template/package.json to replace react-native version 1000.0.0 with 0.67.0-rc.2
  3. Make a temporary edit to template/android/app/src/debug/.../ReactNativeFlipper.java to revert this change which makes ReactNativeFlipper incompatible with the react-native 0.67-0-rc.2 release (ce74aa4#diff-1e429281c560d83c035cbe6f50683ada72292f7860de108d420fb55185ababad)
  4. npx react-native init Android12Test --template git@github.com:mikehardy/react-native.git#android12
  5. cd Android12Test && npx react-native run-android

Those test steps worked for me

I assume rn-tester is tested in CI? If not it will clearly work or clearly blow up when run, but I did check locally:

  1. Check out my branch
  2. yarn install
  3. ./gradlew :packages:rn-tester:android:app:installJscDebug (this is where it would fail if there was a problem)
  4. ./scripts/packager.sh
  5. start the app on the device - it all seems to work

Note that I am able to pop up the android debug settings (the DevSupportActivity) with ctrl-M on linux but I am simply not able to get the emulator shake to bring it up with exported:true or exported:false. Just in case, and since it is debug only I labeled that activity exported:true to make sure it could be called if needed.

If someone with more knowledge of how and when DevSupportActivity is used in the ecosystem it may be possible to have this as exported:false safely.

- bump targetSdkVersion / compileSdkVersion to 31
- add required `android:exported` notations for activities in AndroidManifest / docs
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Sony Partner: Sony Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 4, 2021
@ShikaSD
Copy link
Contributor

ShikaSD commented Nov 4, 2021

hey, that's for contributing this!
we already have a similar change internally, but there are some issues with JDK 11 on CI that block this from moving forward :)

@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Nov 4, 2021
@mikehardy
Copy link
Contributor Author

hey @ShikaSD yeah - the changes are tiny and pretty important so I am not surprised if it is duplicate.
I am curious though, I don't think JDK11 is necessary for these specific changes?

I was building with JDK11 during my initial round of testing, but just to make sure I just did this:

PATH=:/usr/lib/jvm/java-8-openjdk-amd64/bin:$PATH java -fullversion

and received: openjdk full version "1.8.0_302-8u302-b08-0ubuntu2-b08"

Then this works as well:

PATH=:/usr/lib/jvm/java-8-openjdk-amd64/bin:$PATH ./gradlew :packages:rn-tester:android:app:installJscDebug

Then in my Android12Test init app I did this in the android directory:

PATH=:/usr/lib/jvm/java-8-openjdk-amd64/bin:$PATH ./gradlew assembleDebug

It all works.

This should not be blocked on any JDK11 issues I think.

@mikehardy
Copy link
Contributor Author

CI failure analyze_code is eslint issues, not touched in this PR, not related
CI failure build_npm_package-1 is a PR bot failing to post links to the PR at the end of the run, not related

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,350,982 +13
android hermes armeabi-v7a 8,333,050 +16
android hermes x86 9,983,368 +11
android hermes x86_64 9,768,735 +5
android jsc arm64-v8a 10,665,543 -4
android jsc armeabi-v7a 9,311,087 +1
android jsc x86 10,777,600 -5
android jsc x86_64 11,216,911 +6

Base commit: e918362
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: e918362
Branch: main

@mikehardy
Copy link
Contributor Author

Okay @ShikaSD this appears to be proof there are no JDK dependencies for this to work: https://app.circleci.com/pipelines/github/facebook/react-native/10992/workflows/be8368b7-8ef7-4671-b057-6ebc3a3e232f/jobs/224244

Only CI failures are unrelated, and it works for android in CI and local testing with JDK11 or JDK8

Hopefully this may be considered for merge

@mikehardy
Copy link
Contributor Author

In conversation on Discord, it was requested that an attempt be made to move ./ReactAndroid/build.gradle to 31 as well, that does not seem possible on JDK1.8:

> Task :ReactAndroid:compileDebugJavaWithJavac FAILED
An exception has occurred in the compiler (1.8.0_302). Please file a bug against the Java compiler via the Java bug reporting page (http://bugreport.java.com) after checking the Bug Database (http://bugs.java.com) for duplicates. Include your program and the following diagnostic in your report. Thank you.
java.lang.AssertionError: annotationType(): unrecognized Attribute name MODULE (class com.sun.tools.javac.util.UnsharedNameTable$NameImpl)
        at com.sun.tools.javac.util.Assert.error(Assert.java:133)
        at com.sun.tools.javac.code.TypeAnnotations.annotationType(TypeAnnotations.java:231)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.separateAnnotationsKinds(TypeAnnotations.java:294)
        at com.sun.tools.javac.code.TypeAnnotations$TypeAnnotationPositions.visitMethodDef(TypeAnnotations.java:1066)
        at com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:778)

But this PR, just moving the rn-tester app and template seem fine.

@yungsters
Copy link
Contributor

Thanks, @mikehardy. I'm going to import this and ask @ShikaSD to take a closer look using internal tools.

@facebook-github-bot
Copy link
Contributor

@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@mikehardy
Copy link
Contributor Author

Sounds good - I think the general sentiment was that unblocking JDK11 and moving ReactAndroid to api 31 at the same time was ideal, at which point this is abandoned, but if this works now and JDK11 doesn't look like it'll happen, it'll at least get new users' apps on 31 from template init :-). Cheers

@yungsters
Copy link
Contributor

Quick update here… @ShikaSD and @cortinico are investigating the best path forward.

@ShikaSD
Copy link
Contributor

ShikaSD commented Nov 25, 2021

Fixed by #32606

@ShikaSD ShikaSD closed this Nov 25, 2021
@mikehardy
Copy link
Contributor Author

Beautiful! I know that was one of those seemingly small changes that actually rippled out everywhere and only looked small. I appreciate it. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Sony Partner: Sony Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants