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

[ios] Fix broken use_frameworks from React-bridging #34011

Closed
wants to merge 2 commits into from

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Jun 14, 2022

Summary

use_frameworks! is broken again in react-native 0.69 because React-bridging. in the use_frameworks! mode, header structures are flattened, so #include <react/bridging/CallbackWrapper.h> is not reachable to the header. to somehow workaround the issue without touch React-bridging imports, the pr do these things:

  • use header_mappings_dir to keep react/bridging header structure
  • because the header structure is not default framework header structure, explicitly HEADER_SEARCH_PATHS is necessary.
  • forward declare CallbackWrapper and use it internally in ReactCommon. so that we don't need to add HEADER_SEARCH_PATHS for React-bridging to every pods depending on ReactCommon/turbomodule/core, e.g. React-RCTSettings.podspec.

Changelog

[iOS] [Fixed] - Fix use_frameworks! for 0.69

Test Plan

$ npx react-native init RN069 --version next
# add `use_frameworks!` to ios/Podsfile
# comment out use_flipper!() in ios/Podfile
# patch node_modules/react-native with these changes
$ yarn ios

@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. Contributor A React Native contributor. p: Expo Partner: Expo Partner labels Jun 14, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Jun 14, 2022
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,787,847 +0
android hermes armeabi-v7a 7,176,742 +0
android hermes x86 8,097,679 +0
android hermes x86_64 8,075,549 +0
android jsc arm64-v8a 9,654,628 +0
android jsc armeabi-v7a 8,412,418 +0
android jsc x86 9,605,227 +0
android jsc x86_64 10,200,101 +0

Base commit: bcc69df
Branch: main

@analysis-bot
Copy link

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

Base commit: bcc69df
Branch: main

@cortinico
Copy link
Contributor

cc @dmitryrykun

@Kudo Kudo marked this pull request as ready for review June 15, 2022 02:52
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jun 15, 2022
@Kudo
Copy link
Contributor Author

Kudo commented Jun 15, 2022

thanks @cortinico @dmitryrykun. this is the minimum impact solution i can do so far. let me know whether it makes sense to you.
basically, the nested include or import like #include <react/bridging/CallbackWrapper.h> or #include <react/renderer/uimanager/primitives.h> will break use_frameworks. in framework header search, the formal way is #import <Framework/Header.h>. that's basically the main reason why use_frameworks is broken in new architecture mode. hopefully you can have some discussion for the nested import inside Meta.

@dmytrorykun
Copy link
Contributor

Great work @Kudo, thanks!

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

