-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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 build failure on iOS with pnpm and use_frameworks! #38158
Conversation
Hi @evelant! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Base commit: 971bb81 |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there, thank you so much for taking the time of fixing this!
Before importing and merging this, we need to make sure that CI is green again.
I left a couple of questions and suggestions to improve the solution.
Could you give them a shot?
"\"$(PODS_CONFIGURATION_BUILD_DIR)/React-NativeModulesApple/React_NativeModulesApple.framework/Headers/build/generated/ios\"", | ||
"\"$(PODS_CONFIGURATION_BUILD_DIR)/React-Codegen/React_Codegen.framework/Headers/build/generated/ios\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is generating code inside these folders? The default setup for Codegen does not do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure, that's a react-native iOS build detail beyond my understanding.
To make this patch I repeatedly compiled the simple reproduction here https://github.com/evelant/rn072-ios-frameworks-static-failure and noted which pod and headers it was failing on each time. I located the missing header files in the build output and added those locations to the search path.
After repeating the above enough times all the headers resolved correctly and the project compiled successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think it's due to this line https://github.com/facebook/react-native/pull/38158/files#diff-c09e8b79ee60d74ca03bb3960e11fd2155e2391451933b8f6d008a56dfac4e3cR40
When cocoapods copies the headers using the full path for header_mappings_dir
this is where they end up.
This would be great. I have the knowledge of how to set up the CI, but no knowledge on how to run pnpm. 😝 |
@cipolleschi Just the instructions on the reproduction above. |
…ds copying symlinked headers to wrong paths When using pnpm all packages are symlinked to node_modules/.pnpm to prevent phantom dependency resolution. This causes react-native iOS build to fail because Cocoapods copies headers to incorrect destinations when they're behind symlinks. The fix resolves absolute paths to the header_mappings_dir at pod install time. With absolute paths cocoapods copies the headers correctly. This commit also adds a few missing header search paths in use_frameworks! mode.
2a1de02
to
09e1cea
Compare
Sorry about that, somehow my patch didn't get fully applied when I made the commit for this PR. I've fixed it now. |
I understand... I'm not sure that package is available on CircleCI. How do you install |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There are a number of other install methods if they might be more appropriate for CI https://pnpm.io/installation |
This pull request was successfully merged by evelant in 58adc5e. When will my fix make it into a release? | Upcoming Releases |
Summary: Fix build failure on iOS with pnpm and use_frameworks! due to cocoapods copying symlinked headers to wrong paths When using pnpm all packages are symlinked to node_modules/.pnpm to prevent phantom dependency resolution. This causes react-native iOS build to fail because Cocoapods copies headers to incorrect destinations when they're behind symlinks. The fix resolves absolute paths to the header_mappings_dir at pod install time. With absolute paths cocoapods copies the headers correctly. This commit also adds a few missing header search paths in use_frameworks! mode. Fixes #38140 ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [IOS] [FIXED] - Build failure with pnpm and use_frameworks! due to incorrect header paths For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #38158 Test Plan: 1. `pnpm pnpx react-native@latest init AwesomeProject` 2. `cd AwesomeProject` 3. `rm -rf node_modules yarn.lock` 4. `mkdir patches` 5. copy [react-native@0.72.1.patch](https://github.com/facebook/react-native/files/11937570/react-native%400.72.1.patch) to `patches/` 6. Add patch to package.json ``` "pnpm": { "patchedDependencies": { "react-native@0.72.1": "patches/react-native@0.72.1.patch" } } ``` 7. `pnpm install` 8. `cd ios` 9. `NO_FLIPPER=1 USE_FRAMEWORKS=static pod install` 10. `cd ..` 11. `pnpm react-native run-ios` Hopefully an automated test of building with `pnpm` can be added to CI. I don't know how to make that happen, hopefully someone at facebook can do it. Reviewed By: dmytrorykun Differential Revision: D47211946 Pulled By: cipolleschi fbshipit-source-id: 87640bd3f32f023c43291213b5291a7b990d7e1f
Summary: Fix build failure on iOS with pnpm and use_frameworks! due to cocoapods copying symlinked headers to wrong paths When using pnpm all packages are symlinked to node_modules/.pnpm to prevent phantom dependency resolution. This causes react-native iOS build to fail because Cocoapods copies headers to incorrect destinations when they're behind symlinks. The fix resolves absolute paths to the header_mappings_dir at pod install time. With absolute paths cocoapods copies the headers correctly. This commit also adds a few missing header search paths in use_frameworks! mode. Fixes facebook#38140 ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [IOS] [FIXED] - Build failure with pnpm and use_frameworks! due to incorrect header paths For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#38158 Test Plan: 1. `pnpm pnpx react-native@latest init AwesomeProject` 2. `cd AwesomeProject` 3. `rm -rf node_modules yarn.lock` 4. `mkdir patches` 5. copy [react-native@0.72.1.patch](https://github.com/facebook/react-native/files/11937570/react-native%400.72.1.patch) to `patches/` 6. Add patch to package.json ``` "pnpm": { "patchedDependencies": { "react-native@0.72.1": "patches/react-native@0.72.1.patch" } } ``` 7. `pnpm install` 8. `cd ios` 9. `NO_FLIPPER=1 USE_FRAMEWORKS=static pod install` 10. `cd ..` 11. `pnpm react-native run-ios` Hopefully an automated test of building with `pnpm` can be added to CI. I don't know how to make that happen, hopefully someone at facebook can do it. Reviewed By: dmytrorykun Differential Revision: D47211946 Pulled By: cipolleschi fbshipit-source-id: 87640bd3f32f023c43291213b5291a7b990d7e1f
…ook#38158)" This reverts commit 58adc5e.
…" (#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
…" (#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
Summary:
Fix build failure on iOS with pnpm and use_frameworks! due to cocoapods copying symlinked headers to wrong paths
When using pnpm all packages are symlinked to node_modules/.pnpm to prevent phantom dependency resolution. This causes react-native iOS build to fail because Cocoapods copies headers to incorrect destinations when they're behind symlinks. The fix resolves absolute paths to the header_mappings_dir at pod install time. With absolute paths cocoapods copies the headers correctly.
This commit also adds a few missing header search paths in use_frameworks! mode.
Fixes #38140
Changelog:
Test Plan:
pnpm pnpx react-native@latest init AwesomeProject
cd AwesomeProject
rm -rf node_modules yarn.lock
mkdir patches
patches/
pnpm install
cd ios
NO_FLIPPER=1 USE_FRAMEWORKS=static pod install
cd ..
pnpm react-native run-ios
Hopefully an automated test of building with
pnpm
can be added to CI. I don't know how to make that happen, hopefully someone at facebook can do it.