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

Revert "Fix build failure on iOS with pnpm and use_frameworks! (#38158)" #39177

Closed
wants to merge 1 commit into from

Conversation

koenpunt
Copy link
Contributor

@koenpunt koenpunt commented Aug 27, 2023

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
[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

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 27, 2023
@Shahaed
Copy link

Shahaed commented Aug 28, 2023

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
[iOS] [Breaking] - Breaks pnpm support for building iOS project

@cipolleschi
Copy link
Contributor

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?

@facebook-github-bot
Copy link
Contributor

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

@koenpunt
Copy link
Contributor Author

Haven't used pnpm before, so have to try, but in any case, wouldn't that then not also result in a unstable Podfile.lock?

@cipolleschi
Copy link
Contributor

I think that it is unstable because, with Absolute Path, we end up having podspec.json files with full path, like:

<!-- other keys -->
"header_mapping_dir": "/User/<username>/<path>/<to>/<project>/node_modules/<path>/<to>/<header_mapping_dir>"
<!-- other keys -->

The variable part is the /User/<username>/ which is of course always different.

My suggestion is to find a way to get the relative path to <path>/<to>/<project> so that the header mapping dir results in something like:

<!-- 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.
So, in normal projects it would probably be something like:

<!-- 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 .. and/or a different path from node_modules.

We are not using pnpm as well, so it's hard to reproduce also for us, unfortunately.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Aug 30, 2023
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 16fc065.

fortmarek pushed a commit that referenced this pull request Sep 4, 2023
…" (#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
@caustin24345
Copy link

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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.

5 participants