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

Move cocoapods cli native_modules require from template to rn scripts #34215

Closed
wants to merge 1 commit into from

Conversation

danilobuerger
Copy link
Contributor

Summary

This resolves issues where the node_modules structure is not hoisted (like with pnpm). Since the template does not directly depend on the cli, it doesn't exist in the pnpm node_modules root. Moving it to the rn scripts makes sure that the relative require starts in the correct directory for both hoisted and pnpm structures.

Changelog

[iOS] [Fixed] - Fix cocoapods cli native_modules require for pnpm node_modules

Test Plan

  1. react-native init
  2. rm -rf node_modules
  3. pnpm i
  4. bundle install
  5. bundle exec pod install --project-directory=ios

This should succeed. Without the patch, it will fail with

[!] Invalid `Podfile` file: cannot load such file -- /.../node_modules/@react-native-community/cli-platform-ios/native_modules.

 #  from /.../ios/Podfile:2
 #  -------------------------------------------
 #  require_relative '../node_modules/react-native/scripts/react_native_pods'
 >  require_relative '../node_modules/@react-native-community/cli-platform-ios/native_modules'
 #  
 #  -------------------------------------------

@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 Jul 18, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Jul 18, 2022
@analysis-bot
Copy link

analysis-bot commented Jul 18, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,822,695 +0
android hermes armeabi-v7a 7,215,901 +0
android hermes x86 8,135,810 +0
android hermes x86_64 8,113,666 +0
android jsc arm64-v8a 9,699,866 +0
android jsc armeabi-v7a 8,455,322 +0
android jsc x86 9,651,196 +0
android jsc x86_64 10,248,922 +0

Base commit: 79a37e5
Branch: main

@analysis-bot
Copy link

analysis-bot commented Jul 18, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 1d997ce
Branch: main

@cipolleschi
Copy link
Contributor

Hi @danilobuerger, thanks for the PR. The CI is failing, could you please fix it before we can import it internally?

@danilobuerger
Copy link
Contributor Author

Hi @cipolleschi I am trying to figure it out but the error message is not giving me much. Maybe you have an insight? It works locally 🤷‍♂️

@cipolleschi
Copy link
Contributor

It looks like that the template failed to install the pods:
https://app.circleci.com/pipelines/github/facebook/react-native/14190/workflows/3e113393-db07-48a8-82f5-a770aad994e5/jobs/265977?invite=true#step-111-556

My suggestion is to rerun the task in SSH and, once it fails, try to install the pods manually to see what failed.
If it is hermes, because it was built from source, then there is something wrong this morning on other PRs as well. I hope we would be able to fix that soon!

@danilobuerger
Copy link
Contributor Author

For me the "Rerun Job with SSH" button is greyed out with the message "You need write permissions to trigger this build.".

@cipolleschi
Copy link
Contributor

oh... ok. Let me have a look, then!

@cipolleschi
Copy link
Contributor

@danilobuerger The iOS templates fails with:

static:ios distiller$ pod install

[!] Invalid `Podfile` file: cannot load such file -- /Users/distiller/tmp/iOSTemplateProject/node_modules/react-native/scripts/native_modules.

 #  from /Users/distiller/tmp/iOSTemplateProject/ios/Podfile:2
 #  -------------------------------------------
 #  require_relative '../node_modules/react-native/scripts/react_native_pods'
 >  require_relative '../node_modules/react-native/scripts/native_modules'
 #  
 #  -------------------------------------------
static:ios distiller$ 

I think that the path used in the template does not works in this case.

@pull-bot
Copy link

pull-bot commented Jul 19, 2022

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against dcee537

This resolves issues where the node_modules structure is not hoisted (like with pnpm). Since the template does not directly depend on the cli, it doesn't exist in the pnpm node_modules root. Moving it to the rn scripts makes sure that the relative require starts in the correct directory for both hoisted and pnpm structures.
@danilobuerger
Copy link
Contributor Author

@cipolleschi thanks for providing the error message! I was just missing the entry in package.json to make it work. All tests pass now.

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

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this 🙏

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @danilobuerger in af3dfba.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 19, 2022
danilobuerger added a commit to feastr/react-native that referenced this pull request Jul 20, 2022
…facebook#34215)

Summary:
This resolves issues where the node_modules structure is not hoisted (like with pnpm). Since the template does not directly depend on the cli, it doesn't exist in the pnpm node_modules root. Moving it to the rn scripts makes sure that the relative require starts in the correct directory for both hoisted and pnpm structures.

## Changelog

[iOS] [Fixed] - Fix cocoapods cli native_modules require for pnpm node_modules

Pull Request resolved: facebook#34215

Test Plan:
1. react-native init
2. rm -rf node_modules
3. pnpm i
4. bundle install
5. bundle exec pod install --project-directory=ios

This should succeed. Without the patch, it will fail with

```
[!] Invalid `Podfile` file: cannot load such file -- /.../node_modules/react-native-community/cli-platform-ios/native_modules.

 #  from /.../ios/Podfile:2
 #  -------------------------------------------
 #  require_relative '../node_modules/react-native/scripts/react_native_pods'
 >  require_relative '../node_modules/react-native-community/cli-platform-ios/native_modules'
 #
 #  -------------------------------------------
```