facebook-github-bot pushed a commit that referenced this pull request Jun 21, 2022
…abled (#34030)

Summary:
This PR is fixing the build errors on iOS when `use_frameworks!` and `:hermes_enabled` are both enabled. There are two errors:

- fmt/compile.h include not found: This PR adds fmt in header search paths.
- undefined symbols `_jump_fcontext` and `_make_fcontext` from boost. the two symbols are actually not be unused. because to generate the shared library in dynamic framework mode, LTO (Link-Time-Optimization) is not as powerful as generating a single executable.

## Changelog

[iOS] [Fixed] - Fix RCT-Folly build error when use_frameworks! and hermes are both enabled

Pull Request resolved: #34030

Test Plan:
- CI passed

-
```
$ npx react-native init RN069 --version next
# edit RN069/ios/Podfile to enable use_frameworks! and hermes_enabled
# patch node_modules/react-native from both #34011 and this prs' patch
$ pod install
$ yarn ios
```

Reviewed By: cortinico

Differential Revision: D37284084

Pulled By: dmitryrykun

fbshipit-source-id: 923fa03d7844d1d227880919c8b2c8614c848d59
@Kudo
Copy link
Contributor Author

Kudo commented Jun 21, 2022

cool. the internal CI failed from Meta looks success suddenly. that should be you folks from Meta helping. hopefully we can fix the use_frameworks problems soon.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @Kudo in f97c6a5.

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 Jun 21, 2022
Kudo added a commit to expo/react-native that referenced this pull request Jun 23, 2022
…abled (facebook#34030)

Summary:
This PR is fixing the build errors on iOS when `use_frameworks!` and `:hermes_enabled` are both enabled. There are two errors:

- fmt/compile.h include not found: This PR adds fmt in header search paths.
- undefined symbols `_jump_fcontext` and `_make_fcontext` from boost. the two symbols are actually not be unused. because to generate the shared library in dynamic framework mode, LTO (Link-Time-Optimization) is not as powerful as generating a single executable.

## Changelog

[iOS] [Fixed] - Fix RCT-Folly build error when use_frameworks! and hermes are both enabled

Pull Request resolved: facebook#34030

Test Plan:
- CI passed

-
```
$ npx react-native init RN069 --version next
# edit RN069/ios/Podfile to enable use_frameworks! and hermes_enabled
# patch node_modules/react-native from both facebook#34011 and this prs' patch
$ pod install
$ yarn ios
```

Reviewed By: cortinico

Differential Revision: D37284084

Pulled By: dmitryrykun

fbshipit-source-id: 923fa03d7844d1d227880919c8b2c8614c848d59
(cherry picked from commit 79baca6)
Kudo added a commit to expo/react-native that referenced this pull request Jun 23, 2022
Summary:
`use_frameworks!` is broken again in react-native 0.69 because React-bridging. in the `use_frameworks!` mode, header structures are flattened, so `#include <react/bridging/CallbackWrapper.h>` is not reachable to the header. to somehow workaround the issue without touch React-bridging imports, the pr do these things:

- use `header_mappings_dir` to keep `react/bridging` header structure
- because the header structure is not default framework header structure, explicitly `HEADER_SEARCH_PATHS` is necessary.
- forward declare `CallbackWrapper` and use it internally in ReactCommon. so that we don't need to add `HEADER_SEARCH_PATHS` for React-bridging to every pods depending on `ReactCommon/turbomodule/core`, e.g. React-RCTSettings.podspec.

[iOS] [Fixed] - Fix use_frameworks! for 0.69

Pull Request resolved: facebook#34011

Test Plan:
```sh
$ npx react-native init RN069 --version next
$ yarn ios
```

Reviewed By: cortinico, cipolleschi

Differential Revision: D37169699

Pulled By: dmitryrykun

fbshipit-source-id: 309c55f1c611a2fc3902a83e8af814daaf2af6a0
(cherry picked from commit f97c6a5)
@imWildCat
Copy link

imWildCat commented Jun 25, 2022

Hello folks, I don't think this fix in in 0.69.0 as of now? Do we need a quick bump to include it?

➜  react-native git:(main) echo $(git tag --contains f97c6a5b498eec95e99a02c7842cb2ae160cd6cd)

➜  react-native git:(main)

@fortmarek
Copy link
Contributor

fortmarek commented Jun 25, 2022

Hey @imWildCat 👋

This is already in the list of commits for 0.69.1 release.

@imWildCat
Copy link

@fortmarek thanks for your prompt reply! I subscribed this discussion.

@mikehardy
Copy link
Contributor

@imWildCat you might like https://github.com/mikehardy/rnfbdemo/blob/main/patches/react-native%2B0.69.0.patch

@linear linear bot mentioned this pull request Jun 28, 2022
4 tasks
fortmarek pushed a commit that referenced this pull request Jun 29, 2022
Summary:
`use_frameworks!` is broken again in react-native 0.69 because React-bridging. in the `use_frameworks!` mode, header structures are flattened, so `#include <react/bridging/CallbackWrapper.h>` is not reachable to the header. to somehow workaround the issue without touch React-bridging imports, the pr do these things:

- use `header_mappings_dir` to keep `react/bridging` header structure
- because the header structure is not default framework header structure, explicitly `HEADER_SEARCH_PATHS` is necessary.
- forward declare `CallbackWrapper` and use it internally in ReactCommon. so that we don't need to add `HEADER_SEARCH_PATHS` for React-bridging to every pods depending on `ReactCommon/turbomodule/core`, e.g. React-RCTSettings.podspec.

[iOS] [Fixed] - Fix use_frameworks! for 0.69

Pull Request resolved: #34011

Test Plan:
```sh
$ npx react-native init RN069 --version next
$ yarn ios
```

Reviewed By: cortinico, cipolleschi

Differential Revision: D37169699

Pulled By: dmitryrykun

fbshipit-source-id: 309c55f1c611a2fc3902a83e8af814daaf2af6a0
fortmarek pushed a commit that referenced this pull request Jun 29, 2022
…abled (#34030)

Summary:
This PR is fixing the build errors on iOS when `use_frameworks!` and `:hermes_enabled` are both enabled. There are two errors:

- fmt/compile.h include not found: This PR adds fmt in header search paths.
- undefined symbols `_jump_fcontext` and `_make_fcontext` from boost. the two symbols are actually not be unused. because to generate the shared library in dynamic framework mode, LTO (Link-Time-Optimization) is not as powerful as generating a single executable.

## Changelog

[iOS] [Fixed] - Fix RCT-Folly build error when use_frameworks! and hermes are both enabled

Pull Request resolved: #34030

Test Plan:
- CI passed

-
```
$ npx react-native init RN069 --version next
# edit RN069/ios/Podfile to enable use_frameworks! and hermes_enabled
# patch node_modules/react-native from both #34011 and this prs' patch
$ pod install
$ yarn ios
```

Reviewed By: cortinico

Differential Revision: D37284084

Pulled By: dmitryrykun

fbshipit-source-id: 923fa03d7844d1d227880919c8b2c8614c848d59
christophpurrer added a commit to christophpurrer/react-native-macos that referenced this pull request Nov 5, 2022
Summary:
A previous change - facebook#34011 - already fixed basic usage of <react/bridging/.../ imports.
However that change was only tailored towards the usage of: <react/bridging/CallbackWrapper.h>

For C++ TurboModules we need to be able to access *any* <react/bridging/...> header via the React-Codegen CocoaPod.
Hence adding bridging now as a sub-spec to the ReactCommon CocoaPod

Changelog: [Internal]

Differential Revision: D41057878

fbshipit-source-id: 41d5e38d298695fe2084fd650b5cc8abc740217b
christophpurrer added a commit to christophpurrer/react-native-macos that referenced this pull request Nov 5, 2022
Summary:
Pull Request resolved: facebook#35212

A previous change - facebook#34011 - already fixed basic usage of <react/bridging/.../ imports.
However that change was only tailored towards the usage of: <react/bridging/CallbackWrapper.h>

For C++ TurboModules we need to be able to access *any* <react/bridging/...> header via the React-Codegen CocoaPod.
Hence adding bridging now as a sub-spec to the ReactCommon CocoaPod

Changelog: [Internal]

Differential Revision: D41057878

fbshipit-source-id: b62308ebc7fe450092981c991a73d85b5a6e8e50
christophpurrer added a commit to christophpurrer/react-native-macos that referenced this pull request Nov 5, 2022
Summary:
Pull Request resolved: facebook#35212

A previous change - facebook#34011 - already fixed basic usage of <react/bridging/.../ imports.
However that change was only tailored towards the usage of: <react/bridging/CallbackWrapper.h>

For C++ TurboModules we need to be able to access *any* <react/bridging/...> header via the React-Codegen CocoaPod.
Hence adding bridging now as a sub-spec to the ReactCommon CocoaPod

Changelog: [Internal]

Differential Revision: D41057878

fbshipit-source-id: c2b5299cbdfa3be4b8590a968470f5e0338eb6e8
christophpurrer added a commit to christophpurrer/react-native-macos that referenced this pull request Nov 5, 2022
Summary:
Pull Request resolved: facebook#35212

A previous change - facebook#34011 - already fixed basic usage of <react/bridging/.../ imports.
However that change was only tailored towards the usage of: <react/bridging/CallbackWrapper.h>
Any other header besides <react/bridging/CallbackWrapper.h> from <react/bridging/... can't be imported at this in Xcode .... which is bad.

For C++ TurboModules we need to be able to access *any* <react/bridging/...> header via the React-Codegen CocoaPod.
Hence adding bridging now as a sub-spec to the ReactCommon CocoaPod

Changelog: [Internal]

Differential Revision: D41057878

fbshipit-source-id: 13995636132576cbe325473db667e45aea3cf489
christophpurrer added a commit to christophpurrer/react-native-macos that referenced this pull request Nov 7, 2022
Summary:
Pull Request resolved: facebook#35212

A previous change - facebook#34011 - already fixed basic usage of <react/bridging/.../ imports.
However that change was only tailored towards the usage of: <react/bridging/CallbackWrapper.h>
Any other header besides <react/bridging/CallbackWrapper.h> from <react/bridging/... can't be imported at this time in Xcode ... ... which is bad.

For C++ TurboModules we need to be able to access *any* <react/bridging/...> header via the React-Codegen CocoaPod.
Hence adding bridging now as a sub-spec to the ReactCommon CocoaPod

Changelog: [Internal]

Differential Revision: D41057878

fbshipit-source-id: 9125933c3590ac26ea51c66c5fd8d1a215e9f0d8
facebook-github-bot pushed a commit that referenced this pull request Nov 7, 2022
Summary:
Pull Request resolved: #35212

A previous change - #34011 - already fixed basic usage of <react/bridging/.../ imports.
However that change was only tailored towards the usage of: <react/bridging/CallbackWrapper.h>
Any other header besides <react/bridging/CallbackWrapper.h> from <react/bridging/... can't be imported at this time in Xcode ... ... which is bad.

For C++ TurboModules we need to be able to access *any* <react/bridging/...> header via the React-Codegen CocoaPod.
Hence adding bridging now as a sub-spec to the ReactCommon CocoaPod

Changelog: [Internal]

Reviewed By: cipolleschi

Differential Revision: D41057878

fbshipit-source-id: 83c117bc5252d84dd419cdb72f145f65547d23b2
kelset pushed a commit that referenced this pull request Nov 22, 2022
Summary:
Pull Request resolved: #35212

A previous change - #34011 - already fixed basic usage of <react/bridging/.../ imports.
However that change was only tailored towards the usage of: <react/bridging/CallbackWrapper.h>
Any other header besides <react/bridging/CallbackWrapper.h> from <react/bridging/... can't be imported at this time in Xcode ... ... which is bad.

For C++ TurboModules we need to be able to access *any* <react/bridging/...> header via the React-Codegen CocoaPod.
Hence adding bridging now as a sub-spec to the ReactCommon CocoaPod

Changelog: [Internal]

Reviewed By: cipolleschi

Differential Revision: D41057878

fbshipit-source-id: 83c117bc5252d84dd419cdb72f145f65547d23b2

# Conflicts:
#	scripts/cocoapods/__tests__/codegen_utils-test.rb
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Pull Request resolved: facebook#35212

A previous change - facebook#34011 - already fixed basic usage of <react/bridging/.../ imports.
However that change was only tailored towards the usage of: <react/bridging/CallbackWrapper.h>
Any other header besides <react/bridging/CallbackWrapper.h> from <react/bridging/... can't be imported at this time in Xcode ... ... which is bad.

For C++ TurboModules we need to be able to access *any* <react/bridging/...> header via the React-Codegen CocoaPod.
Hence adding bridging now as a sub-spec to the ReactCommon CocoaPod

Changelog: [Internal]

Reviewed By: cipolleschi

Differential Revision: D41057878

fbshipit-source-id: 83c117bc5252d84dd419cdb72f145f65547d23b2
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. Contributor A React Native contributor. Merged This PR has been merged. p: Expo Partner: Expo Partner 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.

9 participants