-
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
Revert "Fix build failure on iOS with pnpm and use_frameworks! (#38158)" #39177
Conversation
…ook#38158)" This reverts commit 58adc5e.
For anyone that's come across this before the fix is merged, reverting to < v0.72.2 should fix this. @cipolleschi approved the PR that caused this regression, so I'm tagging him here. @koenpunt thanks for the PR, tested it and can verify it fixes the checksum in the podfile.lock issue and fixes our CI. Since the github action checks requires a Changelog before going green, feel free to add this: Changelog[iOS] [Fixed] - Fix conflicting hashes in pods lockfile from absolute paths in podspec files |
This makes sense to me, but it will break again the pnpm setup. I'm thinking whether we can find an alternative way to do that. For example, passing down the relative path to where the pods are copied... which is an empty string in case of stadard node and a proper value in case of pnpm. WDYT? |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Haven't used pnpm before, so have to try, but in any case, wouldn't that then not also result in a unstable |
I think that it is unstable because, with Absolute Path, we end up having <!-- other keys -->
"header_mapping_dir": "/User/<username>/<path>/<to>/<project>/node_modules/<path>/<to>/<header_mapping_dir>"
<!-- other keys --> The variable part is the My suggestion is to find a way to get the relative path to <!-- other keys -->
"header_mapping_dir": "/<relative>/<path>/<to>/<project>/node_modules/<path>/<to>/<header_mapping_dir>"
<!-- other keys --> In this case the relative path to would always be the same because it will be computed by cocoapods using the same setup that you are running locally and in CI. <!-- other keys -->
"header_mapping_dir": "../node_modules/react-native/<header_mapping_dir>"
<!-- other keys --> And in pnpm setup it would have a different number of We are not using pnpm as well, so it's hard to reproduce also for us, unfortunately. |
@cipolleschi merged this pull request in 16fc065. |
…" (#39177) Summary: This partially reverts commit 58adc5e. Using an absolute path in a podspec is wrong, and causes checksum issues when installs are ran on different systems. Example: `pod install --deployment --clean-install` breaks on non-matching checksums on CI because the absolute path is not the same in that environment. ``` Adding spec repo `trunk` with CDN `https://cdn.cocoapods.org/` Verifying no changes [!] There were changes to the lockfile in deployment mode: SPEC CHECKSUMS: React-debug: New Lockfile: 419922cde6c58cd5b9d43e4a09805146a7dd13a8 Old Lockfile: 1ce329843d8f9a9cbe0cdd9de264b20e89586734 React-NativeModulesApple: New Lockfile: a683b0c999e76b7d15ad9d5eaf8b6308e710a93e Old Lockfile: f82356d67a137295d098a98a03be5ee35871b5a5 React-runtimescheduler: New Lockfile: 79f8dff11acbe36aaeece63553680d7a8272af96 Old Lockfile: 16c5282d43a0df50d7c68ebf0218aeeb642a7086 React-utils: New Lockfile: 4fabb3cba786651e35bc41e610b0698fa24cecff Old Lockfile: e7e9118d0e85b107bb06d1a5f72ec5db6bddb911 ReactCommon: New Lockfile: af30fb021799e18c85a8e30ce799e15607e82212 Old Lockfile: f04f86f33c22e05dbf875789ea522ee486dace78 ``` And even tho the change fixed an issue with pnpm, the issue it introduces with cocoapods I think is a bigger one. ## Changelog: [INTERNAL] - Revert commit that makes podfile unstable Pull Request resolved: #39177 Test Plan: Tested locally that the hashes are stable Reviewed By: NickGerleman Differential Revision: D48773887 Pulled By: cipolleschi fbshipit-source-id: 96bcdbadc17a24fa9a8669f569d004bee6a03521
@cipolleschi - I'm wondering if pnpm compatibility is something that will be taken into consideration by the RN team in the future. I am maintaining an enterprise RN project which uses pnpm and expo, and have started running into build issues after upgrading to expo 51 (react-native 0.74). Would it be in my best interest to change package managers to something like npm or yarn going forwards - or do you think patching the react-native package locally to support pnpm would be a stable solution? |
Summary:
This partially reverts commit 58adc5e.
Using an absolute path in a podspec is wrong, and causes checksum issues when installs are ran on different systems.
Example:
pod install --deployment --clean-install
breaks on non-matching checksums on CI because the absolute path is not the same in that environment.And even tho the change fixed an issue with pnpm, the issue it introduces with cocoapods I think is a bigger one.
Changelog:
[INTERNAL] - Revert commit that makes podfile unstable
[iOS] [Fixed] - Fix conflicting hashes in pods lockfile from absolute paths in podspec files
[iOS] [Breaking] - Breaks pnpm support for building iOS project
Test Plan:
Tested locally that the hashes are stable