Reviewed By: cortinico

Differential Revision: D37959152

Pulled By: cipolleschi

fbshipit-source-id: 7fa9af4a8c153cfd38360f57eca415a8c252dbd5
danilobuerger added a commit to feastr/react-native that referenced this pull request Aug 10, 2022
…facebook#34215)

Summary:
This resolves issues where the node_modules structure is not hoisted (like with pnpm). Since the template does not directly depend on the cli, it doesn't exist in the pnpm node_modules root. Moving it to the rn scripts makes sure that the relative require starts in the correct directory for both hoisted and pnpm structures.

## Changelog

[iOS] [Fixed] - Fix cocoapods cli native_modules require for pnpm node_modules

Pull Request resolved: facebook#34215

Test Plan:
1. react-native init
2. rm -rf node_modules
3. pnpm i
4. bundle install
5. bundle exec pod install --project-directory=ios

This should succeed. Without the patch, it will fail with

```
[!] Invalid `Podfile` file: cannot load such file -- /.../node_modules/react-native-community/cli-platform-ios/native_modules.

 #  from /.../ios/Podfile:2
 #  -------------------------------------------
 #  require_relative '../node_modules/react-native/scripts/react_native_pods'
 >  require_relative '../node_modules/react-native-community/cli-platform-ios/native_modules'
 #
 #  -------------------------------------------
```

Reviewed By: cortinico

Differential Revision: D37959152

Pulled By: cipolleschi

fbshipit-source-id: 7fa9af4a8c153cfd38360f57eca415a8c252dbd5
danilobuerger added a commit to feastr/react-native that referenced this pull request Sep 6, 2022
…facebook#34215)

Summary:
This resolves issues where the node_modules structure is not hoisted (like with pnpm). Since the template does not directly depend on the cli, it doesn't exist in the pnpm node_modules root. Moving it to the rn scripts makes sure that the relative require starts in the correct directory for both hoisted and pnpm structures.

## Changelog

[iOS] [Fixed] - Fix cocoapods cli native_modules require for pnpm node_modules

Pull Request resolved: facebook#34215

Test Plan:
1. react-native init
2. rm -rf node_modules
3. pnpm i
4. bundle install
5. bundle exec pod install --project-directory=ios

This should succeed. Without the patch, it will fail with

```
[!] Invalid `Podfile` file: cannot load such file -- /.../node_modules/react-native-community/cli-platform-ios/native_modules.

 #  from /.../ios/Podfile:2
 #  -------------------------------------------
 #  require_relative '../node_modules/react-native/scripts/react_native_pods'
 >  require_relative '../node_modules/react-native-community/cli-platform-ios/native_modules'
 #
 #  -------------------------------------------
```

Reviewed By: cortinico

Differential Revision: D37959152

Pulled By: cipolleschi

fbshipit-source-id: 7fa9af4a8c153cfd38360f57eca415a8c252dbd5
danilobuerger added a commit to feastr/react-native that referenced this pull request Nov 18, 2022
…facebook#34215)

Summary:
This resolves issues where the node_modules structure is not hoisted (like with pnpm). Since the template does not directly depend on the cli, it doesn't exist in the pnpm node_modules root. Moving it to the rn scripts makes sure that the relative require starts in the correct directory for both hoisted and pnpm structures.

## Changelog

[iOS] [Fixed] - Fix cocoapods cli native_modules require for pnpm node_modules

Pull Request resolved: facebook#34215

Test Plan:
1. react-native init
2. rm -rf node_modules
3. pnpm i
4. bundle install
5. bundle exec pod install --project-directory=ios

This should succeed. Without the patch, it will fail with

```
[!] Invalid `Podfile` file: cannot load such file -- /.../node_modules/react-native-community/cli-platform-ios/native_modules.

 #  from /.../ios/Podfile:2
 #  -------------------------------------------
 #  require_relative '../node_modules/react-native/scripts/react_native_pods'
 >  require_relative '../node_modules/react-native-community/cli-platform-ios/native_modules'
 #
 #  -------------------------------------------
```

Reviewed By: cortinico

Differential Revision: D37959152

Pulled By: cipolleschi

fbshipit-source-id: 7fa9af4a8c153cfd38360f57eca415a8c252dbd5
facebook-github-bot pushed a commit that referenced this pull request Jun 26, 2023
Summary:
Exercise `pod install --project-directory=ios` when building the generated iOS project to make sure we don't regress in the future.

See also #37992, #35754, #34215, https://github.com/facebook/react-native/issues/33909…

## Changelog:

[INTERNAL] [ADDED] - Exercise `pod install --project-directory=ios` when testing the iOS template

Pull Request resolved: #37996

Test Plan: CI should pass.

Reviewed By: dmytrorykun

Differential Revision: D46972815

Pulled By: cipolleschi

fbshipit-source-id: 69617b09ac599eba3dde3ddddcf08db95bfc4da3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 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. Platform: iOS iOS applications. 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.

7 participants