-
Notifications
You must be signed in to change notification settings - Fork 954
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
🐛 MacCatalyst Does not Build on RN above 64 because of RCT-Folly (1/2 Fix Included Here!) #3117
Comments
Wow, that took a while to find. https://github.com/priteshrnandgaonkar/double-conversion/blob/master/archive-xcframework.sh Pritesh, you need to include a macCatalyst slice, easy to build but needs a very very careful re-arrangement of built artifacts after build, before packaging:
|
@mikehardy thanks for reporting this and finding the problem. I will provide a fix and update once ready. |
Fantastic! As someone that does not generally like workarounds (I love to PR upstream and just fix things...) I was surprised by the need to do that catalyst slice repack but please understand I tested that super carefully back and forth and that strategy was the only way I successfully built for catalyst in a published pod, painful reference: (that was from when Notifee was proprietary, thus this was needed, it's open source now but this was so hard to figure out...) |
@mikehardy I am so glad we got this the attention it needs! I am giving you a virtual high five 🙌 |
In classic "hey, while you're in there..." fashion I'll mention that the tvOS and watchOS folks will be after you next. I had work in that area but didn't have time to merge it in Notifee. The natural extensions on the pbxproj and build framework script did work as expected though, I had a valid xcframework with it https://github.com/invertase/notifee/pull/56/files#diff-5bdb27f5710cdebde3c248e0e983ee16d5e5c1173cfe0cc8b31011f56505d5f1 It's still on my todo list to actually make a github repo and blog about this so anyone doing xcframeworks just has a script that works and we can all maintain just one... |
@lblasa — do you have any progress update on this? |
It seems that the problem is deeper than just 2 fixes. I've forked Flipper-DoubleConversion repo and patched xcframework creation script (https://github.com/Arkkeeper/double-conversion). I've also modified react-native/scripts/react_native_pods.rb in order to fetch Flipper-DoubleConversion from that fork, so now it includes a necessary slice for Mac Catalyst. But that's not enough for successful builds. Now I keep getting errors during linking stage about duplicate symbols between Flipper-Folly and RCT-Folly. We need a hint on how this duplication is being avoided when building for iOS or iOS Simulator. |
Ah I was sort of hoping that this issue was fixed with the 0.67 release. So it still persists? :/ |
Nope, I was too pessimistic :) Everything seems to be ok after fixing Flipper-DoubleConversion slices. "Duplicate symbols" error is caused by absence of DEAD_CODE_STRIPPING = YES in project.pbxproj, it was discussed here facebook/react-native#32333 (comment), but there is no necessary fix in RN 0.67. But... the happy end is still far, because Flipper still can't connect to Catalyst app and debug it. First I thought that it's related to idb, but that's not true. |
Hey @Arkkeeper - I don't see a PR for your maccatalyst xcframework fix - you should PR that! It looked good to me. I had to flatten the slice from it's structure when I did the same for Notifee but there is nothing like proof, and if you have it definitely working then you should PR and get those slices published for next release hopefully |
Our team is doing some additional research now, if everything is ok, I'll certainly publish that patch as a PR tomorrow. |
@priteshrnandgaonkar — looks like @Arkkeeper made a PR :) @mikehardy —what would be the next step(s) to see that added into RN 0.67.2? Can we make a fast patch release of RN that includes this fix? |
If I understand correctly First everything needs to be merged on main branches and working fir flipper Then flipper does a release and you test with a local change to use that version and that should be enough to unblock Then react native gets a PR to bump flipper version Then you use the commitly release from that react native merge, then it's in the next react native version I think since you can specify flipper version it doesn't need to wait for a react native release |
@mikehardy, it looks like @priteshrnandgaonkar is MIA. As the most central member to the RN community, do you want to host a new Flipper Double Conversion that incorporates @Arkkeeper's changes and offer a PR to the Flipper project itself? Let's not sleep on this now that it appears fixed. |
Can anyone give me a quick overview of the status of the open PRs/fixes to solve this? I feel a bit lost atm. Are the PRs to get this address open/merged or some things still need to be worked on? |
@kelset this is really thorny unfortunately. The proposed solution just above of me forking + publishing a new Flipper-DoubleConversion (and Flipper-Glog) based on @priteshrnandgaonkar work but including the work here to add maccatalyst slices to the vendored xcframework is a bad idea. Using more precise technical language: it would be spending dev time on a compile-time optimization that was made in the form of a 3rd-party personal fork of a major library by making another 3rd-party personal fork of the major library and having this major project then switch dependency from one personal fork to another. That's a bad idea right? Here's the technical analysis of where we areSo React Native needs Flipper, and Flipper needs the Flipper-DoubleConversion library. It pulls it in here:
which is just a reference to a specific version of a specific pod that is out-of-tree and live on cocoapods.org and happens to be named "Flipper-Something" but is not actually controlled by facebook/meta or this repo: Which is/was just a one-time publish of a personal repo from @priteshrnandgaonkar : That personal repo is a point in time / unupdated fork of the google/doubleconversion project, but with a pre-compiled xcframework included, you can see the work to do so in commit history (note, Flipper-Glog is doing something similar, pointing directly here: https://github.com/CocoaPods/Specs/blob/master/Specs/2/7/2/Flipper-Glog/0.3.9/Flipper-Glog.podspec.json) ...which is named Flipper-Glog but is just a one-time publish of an unupdated fork from Pritesh that has a pre-compiled xcframework. Here is quick list of pain pointsSo there are a few things going on here:
here's what I think needs to happen1- the compile optimizations introduced in #2153 by including pre-compiled DoubleConversion and Glog should be reverted and the local build/podspec stuff here related to those two libraries needs to be brought up to date with the whatever the local peer pods from 3rd party native libraries are doing so it's in tune again That may be enough to solve this issue by itself now, Maccatalyst will likely work at that point Someone needs to do this, I do not have time. Clone Flipper repo, look at what #2153 changed, manually revert the change so doubleconversion and glog are built again (using the same versions/tags in use now though) and see if Flipper works. PR that. Do a follow-on PR that brings them up to current versions if possible. See if maccatalyst builds for react-native if you depend on that new local flipper, should work? 2- if speed optimizations are desired, in CI here the workflows should enable and use ccache-action (react-native-firebase does this to good effect, as do all my work projects) 3- an audit should be made of dependencies that are spidering out to Pritesh's personal repos, and they should be internalized. A firm policy should be made to never do that again as it makes maintaining the web of dependencies here and moving the project forward impossible when dependencies spider out to personal repos with single maintainers There quite a few at the moment - it's a bit of a rabbit hole but not quite the abyss yet:
|
Sorry for the late reply, I will be looking into this very shortly and hopefully will have an update soon. |
First of all, thanks everybody for the discussion! I've just released new versions of Double-Conversion and Glog which catch up with the upstream repositories and contain slices for Mac Catalyst. I'm now the owner of all Flipper-* pods so I should be able to make any necessary updates and adjustments in the future. Unfortunately, right now, I don't have the capacity to make our dependency consumption better. Having said that, valid points were said and we will definitely look into making things better in future releases. |
I am all for incremental progress :-), changing dependency consumption is not the easiest, no, but at least now we're all aware. Having the catalyst slice in the new pods will be great - Edited: just tested, I see the new pods but So if my tests are valid and I understand correctly, I think there will need to be a Flipper release process in order to ingest this work before catalyst will work for people.
|
@mikehardy @lblasa @kelset — hey all. We are so close on this issue. Let's not drop the ball on it now. |
I’ll check on this early next week, sorry about the delay! |
I found we had pinned some versions in our ReactNativeExample and this was fixed here: After the change, I had a look at what versions were being pulled for our iOS Sample and ReactNativeExample and they all seem to point to the right versions. |
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
Ah ha The difference is that your So, for anyone suffering from this problem right now you may specify the same versions in that commit and you will have a working catalyst build. However in the empty / default Those versions need a change - @lblasa what versions should we put in there? Last person to do it was @cortinico in facebook/react-native#32923 and it appears we need to pick versions carefully. Should we just update the ones you touched? Assuming answer is yes - update only the ones updated here - I've posted a PR that attempts to do it, we'll see how CI likes it... |
…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
@Pajn I finally got a chance to try this on an M1 machine, I have it fully scripted and working on x86_64, but on M1 this morning, I got the same error you received. Are you on an M1 machine?
|
@mikehardy yes, I am. Forgot to say that, sorry; |
Okay - for anyone else following along, @Pajn and I have gotten to this point on an M1 machine for catalyst (it works for x86_64):
And that is where I am stuck right now 🤷 - I hit the end of my timebox on this for a while so I may not be able to look again for some time. macCatalyst +flipper works for x86_64, stuck on arm64 until someone can bust through the block here. |
Do we have an update on this ? |
@cyberdude Please read this and reflect: https://hackernoon.com/i-thought-i-understood-open-source-i-was-wrong-cf54999c097b There are no secret updates, I can say that. Catalyst works great everywhere now except M1 mac, but you could be the person to move that forward based on comments above in this issue |
@mikehardy Finally I've got some time for experiments. I've edited archive-xcframework.sh inside Flipper-Glog and Flipper-DoubleConversion in this manner: And I can confirm that now everything is ok on my M1 Mac with Catalyst (even with the new architecture enabled on RN 0.68.1). In order to try this on Intel Mac, one can patch node_modules/react-native/scripts/react_native_pods.rb and replace corresponding lines inside use_flipper! to:
Then delete ios/Pods and ios/Podfile.lock and do pod install or RCT_NEW_ARCH_ENABLED=1 pod install. |
@Arkkeeper excellent! https://github.com/lblasa/double-conversion |
I have tested new xcframework on Intel Mac by myself, it works as expected. My PRs are published to @lblasa forks. |
Hi @Arkkeeper thanks for the PRs and sorry about the late reply. I will be integrating these changes very soon and will push a new pod version for both dependencies :) |
Just a very quick follow-up. I've published new versions for both libraries based of @Arkkeeper PR's. @Arkkeeper I think is good for a re-test. If it does work, then I believe we could close this long-standing issue. |
Patch release bumps that just change arm64 slice packaging See facebook/flipper#3117 (comment)
Confirmed fixed! Oh my $deity!! Finally able to close this one I think. I'm PR'ing version bumps to react-native now. Fantastic! |
Summary: Patch release bumps that just change arm64 slice packaging See facebook/flipper#3117 (comment) These pods were just released by lblasa and this PR integrates them - I personally confirm success on an arm64 doing a macCatalyst build for the first time since react-native 0.64, and users report intel works fine as well - no regression ## Changelog [Catalyst][Fix] - use pods with arm64 macCatalyst slices Pull Request resolved: #33809 Test Plan: Run a macCatalyst build on intel machine and m1 machine, I use this harness as part of release-testers facebook group, and it exercises macCatalyst build if you pass in a valid development team (for signing) https://github.com/mikehardy/rnfbdemo/blob/main/make-demo.sh (the test harness is locally modified to no longer exclude M1 builds and with a patch-package that implements this PR, pending this merge+release...) Reviewed By: cortinico Differential Revision: D36339335 Pulled By: cipolleschi fbshipit-source-id: d4574fc960e6ff345b31a83ff4629e22edfcf2f7
I've also tested the new versions by setting There's one more bug with Flipper-Folly that prevents RN 0.68.x from being completely compatible with MacCatalyst, but it's another story. |
just to ensure we won't forget about this: does an issue in a repo (either this one or the folly one or the rn one) track this separate issue? |
Could you please elaborate? I'm just about to update my RN app to the latest rn versions and I do need MacCatalyst support here. Which issues are still pending, preventing one to fully work, develop, build, and/or publish in App Store? |
It was mentioned here realm/realm-js#4488 along with my temporary dirty fix. Today I'm going to test it thoroughly on RN 0.69-rc1. If it persists, I'll publish details to the 0.69 discussion. |
Very interesting @Arkkeeper - just deep linking the actual comment since I am here anyway and have it handy, appears to be based on some sort of SharedMutex divergence between FlipperFolly and RCT-Folly realm/realm-js#4488 (comment) |
@mikehardy Anyway, I'm going to PR some fixes for 0.69, so it would work with MacCatalyst out of the box. |
I think the cocoapods version updates are already in process for cherry-picking to 0.69, if that's what you mean but if it's something else, you'll obviously know better :-) Here's what I see in the pick list for 0.69.0-rc.2, the first one is the pod version bump commit
|
I've described the details here reactwg/react-native-releases#21 (comment) @kelset two issues about Flipper-Folly in Release scheme already exist facebook/react-native#33764, facebook/react-native#33753, they are duplicates I'm sorry to say that I've got this errors even on RN 0.69-rc1, trying the fresh new project. |
…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
🐛 Bug Report
FLIPPER FOLLY MAC CATALYST SUPPORT BROKE AFTER SWITCHING TO XCFRAMEWORK. HERE IS WHAT HAPPENED:
There are two bugs when building RN 65+ targeted to MacCatalyst located in Flipper:
1. Time.h redefinition. There is a hack in here in Time.h in RCT which I was able to consistently fix by adding a TARGET_OS_MACCATALYST flag on line 31. See my comment on the quick, robust, fix here.
2. DoubleConversion Not Found when building:
When Flipper/Folly switched to xcFrameworks from the precompiled .a file, it seems they did not include a slice for MacCatalyst. I will include some helpful references here which the team at Flipper who is more familiar with the archiving process of the xcframework can use to quickly rectify this support regression:
a) Mike Hardy's awesome script which may help compile the DoubleConversion Library properly
b) A simple guide to building MacCatalyst slices when using an xcFrameworks
To Reproduce
Try building any react-native on any version 65 or greater with Mac as the target destination (MacCatalyst). You can do that by selecting iPad, then Mac in the General settings of the template HelloWorld Xcode Project. Switch Build Target to Mac at the top.
Recommend using the template in this branch of a release candidate of 67 which adds some robust fixes that will be integrated soon (but you can try on the latest RN 66 build).
Environment
Intel and M1 Macs alike: targeting MacCatalsyt. XCode Version 13.0 (13A233). Any version of React Native above 64, but you can use 66 or the release candidate mention above.
The text was updated successfully, but these errors were encountered: