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

Fix pod install for swift libs using new arch #38121

Closed
wants to merge 1 commit into from
Closed

Fix pod install for swift libs using new arch #38121

wants to merge 1 commit into from

Conversation

louiszawadzki
Copy link
Contributor

@louiszawadzki louiszawadzki commented Jun 29, 2023

Summary:

This fixes a bug that started with React Native 0.72.0 when using the new architecture and installing a native lib that has Swift code (in my case, @datadog/mobile-react-native).

Running pod install errors with the following output (DatadogSDKReactNative is the pod containing the Swift code):

[...]
Analyzing dependencies
Downloading dependencies
Installing DatadogSDKReactNative 1.8.0-rc0
[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `DatadogSDKReactNative` depends upon `React-Fabric`, `React-graphics`, `React-utils`, and `React-debug`, which do not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.

Indeed, this pods were added as dependencies in packages/react-native/scripts/cocoapods/new_architecture.rb but do not define modules contrary to the other pods in the list.

This PR is solving a problem that already occured in the past and was solved here: #33743
It's a new implementation for the PR initially opened here: #38039

Changelog:

[IOS] [FIXED] - Fix pod install for libraries using Swift code when the new architecture is enabled

Test Plan:

  1. Clone this repo
  2. From main, add a Swift file to the MyNativeView native module in the RN tester app (see inspiration from this commit)
  3. Try to run RCT_NEW_ARCH_ENABLED=1 USE_HERMES=0 bundle exec pod install inside the packages/rn-tester
  4. Observe errors
  5. Apply the commit from this PR
  6. Both pod install and the subsequent build should succeed.
  7. Revert the changes and repeat steps 2 to 6 with RCT_NEW_ARCH_ENABLED=1 USE_HERMES=1 bundle exec pod install

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 29, 2023
@@ -139,7 +139,7 @@ def use_react_native! (
pod 'Yoga', :path => "#{prefix}/ReactCommon/yoga", :modular_headers => true

pod 'DoubleConversion', :podspec => "#{prefix}/third-party-podspecs/DoubleConversion.podspec"
pod 'glog', :podspec => "#{prefix}/third-party-podspecs/glog.podspec"
pod 'glog', :podspec => "#{prefix}/third-party-podspecs/glog.podspec", :modular_headers => true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When adding "DEFINES_MODULE" => "YES" to s.pod_target_xcconfig in packages/react-native/third-party-podspecs/glog.podspec I still get the following error:

[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `MyNativeView` depends upon `glog`, which does not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, glog is a different beast as it is a c++ library we don't own and that does not actually support Cocoapods. So we are doing some black magic to make it work.

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,001,275 -44,720
android hermes armeabi-v7a 8,261,871 -33,400
android hermes x86 9,514,686 -47,488
android hermes x86_64 9,357,491 -47,033
android jsc arm64-v8a 9,560,258 -44,993
android jsc armeabi-v7a 8,698,149 -33,817
android jsc x86 9,644,525 -47,766
android jsc x86_64 9,891,372 -47,251

Base commit: 0bd6b28
Branch: main

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

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

@cipolleschi merged this pull request in a4a0655.

kelset pushed a commit that referenced this pull request Jul 10, 2023
Summary:
This fixes a bug that started with React Native 0.72.0 when using the new architecture and installing a native lib that has Swift code (in my case, `datadog/mobile-react-native`).

Running `pod install` errors with the following output (`DatadogSDKReactNative` is the pod containing the Swift code):

```
[...]
Analyzing dependencies
Downloading dependencies
Installing DatadogSDKReactNative 1.8.0-rc0
[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `DatadogSDKReactNative` depends upon `React-Fabric`, `React-graphics`, `React-utils`, and `React-debug`, which do not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.
```

Indeed, this pods were added as dependencies in `packages/react-native/scripts/cocoapods/new_architecture.rb` but do not define modules contrary to the other pods in the list.

This PR is solving a problem that already occured in the past and was solved here: #33743
It's a new implementation for the PR initially opened here: #38039

## Changelog:
[IOS] [FIXED] - Fix pod install for libraries using Swift code when the new architecture is enabled

Pull Request resolved: #38121

Test Plan:
1. Clone [this](https://github.com/louiszawadzki/react-native) repo
2. From `main`, add a Swift file to the `MyNativeView` native module in the RN tester app (see inspiration from [this commit](fortmarek@26958fc))
3. Try to run `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=0 bundle exec pod install` inside the `packages/rn-tester`
4. Observe errors
5. Apply [the commit](7b7c3ff) from this PR
6. Both pod install and the subsequent build should succeed.
7. Revert the changes and repeat steps 2 to 6 with `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=1 bundle exec pod install`

Reviewed By: cortinico

Differential Revision: D47127854

Pulled By: cipolleschi

fbshipit-source-id: bf7f65e0d126195a76a0fafbe2f3172f00d5adc1

# Conflicts:
#	packages/react-native/ReactCommon/react/renderer/debug/React-rendererdebug.podspec
Kudo pushed a commit to expo/react-native that referenced this pull request Jul 11, 2023
Summary:
This fixes a bug that started with React Native 0.72.0 when using the new architecture and installing a native lib that has Swift code (in my case, `datadog/mobile-react-native`).

Running `pod install` errors with the following output (`DatadogSDKReactNative` is the pod containing the Swift code):

```
[...]
Analyzing dependencies
Downloading dependencies
Installing DatadogSDKReactNative 1.8.0-rc0
[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `DatadogSDKReactNative` depends upon `React-Fabric`, `React-graphics`, `React-utils`, and `React-debug`, which do not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.
```

Indeed, this pods were added as dependencies in `packages/react-native/scripts/cocoapods/new_architecture.rb` but do not define modules contrary to the other pods in the list.

This PR is solving a problem that already occured in the past and was solved here: facebook#33743
It's a new implementation for the PR initially opened here: facebook#38039

## Changelog:
[IOS] [FIXED] - Fix pod install for libraries using Swift code when the new architecture is enabled

Pull Request resolved: facebook#38121

Test Plan:
1. Clone [this](https://github.com/louiszawadzki/react-native) repo
2. From `main`, add a Swift file to the `MyNativeView` native module in the RN tester app (see inspiration from [this commit](fortmarek@26958fc))
3. Try to run `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=0 bundle exec pod install` inside the `packages/rn-tester`
4. Observe errors
5. Apply [the commit](facebook@7b7c3ff) from this PR
6. Both pod install and the subsequent build should succeed.
7. Revert the changes and repeat steps 2 to 6 with `RCT_NEW_ARCH_ENABLED=1 USE_HERMES=1 bundle exec pod install`

Reviewed By: cortinico

Differential Revision: D47127854

Pulled By: cipolleschi

fbshipit-source-id: bf7f65e0d126195a76a0fafbe2f3172f00d5adc1

# Conflicts:
#	packages/react-native/ReactCommon/react/renderer/debug/React-rendererdebug.podspec
kelset added a commit that referenced this pull request Jul 12, 2023
@kelset
Copy link
Contributor

kelset commented Jul 12, 2023

posting an update here so it's easier to catch up once @cipolleschi is back: this commit only works on Xcode >=14.3, and breaks for both new and old arch for anything lower (see #38294).

To discuss the general situation and approach, there's now this react-native-community/discussions-and-proposals#687 - but I'm writing a comment here because we need to decide two things:

  1. is there a way to change this code to work also in version of Xcode lower than 14.3? if so, we could re-port this in the 0.72 branch with the extra patch for older versions of Xcode
  2. if that's not possible, do we need to revert this also on main?

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants