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

Consider relative to pwd installation root when looking for files in rn module via cocoapods #33399

Closed
wants to merge 1 commit into from

Conversation

danilobuerger
Copy link
Contributor

Summary

The :reactNativePath provided by use_native_modules! is the rn module path relative to the installation root (usually ./ios). However, when executing cocoapods from a dir thats not the installation root, packages that use the relative :reactNativePath variable in their path must also consider the relative to pwd installation root.

This fixes usage of cocoapods with the --project-directory flag like

bundle exec pod install --project-directory=ios

Changelog

[iOS] [Fixed] - Fix usage of cocoapods with --project-directory flag and new arch

Test Plan

  1. Enable the new arch
  2. Execute from the projects root dir
bundle exec pod install --project-directory=ios
  1. It will fail with
[!] Invalid `Podfile` file: [codegen] Couldn't not find react-native-codegen..
  1. Apply the patch
  2. Execute from the projects root dir
bundle exec pod install --project-directory=ios
  1. It will succeed

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

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

Base commit: bb8ff92
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,214,074 +0
android hermes armeabi-v7a 7,799,789 +0
android hermes x86 8,587,551 +0
android hermes x86_64 8,538,498 +0
android jsc arm64-v8a 9,882,396 +0
android jsc armeabi-v7a 8,852,646 +0
android jsc x86 9,851,969 +0
android jsc x86_64 10,447,035 +0

Base commit: bb8ff92
Branch: main

…rn module via cocoapods

The :reactNativePath provided by use_native_modules! is the rn module path relative to the installation root (usually ./ios). However, when executing cocoapods from a dir thats not the installation root, packages that use the relative :reactNativePath variable in their path must also consider the relative to pwd installation root.

This fixes usage of cocoapods with the --project-directory flag like

bundle exec pod install --project-directory=ios
@cortinico
Copy link
Contributor

Ping @dmitryrykun as he has relevant context on this. He already looked into relative path support for new arch so might be able to support here.

Copy link
Contributor

@dmytrorykun dmytrorykun left a comment

Choose a reason for hiding this comment

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

Great!

@facebook-github-bot
Copy link
Contributor

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

codegen_repo_path = "#{react_native_path}/packages/react-native-codegen";
codegen_npm_path = "#{react_native_path}/../react-native-codegen";
relative_installation_root = Pod::Config.instance.installation_root.relative_path_from(Pathname.pwd)
codegen_repo_path = "#{relative_installation_root}/#{react_native_path}/packages/react-native-codegen";
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @danilobuerger! Is react_native_path guaranteed to be relative? Does this work if it is absolute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmitryrykun guaranteed might be a bit too strong 😅 But currently its always returned as a relative path from use_native_modules!. See https://github.com/react-native-community/cli/blob/master/packages/platform-ios/native_modules.rb#L124

Also the rest of the code assumes its relative too: For example

# react_native_path should be relative already.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @danilobuerger in 2f813f8.

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 Mar 14, 2022
@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by 06f504b.

@danilobuerger
Copy link
Contributor Author

Hi @cortinico , @dmitryrykun 👋, any idea why this was reverted?

@dmytrorykun
Copy link
Contributor

Hi @danilobuerger , the PR itself is okay. I'm not sure what happened. I probably did something wrong during merge, and this PR landed in conjunction with other changes, that broke our internal CI. I'll investigate this tomorrow. Sorry for the confusion.

cortinico pushed a commit that referenced this pull request Mar 15, 2022
… files in rn module via cocoapods (#33399)

Summary:
The `:reactNativePath` provided by `use_native_modules!` is the rn module path relative to the installation root (usually `./ios`). However, when executing cocoapods from a dir thats not the installation root, packages that use the relative `:reactNativePath` variable in their path must also consider the relative to pwd installation root.

This fixes usage of cocoapods with the `--project-directory` flag like

```bash
bundle exec pod install --project-directory=ios
```

[iOS] [Fixed] - Fix usage of cocoapods with --project-directory flag and new arch

Pull Request resolved: #33399

Test Plan:
1) Enable the new arch
2) Execute from the projects root dir

```bash
bundle exec pod install --project-directory=ios
```

3) It will fail with

```
[!] Invalid `Podfile` file: [codegen] Couldn't not find react-native-codegen..
```

4) Apply the patch
5) Execute from the projects root dir

```bash
bundle exec pod install --project-directory=ios
```

6) It will succeed
@cortinico
Copy link
Contributor

that broke our internal CI

That was the reason essentially. It was reverted by another engineer. We can safely re-apply it now: #33427

Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…rn module via cocoapods (facebook#33399)

Summary:
The `:reactNativePath` provided by `use_native_modules!` is the rn module path relative to the installation root (usually `./ios`). However, when executing cocoapods from a dir thats not the installation root, packages that use the relative `:reactNativePath` variable in their path must also consider the relative to pwd installation root.

This fixes usage of cocoapods with the `--project-directory` flag like

```bash
bundle exec pod install --project-directory=ios
```

## Changelog

[iOS] [Fixed] - Fix usage of cocoapods with --project-directory flag and new arch

Pull Request resolved: facebook#33399

Test Plan:
1) Enable the new arch
2) Execute from the projects root dir

```bash
bundle exec pod install --project-directory=ios
```

3) It will fail with

```
[!] Invalid `Podfile` file: [codegen] Couldn't not find react-native-codegen..
```

4) Apply the patch
5) Execute from the projects root dir

```bash
bundle exec pod install --project-directory=ios
```

6) It will succeed

Reviewed By: ShikaSD

Differential Revision: D34784966

Pulled By: dmitryrykun

fbshipit-source-id: d6d5e71bc2fcd32f2cd60a498f39e6f772fc9005
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. Reverted 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.

6 participants