Flatten embedded frameworks when generating XCFramework #5968

fluiddot · 2023-07-17T14:43:55Z

The goal of this PR is to flatten frameworks nested under the Frameworks folder of other frameworks. Especially, the Hermes framework which is nested under the Gutenberg framework.

This has been achieved by simply moving the embedded frameworks to the root level (e03356d), plus including the Hermes framework in the vendor framework list of the Gutenberg podspec (355f431).

To test

Preparation:

Check out the branch gutenberg/tweak-pod-deps-by-rn-version from the WP-iOS repository (PR reference).

Test via CI

Wait until Build iOS RN XCFramework & Publish to S3 CI job finishes.
Update the Gutenberg Mobile reference to the commit 64a5fed3fd28e7fa4aa25edc78f39b76617e5f71.
Run the command bundle install && bundle exec pod install.
NOTE: If any error is encountered, try to run the command rm -rf build && rm -rf Pods before installing Pods.
Build and run the WP-iOS app using a physical device.
Observe that the app doesn't crash and that the editor works as expected.

Test via local installation

⚠️ Note that this testing approach is mainly to test XCFramework integration without having to publish the framework.

Apply the following patch:

diff --git forkSrcPrefix/ios-xcframework/Gutenberg.podspec forkDstPrefix/ios-xcframework/Gutenberg.podspec
new file mode 100644
index 0000000000000000000000000000000000000000..ccba32ed4e4053b51f638d3238e4213324f34ca9
--- /dev/null
+++ forkDstPrefix/ios-xcframework/Gutenberg.podspec
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+Pod::Spec.new do |s|
+  s.name = 'Gutenberg'
+  s.summary = 'Printing since 1440'
+  s.homepage = 'https://github.com/wordpress-mobile/gutenberg-mobile'
+  s.authors = 'Automattic'
+
+  s.version = '1.98.1'
+
+  s.license = { type: 'GPL-2' }
+
+  s.ios.deployment_target = '15.0'
+  s.swift_version = '5.0'
+
+  s.source = {
+    http: 'file://' + __dir__ + '/build/xcframeworks/Gutenberg.tar.gz'
+  }
+
+  s.vendored_frameworks = [
+    'Frameworks/Aztec.xcframework',
+    'Frameworks/Gutenberg.xcframework',
+    'Frameworks/React.xcframework',
+    'Frameworks/RNTAztecView.xcframework',
+    'Frameworks/yoga.xcframework',
+    'Frameworks/hermes.xcframework'
+  ]
+end

In the WP-iOS project, change the Gutenberg Mobile podspec reference to: ../gutenberg-mobile/ios-xcframework/Gutenberg.podspec
Run the command bundle install && bundle exec pod install.
NOTE: If any error is encountered, try to run the command rm -rf build && rm -rf Pods before installing Pods.
Build and run the WP-iOS app using a physical device.
Observe that the app doesn't crash and that the editor works as expected.

PR submission checklist:

I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

Add `flattenFrameworks` helper in xcframework build script

peril-wordpress-mobile · 2023-07-17T17:04:13Z

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

fluiddot · 2023-07-17T17:06:41Z

ios-xcframework/build.sh

 #
 # Notice how we loop on $PLATFORM_IOS as a way to get the frameworks for both platforms.
 # We could use either platform to achieve the same result.
