zulip / zulip-mobile

Zulip mobile apps for Android and iOS.
https://zulip.com/apps/
Apache License 2.0
1.3k stars 656 forks source link

android: Update targetSdkVersion to 34, i.e., Android 14 #5892

Closed chrisbobbe closed 1 month ago

chrisbobbe commented 2 months ago

With a prep commit that came from @gnprice's investigation into some code in react-native that needed to change; thanks Greg for that!

Fixes: #5877

chrisbobbe commented 2 months ago

Greg, when you have some time to do so, could you test this on your device running Android 14?

gnprice commented 2 months ago

Oh, do you have these changes as a branch in a react-native Git repo? It'd be good to (a) push that branch to our fork zulip/react-native, and (b) mention the branch name and commit ID here, in the commit that updates the patch.

chrisbobbe commented 2 months ago

Oh, do you have these changes as a branch in a react-native Git repo? It'd be good to (a) push that branch to our fork zulip/react-native, and (b) mention the branch name and commit ID here, in the commit that updates the patch.

I don't, but I'll create such a branch and do those things.

chrisbobbe commented 2 months ago

Done; revision pushed.

gnprice commented 2 months ago

Thanks!

I just tried this version with tools/run-android on my Android 14 device (a Pixel 8). It still crashes on startup, with the same stack trace as at https://github.com/zulip/zulip-mobile/issues/5877#issuecomment-2364347383 .

I looked at the Java source file that's supposed to be patched, and it has the patch:

$ grep -C2 compatRegister node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerBase.java
        IntentFilter filter = new IntentFilter();
        filter.addAction(getReloadAppAction(mApplicationContext));
        compatRegisterReceiver(mApplicationContext, mReloadAppBroadcastReceiver, filter, true);
        mIsReceiverRegistered = true;
      }
--
   * <p>https://developer.android.com/about/versions/14/behavior-changes-14#runtime-receivers-exported
   */
  private void compatRegisterReceiver(
      Context context, BroadcastReceiver receiver, IntentFilter filter, boolean exported) {
    if (Build.VERSION.SDK_INT >= 34 && context.getApplicationInfo().targetSdkVersion >= 34) {

I suspect what's happening is that the build of the Android side is using a published binary artifact for the 0.68.7 release, not the source code in node_modules/. That's the reason for the Gradle-related steps described here: https://github.com/zulip/zulip-mobile/blob/main/docs/howto/rn-from-git.md#per-project-setup

So I think those steps will still be necessary.

The other two changes we currently have in this file are in JS code (for tests only, in fact) and in a build-time script for the iOS build. That's why they didn't need those changes.

When I read this PR on Friday, I looked in the history and saw our previous use of patch-package for react-native, at b546b0785771363a3fe28037bf4c22d88608dbb0, which is in native code that I expect is part of that same build. So that caused me to think that maybe things had changed since I last studied how this aspect of the build worked, and those setup changes weren't needed.

But looking again now, I see from the commit message that the problem it was meant to solve was only in a build with Xcode, for iOS. Those have always built from source, rather than using a published build artifact like the Android build. So I think the story must be that that patch never did have an effect on Android — and that just was fine, because the problem it was solving didn't exist there.

chrisbobbe commented 2 months ago

I see. So, more work to do here.

In step 4 of that doc:

  1. Follow the various steps in this doc that involve tweaking several Gradle files located under android/ in the app. Commit the result.

I followed that link and added the includeBuild lines it says to add:

  // ...
  include ':app'
  includeBuild('../node_modules/@react-native/gradle-plugin')

+ includeBuild('../node_modules/react-native') {
+     dependencySubstitution {
+         substitute(module("com.facebook.react:react-android")).using(project(":packages:react-native:ReactAndroid"))
+         substitute(module("com.facebook.react:react-native")).using(project(":packages:react-native:ReactAndroid"))
+         substitute(module("com.facebook.react:hermes-android")).using(project(":packages:react-native:ReactAndroid:hermes-engine"))
+         substitute(module("com.facebook.react:hermes-engine")).using(project(":packages:react-native:ReactAndroid:hermes-engine"))
+     }
+ }

But that caused a build failure:

$ tools/gradle :app:assembleDebug

FAILURE: Build failed with an exception.

* Where:
Settings file '/Users/chrisbobbe/dev/zulip-mobile/android/settings.gradle' line: 13

* What went wrong:
Project with path ':packages:react-native:ReactAndroid' not found in build 'react-native'.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 1s

That docs website has been updated a few times, and I think we'll need to follow an older version of these instructions.

I tried the next-to-latest version, before facebook/react-native-website@22da0cc63 "Update how-to-build-from-source for 0.72+". That failed with Project with path ':ReactAndroid' not found in build 'react-native'..

So I think the next thing for me to try is the version just before facebook/react-native-website@50a01ac0d 'Update the "Build from source" page'. Those instructions are much more complicated, though, so I'd like to check in before I get started. Does that seem like the right next step? Thanks in advance :)

gnprice commented 2 months ago

So I think the next thing for me to try […] Does that seem like the right next step?

Let's move to a chat thread for that discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.20from.20Git/near/1948101

chrisbobbe commented 1 month ago

Thanks for all your help on this, @gnprice! Revision pushed.

chrisbobbe commented 1 month ago

OK, added a commit to let Gradle use more memory; that should help with the CI failure (which was showing out-of-memory errors).

gnprice commented 1 month ago

Great, glad to see this working!

The changes all look good. And I just tested making a debug build on my laptop and running it on my Android 14 device, and it seems to run fine. Merging now.