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(ios, flipper): update flipper sub-pods to support macCatalyst #33406

Closed
wants to merge 1 commit into from

Conversation

mikehardy
Copy link
Contributor

@mikehardy mikehardy commented Mar 10, 2022

Summary

Flipper-DoubleConversion and Flipper-Glog iOS pods received a build optimization
a few versions back that pre-compiled the pods and references the xcframework slices

Unfortunately, the pre-compile did not include macCatalyst slices, so this disabled support
for flipper on macOS for react-native >0.65

@lblasa has re-compiled the pods with the macCatalyst slices added

See facebook/flipper#3117

Changelog

[iOS] [Fixed] - update Flipper pods to support re-enable macCatalyst

Test Plan

  • The Flipper repo has a react-native test that appeared to work with these versions, CI should work here
  • Prove there is no regression, a flipper-enabled build test should work for simulator iOS target on arm64
  • Prove there is no regression, a flipper-enabled build test should work for simulator iOS target on x86_64 mac
  • Prove there is no regression, a flipper-enabled build test should work for real device iOS target
  • To prove the issue is resolved, a build should be attempted for a macCatalyst target, and it should work.

Flipper-DoubleConversion and Flipper-Glog iOS pods received a build optimization
a few versions back that pre-compiled the pods and references the xcframework slices

Unfortunately, the pre-compile did not include macCatalyst slices, so this disabled support
for flipper on macOS for react-native 0.65

@lblasa has re-compiled the pods with the macCatalyst slices added

See facebook/flipper#3117
@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 Mar 10, 2022
@kelset kelset removed the p: Sony Partner: Sony label Mar 10, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Mar 10, 2022
@kelset
Copy link
Collaborator

kelset commented Mar 10, 2022

I guess we could even dare to cherry-pick this into 0.68? (I'd guess it'd require also 5005715 to be picked though?)

@mikehardy
Copy link
Contributor Author

Unsure - I think @cortinico would have the most "taste" (read as: wisdom) on flipper bumps as the previous updater. should be safe? Would be great to pick it and I noted so in the road to 0.68 issue with links, but obviously if it's a mess then that's that and 0.69 it is. Catalyst has waited this long and it is still a bit of a niche so I'd lean towards release cadence vs perfect release, same feeling as I have for static frameworks (which I also want to work, but reality sometimes disagrees)

@kelset
Copy link
Collaborator

kelset commented Mar 10, 2022

I think the original concern with bumping to newer Flipper version was related to some Flipper-side requirement around the NDK which IIRC should have disappeared? But yeah 100% let's leave the last word to Nicola 👍

@facebook-github-bot
Copy link
Contributor

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

@cortinico
Copy link
Contributor

But yeah 100% let's leave the last word to Nicola 👍

Thanks for raising this @mikehardy

I believe we're safe merging this as the Flipper version (0.125.0) is untouched.
Therefore should have no repercussion on the Android side of things. Should be safe to merge.

