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

Update XCFramework setup to work with React Native 0.71.11 #5924

Merged

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Jul 3, 2023

What is says on the title. See also the work done in #5908 and #5912.

Leaving this as a draft so I can rebase freely until I have generated an binary that runs fine via installable builds in Jetpack/WordPress iOS.

This build made from b275a9d builds and has green unit tests. However, the editor results empty and crashes when it's dismissed UPDATE: This crash has been fixed as mentioned here..

These are warnings thrown in the console when loading the editor:

2023-07-04 09:47:28.012280+1000 WordPress[57333:39894628] Message: Tag aztec.htmltag.rootnode invalid

2023-07-04 09:47:28.017045+1000 WordPress[57333:39894628] Message: Tag aztec.htmltag.rootnode invalid

2023-07-04 09:47:28.020425+1000 WordPress[57333:39894628] -[__NSDictionaryM firstObject]: unrecognized selector sent to instance 0x2dee22210
2023-07-04 09:47:28.021078+1000 WordPress[57333:39894628] [native] Exception thrown while executing UI block: -[__NSDictionaryM firstObject]: unrecognized selector sent to instance 0x2dee22210

And this is how it crashes:

Screenshot 2023-07-04 at 9 48 29 am

For what is worth, the crashes happen in the version using JavaScriptCore instead of Hermes.

SiobhyB and others added 30 commits June 21, 2023 13:54
The method we used to generate React-Codegen (get_react_codegen_spec) was moved to a different file in newer versions of React Native. We need to update the script for generating it accordingly.
The file was automatically deleted via the generate-podspecs.sh script. It has been manually added as a temporary measure, to enable further testing.
This is intended as a temporary workaround, we should implement a more robust fix before merging.
…0.71-ios

# Conflicts:
#	gutenberg
#	ios-xcframework/Podfile.lock
The issue is due to RNReanimated looking for react-native in the
node_modules relative to its location, which is how most projects would
be setup. Unfortunately, our projects is not standard.

When running `pod install` or `pod update`, we'd get:

```
Installing RNReanimated 2.17.0 (was 2.9.1-wp-4)
internal/modules/cjs/loader.js:934
  throw err;
  ^

Error: Cannot find module 'react-native/package.json'
Require stack:
- /Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/[eval]
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:931:15)
    at Function.resolve (internal/modules/cjs/helpers.js:113:19)
    at [eval]:1:9
    at Script.runInThisContext (vm.js:134:12)
    at Object.runInThisContext (vm.js:310:38)
    at internal/process/execution.js:81:19
    at [eval]-wrapper:6:22
    at evalScript (internal/process/execution.js:80:60)
    at internal/main/eval_string.js:27:3 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/[eval]'
  ]
}

[!] Invalid `RNReanimated.podspec` file: no implicit conversion of nil into String.

 #  from /var/folders/dq/cdqxvx3s5ps75564rpmb_dc00000gn/T/d20230627-89428-abwplk/RNReanimated.podspec:5
 #  -------------------------------------------
 #  reanimated_package_json = JSON.parse(File.read(File.join(__dir__, "package.json")))
 >  config = find_config()
 #  assert_no_multiple_instances(config)
 #  -------------------------------------------
```
Using only our custom specs, the build failed with

```
Multiple commands produce '/Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/DerivedData/XCFrameworkScaffold/Build/Products/Debug-iphonesimulator/ReactCommon/ReactCommon.framework/Headers/CallbackWrapper.h'

- Target 'ReactCommon' (project 'Pods') has copy command from '/Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/Pods/ReactCommon/react/bridging/CallbackWrapper.h' to '/Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/DerivedData/XCFrameworkScaffold/Build/Products/Debug-iphonesimulator/ReactCommon/ReactCommon.framework/Headers/CallbackWrapper.h'
- Target 'ReactCommon' (project 'Pods') has copy command from '/Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/Pods/ReactCommon/react/nativemodule/core/ReactCommon/CallbackWrapper.h' to '/Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/DerivedData/XCFrameworkScaffold/Build/Products/Debug-iphonesimulator/ReactCommon/ReactCommon.framework/Headers/CallbackWrapper.h'
```

However, with this setup the build fails with:

