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

Allow ReactCommon/Folly to be used in Swift libraries (DEFINES_MODULE) #31858

Closed
wants to merge 4 commits into from

Conversation

mrousavy
Copy link
Contributor

@mrousavy mrousavy commented Jul 14, 2021

Summary

Swift Pods require the use of modular headers to be statically linked. To interop with Objective-C modules, you need to make the Objective-C module "define a Module", that is modular header export.

This is already the case for a few podspecs so they can be consumed in Swift libraries, but ReactCommon and RCT-Folly don't do this yet and therefore this breaks in a few libraries of mine, for example see this issue: mrousavy/react-native-vision-camera#195.

If I were to include ReactCommon or RCT-Folly in my Swift library's podspec, the following error arises:

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

The Swift pod `VisionCamera` depends upon `RCT-Folly`, 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.

So this PR fixes this issue by allowing Swift libraries to consume the ReactCommon and RCT-Folly podspecs since they now export modular headers.

Changelog

[General] [Fixed] - Expose Modular Headers for ReactCommon podspec
[General] [Fixed] - Expose Modular Headers for RCT-Folly podspec

Test Plan

Swift Pods require the use of [modular headers](https://blog.cocoapods.org/CocoaPods-1.5.0/) to be statically linked. To interop with Objective-C modules, you need to make the Objective-C module "define a Module", that is modular header export.

This is already the case for a few podspecs so they can be consumed in Swift libraries, but `ReactCommon` doesn't do this yet and therefore breaks in a few libraries of mine, for example see this issue: mrousavy/react-native-vision-camera#195.

If I were to include `ReactCommon` in my Swift library's podspec, the following error arises:

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

The Swift pod `VisionCamera` depends upon `RCT-Folly`, 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.
```

So this PR fixes this issue by allowing Swift libraries to consume the `ReactCommon` podspec since it now exports modular headers.
@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 Jul 14, 2021
@mrousavy mrousavy changed the title Allow ReactCommon to be used in Swift libraries (DEFINES_MODULE) Allow ReactCommon/Folly to be used in Swift libraries (DEFINES_MODULE) Jul 14, 2021
@analysis-bot
Copy link

analysis-bot commented Jul 14, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,142,439 +0
android hermes armeabi-v7a 8,669,412 +0
android hermes x86 9,582,568 +0
android hermes x86_64 9,548,460 +0
android jsc arm64-v8a 10,781,505 +0
android jsc armeabi-v7a 9,699,753 +0
android jsc x86 10,817,031 +0
android jsc x86_64 11,424,434 +0

Base commit: bb33c10

@mrousavy
Copy link
Contributor Author

Why do the tests fail and how can I reproduce this locally?

@matheusmatos
Copy link

@mrousavy it's just a syntax error:

image

the line 37 of ReactCommon.podspec is missing a comma.

@mrousavy
Copy link
Contributor Author

ah, whoops. thanks!

@analysis-bot
Copy link

analysis-bot commented Jul 14, 2021

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

Base commit: bb33c10

@yungsters yungsters added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 10, 2021
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Hi @mrousavy, thanks for taking care of this. Could you please rebase on main and fix the conflict?

Copy link

github-actions bot commented Mar 2, 2024

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Mar 2, 2024
@cipolleschi
Copy link
Contributor

Will take care of apply the same changes on Monday, PR is pretty old, now...(2021)

@mrousavy
Copy link
Contributor Author

mrousavy commented Mar 2, 2024

Hey - whoops sorry forgot to reply here for two years lol - I can rebase on Monday :)

@github-actions github-actions bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Mar 3, 2024
@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.

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Mar 5, 2024
facebook#31858)

Summary:
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. -->

## Summary

Swift Pods require the use of [modular headers](https://blog.cocoapods.org/CocoaPods-1.5.0/) to be statically linked. To interop with Objective-C modules, you need to make the Objective-C module "define a Module", that is modular header export.

This is already the case for a few podspecs so they can be consumed in Swift libraries, but `ReactCommon` and `RCT-Folly` don't do this yet and therefore this breaks in a few libraries of mine, for example see this issue: mrousavy/react-native-vision-camera#195.

If I were to include `ReactCommon` or `RCT-Folly` in my Swift library's podspec, the following error arises:

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

The Swift pod `VisionCamera` depends upon `RCT-Folly`, 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.
```

So this PR fixes this issue by allowing Swift libraries to consume the `ReactCommon` and `RCT-Folly` podspecs since they now export modular headers.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Expose Modular Headers for `ReactCommon` podspec
[General] [Fixed] - Expose Modular Headers for `RCT-Folly` podspec

Pull Request resolved: facebook#31858

Test Plan: * Add s.dependency "ReactCommon" or RCT-Folly to a Swift pod and see what happens. (See mrousavy/react-native-vision-camera#273)

Differential Revision: D54539127

Pulled By: cipolleschi
@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 5, 2024
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 8769245.

cipolleschi pushed a commit that referenced this pull request May 1, 2024
#43327)

Summary:
Pull Request resolved: #43327

<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. -->

## Summary

Swift Pods require the use of [modular headers](https://blog.cocoapods.org/CocoaPods-1.5.0/) to be statically linked. To interop with Objective-C modules, you need to make the Objective-C module "define a Module", that is modular header export.

This is already the case for a few podspecs so they can be consumed in Swift libraries, but `ReactCommon` and `RCT-Folly` don't do this yet and therefore this breaks in a few libraries of mine, for example see this issue: mrousavy/react-native-vision-camera#195.

If I were to include `ReactCommon` or `RCT-Folly` in my Swift library's podspec, the following error arises:

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

The Swift pod `VisionCamera` depends upon `RCT-Folly`, 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.
```

So this PR fixes this issue by allowing Swift libraries to consume the `ReactCommon` and `RCT-Folly` podspecs since they now export modular headers.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Expose Modular Headers for `ReactCommon` podspec
[General] [Fixed] - Expose Modular Headers for `RCT-Folly` podspec

Pull Request resolved: #31858

Test Plan: * Add s.dependency "ReactCommon" or RCT-Folly to a Swift pod and see what happens. (See mrousavy/react-native-vision-camera#273)

Reviewed By: dmytrorykun

Differential Revision: D54539127

Pulled By: cipolleschi

fbshipit-source-id: 2291cc0c8d6675521b220b02ef0c3c6a3e73be38
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. 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