It would require some manual intervention on the internal build, but it should be easy to do (I'll look into it).

@cortinico
Copy link
Contributor

(I'll look into it).

Heads up, @lblasa is actually taking this over 🙏

@mikehardy
Copy link
Contributor Author

🤔

Testing failed:
	RNTesterIntegrationTests:
		RNTester (8428) encountered an error (Early unexpected exit, operation never finished bootstrapping - no restart will be attempted. (Underlying Error: Test crashed with signal abrt before starting test execution. If you believe this error represents a bug, please attach the result bundle at /Users/distiller/Library/Developer/Xcode/DerivedData/RNTesterPods-cbjldxeeenzmdtbkoznuxxultmjt/Logs/Test/Run-RNTester-2022.03.10_16-06-12-+0000.xcresult))

** TEST FAILED **

https://app.circleci.com/pipelines/github/facebook/react-native/12421/workflows/1936c0ee-ad3a-405f-9382-6ef926d46423/jobs/239513?invite=true#step-123-10315

In the testing do we need to split for JSC / Hermes build as part of regression test matrix? Is test_ios_unit_jsc failing on main / for other PRs and this is just ongoing failure?

I checked and it does not appear any other runs of this job have failed recently, strongly implying something here is either flaky or actually busted.

I just asked CircleCI to rerun, to at least get sample-size of two on flake hypothesis.

@analysis-bot
Copy link

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

Base commit: 325be42
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,214,112 +0
android hermes armeabi-v7a 7,799,826 +0
android hermes x86 8,587,591 +0
android hermes x86_64 8,538,542 +0
android jsc arm64-v8a 9,882,660 +0
android jsc armeabi-v7a 8,852,911 +0
android jsc x86 9,852,230 +0
android jsc x86_64 10,447,303 +0

Base commit: 325be42
Branch: main

@mikehardy
Copy link
Contributor Author

I just asked CircleCI to rerun, to at least get sample-size of two on flake hypothesis.

Okay, second run was clean. Very interesting. Absence of proof is not proof of absence, so there could still be a real error, or there is a test flake, but with sample size of 2 at least, it is not an obvious disaster-type-crash-every-time-failure

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @mikehardy in 2a5265d.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Mar 11, 2022
@kelset kelset deleted the flipper-maccatalyst-3117 branch March 11, 2022 11:18
@mikehardy
Copy link
Contributor Author

Just a note that the version bumps were tested for catalyst by an interested party and are not the solution yet. I believe this PR indicates we can effectively make change without regression (a great result, from a larger perspective) but we'll need new point releases of the pods with a tweak on the catalyst slice before macCatalyst is actually working

@cortinico
Copy link
Contributor

but we'll need new point releases of the pods with a tweak on the catalyst slice before macCatalyst is actually working

Do you need any support from our end?

@mikehardy

This comment was marked as outdated.

@mikehardy
Copy link
Contributor Author

It appears this one was actually sufficient and ready for cherry-picking.
facebook/flipper#3117 (comment) for details, and a doctor check for the build tweaks required has been logged as enhancement request react-native-community/cli#1573

No further action required here, hopefully it picks to rn68 and works well for everyone the way it did in my verification script

ShikaSD pushed a commit that referenced this pull request Mar 17, 2022
…3406)

Summary:
Flipper-DoubleConversion and Flipper-Glog iOS pods received a build optimization
a few versions back that pre-compiled the pods and references the xcframework slices

Unfortunately, the pre-compile did not include macCatalyst slices, so this disabled support
for flipper on macOS for react-native >0.65

lblasa has re-compiled the pods with the macCatalyst slices added

See facebook/flipper#3117

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - update Flipper pods to support re-enable macCatalyst

Pull Request resolved: #33406

Test Plan:
- [ ] The Flipper repo has a react-native test that appeared to work with these versions, CI should work here
- [ ] Prove there is no regression, a flipper-enabled build test should work for simulator iOS target on arm64
- [ ] Prove there is no regression, a flipper-enabled build test should work for simulator iOS target on x86_64 mac
- [ ] Prove there is no regression, a flipper-enabled build test should work for real device iOS target
- [ ] To prove the issue is resolved, a build should be attempted for a macCatalyst target, and it should work.

Reviewed By: cortinico

Differential Revision: D34789654

Pulled By: lblasa

fbshipit-source-id: 466803dd07b5820220512b7d7d760b94b8aa65f7
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…cebook#33406)

Summary:
Flipper-DoubleConversion and Flipper-Glog iOS pods received a build optimization
a few versions back that pre-compiled the pods and references the xcframework slices

Unfortunately, the pre-compile did not include macCatalyst slices, so this disabled support
for flipper on macOS for react-native >0.65

lblasa has re-compiled the pods with the macCatalyst slices added

See facebook/flipper#3117

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - update Flipper pods to support re-enable macCatalyst

Pull Request resolved: facebook#33406

Test Plan:
- [ ] The Flipper repo has a react-native test that appeared to work with these versions, CI should work here
- [ ] Prove there is no regression, a flipper-enabled build test should work for simulator iOS target on arm64
- [ ] Prove there is no regression, a flipper-enabled build test should work for simulator iOS target on x86_64 mac
- [ ] Prove there is no regression, a flipper-enabled build test should work for real device iOS target
- [ ] To prove the issue is resolved, a build should be attempted for a macCatalyst target, and it should work.

Reviewed By: cortinico

Differential Revision: D34789654

Pulled By: lblasa

fbshipit-source-id: 466803dd07b5820220512b7d7d760b94b8aa65f7
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. Merged This PR has been merged. Platform: iOS iOS 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.

None yet

6 participants