```
/Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/Pods/../../gutenberg/node_modules/react-native/React/FBReactNativeSpec/../../scripts/xcode/with-environment.sh: line 35: .xcode.env: command not found
/Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/Pods/../../gutenberg/node_modules/react-native/React/FBReactNativeSpec/../../scripts/xcode/with-environment.sh: line 35: node: command not found
[Warning] You need to configure your node path in the  environment.  You can set it up quickly by running:  echo 'export NODE_BINARY=' > .xcode.env  in the ios folder. This is needed by React Native to work correctly.  We fallback to the DEPRECATED behavior of finding . This will be REMOVED in a future version.  You can read more about this here: https://reactnative.dev/docs/environment-setup#optional-configuring-your-environment
/Users/gio/Developer/a8c/gutenberg-mobile/ios-xcframework/Pods/../../gutenberg/node_modules/react-native/React/FBReactNativeSpec/../../scripts/xcode/with-environment.sh: line 41: /scripts/find-node-for-xcode.sh: No such file or directory
[Error] Could not find node. It looks like that the .xcode.env or .xcode.env.local
```
This way, users can override what CocoaPods does without modifying the
codebase via the `.local` file.
They will take place if the process runs with `HERMES_ENABLED` true.
@mokagio mokagio marked this pull request as ready for review July 4, 2023 03:33
@fluiddot
Copy link
Contributor

fluiddot commented Jul 4, 2023

And this is how it crashes:

Screenshot 2023-07-04 at 9 48 29 am

Yep, I also encountered this issue when closing the editor. The interesting part is that the stack trace of the crash looks related to other crashes we currently have in production (example). We couldn't find a consistent way to reproduce them, so maybe now with Hermes we have a new path to debug it 🙏 .

UPDATE: It also crashes when reloading Metro.

