-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Conversation
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
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?) |
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) |
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 👍 |
@lblasa has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thanks for raising this @mikehardy I believe we're safe merging this as the Flipper version (0.125.0) is untouched. It would require some manual intervention on the internal build, but it should be easy to do (I'll look into it). |
Heads up, @lblasa is actually taking this over 🙏 |
🤔
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. |
Base commit: 325be42 |
Base commit: 325be42 |
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 |
This pull request was successfully merged by @mikehardy in 2a5265d. When will my fix make it into a release? | Upcoming Releases |
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 |
Do you need any support from our end? |
This comment was marked as outdated.
This comment was marked as outdated.
It appears this one was actually sufficient and ready for cherry-picking. No further action required here, hopefully it picks to rn68 and works well for everyone the way it did in my verification script |
…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
…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
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