-for FRAMEWORK in $(find "$ARCHIVES_ROOT/$PLATFORM_IOS.xcarchive/Products/Library/Frameworks" -type d -name "*.framework");
+for FRAMEWORK in "$ARCHIVES_ROOT/$PLATFORM_IOS.xcarchive/Products/Library/Frameworks"/*;


This change implies that we'll only loop over the frameworks at the root level. Embedded frameworks (i.e. nested in framework folders) will be skipped as it should have been processed in the previous step.

fluiddot · 2023-07-17T17:07:41Z

ios-xcframework/build.sh

+function flattenFrameworks {
+  PLATFORM=$1
+
+  for FRAMEWORK in "$ARCHIVES_ROOT/$PLATFORM.xcarchive/Products/Library/Frameworks"/*;
+  do
+    CURRENT_FRAMEWORK_NAME=$(basename "$FRAMEWORK" .framework)
+
+    if [[ -d "$FRAMEWORK/Frameworks" ]]; then
+      log "[$PLATFORM] $CURRENT_FRAMEWORK_NAME has embedded frameworks, flattening them..."
+
+      for EMBEDDED_FRAMEWORK in "$FRAMEWORK/Frameworks"/*;
+      do
+        CURRENT_EMBEDDED_FRAMEWORK_NAME=$(basename "$EMBEDDED_FRAMEWORK" .framework)
+
+        log 'package' "Flattening $CURRENT_EMBEDDED_FRAMEWORK_NAME"
+
+        mv "$EMBEDDED_FRAMEWORK" "$ARCHIVES_ROOT/$PLATFORM.xcarchive/Products/Library/Frameworks"
+      done
+      rm -rf "$FRAMEWORK/Frameworks"
+    fi
+  done


This function simply moves embedded frameworks (i.e. nested under the Frameworks folder within another framework) to the root level.

I'm curious if this would be enough to make it work, i.e. I wonder if we don't need to update some @loadpath (via some install_name_tool dance) so that the dynamic loader (dyld) would be able to properly load the (ex-embded) framework from the parent framework (i.e. load Hermes from Gutenberg) correctly (especially if the load path as printed by otool -L would show Hermes being referenced as a relative path from Gutenberg) 🤔

I think that if you run otool -L DerivedData/Build/Products/…/WordPress.app/WordPress on the compiled and linked WordPress executable after it has integrated the Gutenberg.xcframework via CocoaPods, you should be able to check that both Gutenberg and Hermes are referenced as @rpath/[Name].framework/[Name]? Though I'd be curious to see if otool -L DerivedData/Build/Products/…/WordPress.app/Frameworks/Gutenberg.framework/Gutenberg would reference Hermes itself (and if so, how) or not at that point (I'm not familiar with the setup that Gio put in place and how Gutenberg and Hermes interact / might have link-time dependencies on each other)

In practice, if we can find out how to do it, I think it would be better to have the archive action generate Hermes and Gutenberg as separate flattened frameworks to begin with at compile time, rather than have Hermes be generated as an embed framework by Xcode in the xcarchive it generates, and then have our scripts try to de-embed it manually after the fact… 🤔

Thank you so much @AliSoftware for the feedback about this approach 🙇 !

I'm curious if this would be enough to make it work, i.e. I wonder if we don't need to update some @loadpath (via some install_name_tool dance) so that the dynamic loader (dyld) would be able to properly load the (ex-embded) framework from the parent framework (i.e. load Hermes from Gutenberg) correctly (especially if the load path as printed by otool -L would show Hermes being referenced as a relative path from Gutenberg) 🤔

I'm testing the changes in combination with wordpress-mobile/WordPress-iOS#21021, and so far building the app locally for both the simulator and physical device seems to work. I can take a quick look at the commands you share to validate that references are not broken, although I understand I'd probably experience a build failure or runtime crash in that case 🤔.

In practice, if we can find out how to do it, I think it would be better to have the archive action generate Hermes and Gutenberg as separate flattened frameworks to begin with at compile time, rather than have Hermes be generated as an embed framework by Xcode in the xcarchive it generates, and then have our scripts try to de-embed it manually after the fact… 🤔

Good idea. I assume this would be done by adding a phase script in the Xcode project to flatten the embedded frameworks, is this accurate?

I assume this would be done by adding a phase script in the Xcode project to flatten the embedded frameworks, is this accurate?

I was more thinking about selecting "Do Not Embed" in the "Frameworks and Libraries" settings of the XCFrameworkScaffold.xcodeproj:

Though to be honest, I haven't looked at how the Gutenberg.framework compilation works (how the scaffold project is configured in details and how this interacts with the build scripts like build.sh and the declarations in the podspec and the potential custom scripts / post_install hooks in client app and all that), so I'd prefer to let @mokagio chime in on the details there than trying to grasp at straws without knowing the details about the bigger picture.

I think that if you run otool -L DerivedData/Build/Products/…/WordPress.app/WordPress on the compiled and linked WordPress executable after it has integrated the Gutenberg.xcframework via CocoaPods, you should be able to check that both Gutenberg and Hermes are referenced as @rpath/[Name].framework/[Name]? Though I'd be curious to see if otool -L DerivedData/Build/Products/…/WordPress.app/Frameworks/Gutenberg.framework/Gutenberg would reference Hermes itself (and if so, how) or not at that point (I'm not familiar with the setup that Gio put in place and how Gutenberg and Hermes interact / might have link-time dependencies on each other)

I've run the commands and seems the reference is correct. For both the app's executable and Gutenberg framwork, they point to Hermes (@rpath/hermes.framework).

Result of otool -L DerivedData/.../Build/Products/Debug-iphoneos/Jetpack.app/Jetpack:

[...] @rpath/Gutenberg.framework/Gutenberg (compatibility version 1.0.0, current version 1.0.0) @rpath/hermes.framework/hermes (compatibility version 0.12.0, current version 0.12.0) [...]

Result of otool -L DerivedData/.../Build/Products/Debug-iphoneos/Jetpack.app/Frameworks/Gutenberg.framework/Gutenberg:

[...] @rpath/Gutenberg.framework/Gutenberg (compatibility version 1.0.0, current version 1.0.0) @rpath/hermes.framework/hermes (compatibility version 0.12.0, current version 0.12.0) [...]

I was more thinking about selecting "Do Not Embed" in the "Frameworks and Libraries" settings of the XCFrameworkScaffold.xcodeproj:

FWIW, #5973 uses that approach.

fluiddot · 2023-07-17T17:08:15Z

.buildkite/pipeline.yml

-        echo "--- :package: Run bundle prep work"
-        npm run prebundle:js
-


I skipped this step to speed up the bundle generation, it should be reverted before merging the PR.

I'm surprised the build worked without it 🤔 Maybe it's no longer necessary and we should not add it back?

When installing the dependencies, we download all localizations for all plugins via the command i18n:check-cache. This includes tons of strings that are not even used in the editor, leading to a bigger bundle with an increase of around an extra 20 MB. The command i18n:update analyzes the strings referenced within the bundles generated by React Native, and removes the unneeded ones. As well as it generates the localization strings files (example) to incorporate into the host apps.

gutenberg-mobile/package.json

Line 61 in 6f1d7e7

"postinstall": "patch-package && npm run clean:gutenberg:distclean && npm ci --prefix gutenberg && npm run i18n:check-cache && ./bin/run-jetpack-command.sh \"install --ignore-scripts\"",

Hence, the prebundle command is not strictly needed when generating the bundle, but strongly recommended to optimize the bundle size. In this regard, now that we'll generate both Android and iOS bundles in CI, I'm thinking of reconsidering this step for the sake of reducing the build time. In the end, we'd only need the optimal bundle when releasing a new version of Gutenberg Mobile, so we might add a condition to the job to run prebundle in that situation.

gutenberg-mobile/package.json

Line 73 in a842544

"prebundle:js": "npm run i18n:update",

Thank you for the explanation @fluiddot.

In the end, we'd only need the optimal bundle when releasing a new version of Gutenberg Mobile, so we might add a condition to the job to run prebundle in that situation.

I see... We could do this by checking if the CI build is running on a tag

This reverts commit 603dd7e.

fluiddot · 2023-07-19T14:33:33Z

Closing this PR in favor of #5973.

Update Gutenberg ref

e3630ba

fluiddot added the [Status] DO NOT MERGE Do not merge this PR label Jul 17, 2023

fluiddot self-assigned this Jul 17, 2023

fluiddot changed the base branch from trunk to upgrade/react-native-0.71.8 July 17, 2023 14:44

fluiddot added 4 commits July 17, 2023 19:02

Flatten embedded frameworks when generating xcframework

e03356d

Add `flattenFrameworks` helper in xcframework build script

Add Hermes framework to podspec generator

355f431

Update Podfile.lock file

603dd7e

[TEST] Skip prebundle command

64a5fed

fluiddot force-pushed the test/flatten-embedded-frameworks branch from 1b71a87 to 64a5fed Compare July 17, 2023 17:04

fluiddot commented Jul 17, 2023

View reviewed changes

fluiddot changed the title ~~[TEST] Flatten embedded frameworks when generating XCFramework~~ Flatten embedded frameworks when generating XCFramework Jul 17, 2023

fluiddot removed the [Status] DO NOT MERGE Do not merge this PR label Jul 17, 2023

fluiddot requested review from mokagio and SiobhyB July 17, 2023 17:23

fluiddot mentioned this pull request Jul 17, 2023

[Gutenberg] Tweak Pod dependencies based on React Native version used by Gutenberg wordpress-mobile/WordPress-iOS#21021

Closed

13 tasks

fluiddot added the Tooling label Jul 17, 2023

Revert "Update Podfile.lock file"

df0687d

This reverts commit 603dd7e.

mokagio mentioned this pull request Jul 18, 2023

Do not embed Hermes in the Gutenberg XCFramework. Add to distribution archive instead. #5973

Merged

fluiddot closed this Jul 19, 2023

fluiddot deleted the test/flatten-embedded-frameworks branch July 19, 2023 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flatten embedded frameworks when generating XCFramework #5968

Flatten embedded frameworks when generating XCFramework #5968

fluiddot commented Jul 17, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Jul 17, 2023 •

edited

Loading

fluiddot Jul 17, 2023

fluiddot Jul 17, 2023

AliSoftware Jul 17, 2023

AliSoftware Jul 17, 2023

fluiddot Jul 18, 2023

AliSoftware Jul 18, 2023

fluiddot Jul 18, 2023

mokagio Jul 19, 2023

fluiddot Jul 17, 2023

mokagio Jul 18, 2023

fluiddot Jul 18, 2023

mokagio Jul 19, 2023

fluiddot commented Jul 19, 2023

		echo "--- :package: Run bundle prep work"
		npm run prebundle:js

Flatten embedded frameworks when generating XCFramework #5968

Flatten embedded frameworks when generating XCFramework #5968

Conversation

fluiddot commented Jul 17, 2023 • edited Loading

To test

Test via CI

Test via local installation

peril-wordpress-mobile bot commented Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluiddot commented Jul 19, 2023

fluiddot commented Jul 17, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Jul 17, 2023 •

edited

Loading