As of wordpress-mobile/WordPress-iOS@3fcd1b5 (#20717), I am able to use this version of Gutenberg locally by first running LOCAL_GUTENBERG=true bundle exec pod install.

This is great, I confirmed that I can build the app locally using a local installation of Gutenberg 🎊 .

This is as far as my knowledge goes. I hope it's enough to hand it over to you folks. I'd love if you could tag me in the further PRs in this project, because I'd like to understand how things will get fixed. And of course, happy to help if something doesn't work!

Thank you so much @mokagio for your effort on the Gutenberg Mobile project and enabling the XCFramework 🙇 ! We can take over the last fixes related to the Editor to mark the XCFramework ready to be merged. We'll share keep you updated on the PRs once we finish the investigations.

Base automatically changed from upgrade/react-native-0.71-ios to upgrade/react-native-0.71.8 July 4, 2023 15:19
…71.11-xcframework

# Conflicts:
#	bin/generate-podspecs.sh
#	bundle/ios/App.js
#	bundle/ios/App.js.map
#	gutenberg
#	ios-xcframework/Podfile
#	ios-xcframework/Podfile.lock
#	third-party-podspecs/FBReactNativeSpec/FBReactNativeSpec.podspec.json
#	third-party-podspecs/hermes-engine.podspec.json
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jul 4, 2023

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@fluiddot
Copy link
Contributor

fluiddot commented Jul 4, 2023

Thank you so much @mokagio for your effort on the Gutenberg Mobile project and enabling the XCFramework 🙇 ! We can take over the last fixes related to the Editor to mark the XCFramework ready to be merged. We'll share keep you updated on the PRs once we finish the investigations.

I managed to provide a fix to the crash mentioned in the PR's description 🎊 , more info in this comment. I tested using a local build with local Gutenberg installation and confirmed the crash is no longer happening.

However, after updating the upstream branch (17eae86) looks like something got broken in the CI 🤔. I'll investigate further tomorrow to find out what recent change caused the issue.

Error:

[!] Failed to load 'RNReanimated' podspec:
--
  | [!] Invalid `RNReanimated.podspec` file: key not found: "REACT_NATIVE_NODE_MODULES_DIR".
  |  
  | #  from /var/folders/xl/csng4q450h96m4tbz0p12_xh0000gn/T/d20230704-4112-7kmkhj/RNReanimated.podspec:5
  | #  -------------------------------------------
  | #  reanimated_package_json = JSON.parse(File.read(File.join(__dir__, "package.json")))
  | >  config = find_config()
  | #  assert_no_multiple_instances(config)
  | #  -------------------------------------------

ios-xcframework/.gitignore Outdated Show resolved Hide resolved
ios-xcframework/Podfile Outdated Show resolved Hide resolved
@fluiddot
Copy link
Contributor

fluiddot commented Jul 5, 2023

I investigated the issue shared in this comment and seems the problem is caused by the fact that the Podfile file was updated. Pods are cached based on the checksum of Podfile file, last time it was generated (commit: 91bacf5 with cache key: gutenberg-mobile-local-pod-cache-3c11e3e58b525f775439aa6df93079f6788b02755b055e61b6a336affdbe53fa) when enabling Hermes it included a workaround to set the env var with the React Native module folder (reference). This explains why before updating this PR with the upstream branch, the CI jobs were passing. It can be checked in the Build iOS RN XCFramework & Publish to S3 CI job for ec0c7fa, try to find in the logs the string gutenberg-mobile-local-pod-cache-3c11e3e58b525f775439aa6df93079f6788b02755b055e61b6a336affdbe53fa to confirm that it's using Pods cache using the above key.

As far as I checked, I think setting the env var in pre_install is not working in all cases (reference). It's interesting because I can't reproduce the issue locally, but I'm wondering if we could simply set the env var right before adding the pod (example). @mokagio WDYT?

@dcalhoun
Copy link
Member

dcalhoun commented Jul 5, 2023

As far as I checked, I think setting the env var in pre_install is not working in all cases (reference). It's interesting because I can't reproduce the issue locally, but I'm wondering if we could simply set the env var right before adding the pod (example).

I encountered this same issue locally when testing a different PR related to this work. I do not remember which specific PR. At the time, relocating the environment variable logic out of pre_install worked. This seems like a sound approach to me.

@fluiddot
Copy link
Contributor

fluiddot commented Jul 5, 2023

@mokagio I went ahead and updated the PR with some commits in order to address recent feedback. Let me know if that's ok for you and if you have any concerns/questions. Thanks 🙇 !

@fluiddot
Copy link
Contributor

fluiddot commented Jul 5, 2023

As far as I checked, I think setting the env var in pre_install is not working in all cases (reference). It's interesting because I can't reproduce the issue locally, but I'm wondering if we could simply set the env var right before adding the pod (example).

I encountered this same issue locally when testing a different PR related to this work. I do not remember which specific PR. At the time, relocating the environment variable logic out of pre_install worked. This seems like a sound approach to me.

I fixed this issue via 630b737. I tested on a separate branch and confirmed the CI job is not failing on that step.

@fluiddot fluiddot requested a review from dcalhoun July 5, 2023 14:15
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

Changes look good to me, I'll hold my approval off until all CI jobs pass 🤞 .

Comment on lines +8 to +11
# We are still trying to decide whether to adopt Hermes or not.
#
# This switch allows us to switch between approaches on the go.
HERMES_ENABLED = ENV.fetch('HERMES_ENABLED', true)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could leave this open until we confirm that Hermes works as expected on iOS through testing.

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

Approving it as the CI jobbuild-ios-rn-xcframework-and-publish-to-s3 passed 🎊 .

@fluiddot
Copy link
Contributor

fluiddot commented Jul 5, 2023

Heads up that we managed to produce an installable build using these changes 🎊. I also tested building the app locally and worked, the only caveat is that I had to use Xcode 14.3 (I was previously using Xcode 14.0).

@fluiddot
Copy link
Contributor

fluiddot commented Jul 6, 2023

For the sake of continuing the RN upgrade, I'm going to go ahead and merge this into upgrade/react-native-0.71.8. If there's any feedback or potential changes that we'd like to make related to this workaround, I'd propose to do it directly in that PR.

@fluiddot fluiddot merged commit 30a4548 into upgrade/react-native-0.71.8 Jul 6, 2023
14 of 15 checks passed
@fluiddot fluiddot deleted the mokagio/support-rn-0.71.11-xcframework branch July 6, 2023 08:21
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.

None yet

4 participants