-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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 ios build error when use_frameworks is on and fabric is off #33379
Conversation
Base commit: 5386364 |
Base commit: 5386364 |
@Kudo this may be an odd request and/or difficult - if so please ignore - but if you still have this file tree open + clean somewhere, I'd love a diff of this so I could apply locally via patch-package and test in my reproduction of the same issue, then I could confirm it works as well If that's not easy for any reason just let me know, the changes are pretty easy and though it would be slightly more error-prone than a diff / patch-package I could apply them locally as well for testing + confirmation |
@mikehardy thanks for your help. this commit patch should be good enough for you to verify. |
Oh of course github generates them 🤦 😆 |
This patch is from facebook/react-native#33379 and fixes static frameworks build Thanks @Kudo!
Very tiny differences between the github PR patch and what gets installed in an app, but nothing that should alter validity of test results, here's what it looked like after pulling the patch down and applying it manually inside node_modules against react-native 0.68.0-rc.2:
Resulting patch file (for use with patch-package) for anyone else that wants to test is With the patch integrated, my static frameworks build test now passes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this change via the patch I attached in comment on the PR, for these scenarios [edit: on an intel mac], they all worked:
- fabric disabled, hermes/flipper enabled, release mode
- fabric disabled, hermes/flipper enabled, debug mode
- fabric disabled, hermes/flipper disabled, static frameworks, debug mode
The matrix of possibilities for compiles is pretty big at this point now that I think about it! But these are my normal cases and represent the normal cases for react-native-firebase users so they are what I test.
All works, and the PR looks reasonable
This patch is from facebook/react-native#33379 and fixes static frameworks build Thanks @Kudo!
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Just wanted to add an update that I tested the same set of combinations but on an M1 mac as well (previous comment was an intel mac, sorry I did not specify), and everything worked on it as well, same ✔️ results. |
i am going to rebase main again to fix the conflicts. |
I suppose this is going to be tricky to apply to 0.68 then, because your previous diff applied against the 0.68 branch - if there were conflicts with main it may require 2 PRs ? (one against main, one against 68 branch?). It definitely worked against 0.68 though, I should be specific and say all my testing was applying diff via patch tools against 0.68.0-rc.2 |
the conflict is just one line difference introduced from c2e4ae3. i doubt the pr may break react-native internal BUCK builder because the |
React/CoreModules/RCTSourceCode.mm
Outdated
@@ -7,7 +7,7 @@ | |||
|
|||
#import "RCTSourceCode.h" | |||
|
|||
#import <FBReactNativeSpec/FBReactNativeSpec.h> | |||
#import <ReactCodegen/FBReactNativeSpec/FBReactNativeSpec.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting here but this applies to the whole PR.
Sadly, we can't merge this as it is, and we have to find a different approach to this problem. Specifically this is breaking our internal setup (i.e. building React Native iOS with Buck, which sadly we don't use in OSS).
Specifically here we fail internally with:
xplat/js/react-native-github/React/CoreModules/RCTSourceCode.mm:10:9: fatal error: 'ReactCodegen/FBReactNativeSpec/FBReactNativeSpec.h' file not found
#import <ReactCodegen/FBReactNativeSpec/FBReactNativeSpec.h>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
EDIT: typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, i believed this change would break Buck builder 🤔 could you share more insight for how the Buck targets organized internally? from podspec of view, that's be little weird for FBReactNativeSpec.podspec
doesn't have any source_files and React-Codegen
puts source_files to FBReactNativeSpec
import namespace. is it possible to merge these two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ideally we can't change those #import <FBReactNativeSpec/FBReactNativeSpec.h>
statements as our internal builds rely on them. I'd try to tweak the Podspecs as much as possible to tackle this.
Sadly this is a bigger problem, that is affecting how we do iOS in OSS, and we should look into this problem from a more holistic point of view.
For the sake of 0.68.x, if we could find a patch on the Podspec level, that would be awesome.
cc @cipolleschi @dmitryrykun for visiblity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the pr to keep #import <FBReactNativeSpec/FBReactNativeSpec.h>
imports. please take a look when you get a chance.
this diff is for 0.68.0-rc.2 in case someone need to verify: https://gist.github.com/Kudo/17dde98d7721601697d2dc05ec1dcba1
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…33409) Summary: alternative solution for #33379 > when `use_frameworks!` is on, there are errors like: > ``` > 'FBReactNativeSpec/FBReactNativeSpec.h' file not found > #import <FBReactNativeSpec/FBReactNativeSpec.h> > ``` > this error may come from from f7e4c07c84b6 regression. > > when `use_frameworks!` is on, xcode will search headers from framework directories, the correct imports would be `#import <React_Codegen/FBReactNativeSpec/FBReactNativeSpec.h>` (xcode will transform dash to underscore, so it is `React_Codegen` but not `React-Codegen`). in the other hand, when `use_frameworks!` is off, the correct import is `#import <React-Codegen/FBReactNativeSpec/FBReactNativeSpec.h>`. > > > this fix is specific for old architecture (fabric is off). > > when fabric is on, there are other errors from duplicated headers when copying to build folder. [the reason is that framework build would try to flatten headers](https://mkonrad.net/2015/03/29/xcode-static-libraries-preserving-header-directory-structure.html). we have `primitives.h` in different folders and they would be flattened into `React_Fabric.framework/Headers`. to be honest, i don't know how to deal with the problem in the meantime, maybe subspecs are not enough, we should separate them from subspecs to dedicated podspecs so that we can have these targets as different frameworks. in this alternative fix, i try to add `React-Codegen/React_Codegen.framework/Headers` into header search paths and make original `#import <FBReactNativeSpec/FBReactNativeSpec.h>` reachable. [this change](7a0398c) in the pr is just a workaround to solve breaking in latest main branch and this is not important to the `use_frameworks!` fix at all. this breaking was coming from 1804951. ## Changelog [iOS] [Fixed] - Fix iOS build error when Podfile `use_frameworks!` is on and Fabric is off Pull Request resolved: #33409 Test Plan: verify with rn-tester 1. change `fabric_enabled` to false in `packages/rn-tester/Podfile` 2. `USE_FRAMEWORKS=1 pod install` 3. build rn-tester in xcode Reviewed By: dmitryrykun Differential Revision: D34817041 Pulled By: cortinico fbshipit-source-id: 4d1a610e99a807793eb3f64461e0d735c0a9ca9c
Closing as
|
…33409) Summary: alternative solution for #33379 > when `use_frameworks!` is on, there are errors like: > ``` > 'FBReactNativeSpec/FBReactNativeSpec.h' file not found > #import <FBReactNativeSpec/FBReactNativeSpec.h> > ``` > this error may come from from f7e4c07c84b6 regression. > > when `use_frameworks!` is on, xcode will search headers from framework directories, the correct imports would be `#import <React_Codegen/FBReactNativeSpec/FBReactNativeSpec.h>` (xcode will transform dash to underscore, so it is `React_Codegen` but not `React-Codegen`). in the other hand, when `use_frameworks!` is off, the correct import is `#import <React-Codegen/FBReactNativeSpec/FBReactNativeSpec.h>`. > > > this fix is specific for old architecture (fabric is off). > > when fabric is on, there are other errors from duplicated headers when copying to build folder. [the reason is that framework build would try to flatten headers](https://mkonrad.net/2015/03/29/xcode-static-libraries-preserving-header-directory-structure.html). we have `primitives.h` in different folders and they would be flattened into `React_Fabric.framework/Headers`. to be honest, i don't know how to deal with the problem in the meantime, maybe subspecs are not enough, we should separate them from subspecs to dedicated podspecs so that we can have these targets as different frameworks. in this alternative fix, i try to add `React-Codegen/React_Codegen.framework/Headers` into header search paths and make original `#import <FBReactNativeSpec/FBReactNativeSpec.h>` reachable. [this change](7a0398c) in the pr is just a workaround to solve breaking in latest main branch and this is not important to the `use_frameworks!` fix at all. this breaking was coming from 1804951. [iOS] [Fixed] - Fix iOS build error when Podfile `use_frameworks!` is on and Fabric is off Pull Request resolved: #33409 Test Plan: verify with rn-tester 1. change `fabric_enabled` to false in `packages/rn-tester/Podfile` 2. `USE_FRAMEWORKS=1 pod install` 3. build rn-tester in xcode Reviewed By: dmitryrykun Differential Revision: D34817041 Pulled By: cortinico fbshipit-source-id: 4d1a610e99a807793eb3f64461e0d735c0a9ca9c
…acebook#33409) Summary: alternative solution for facebook#33379 > when `use_frameworks!` is on, there are errors like: > ``` > 'FBReactNativeSpec/FBReactNativeSpec.h' file not found > #import <FBReactNativeSpec/FBReactNativeSpec.h> > ``` > this error may come from from facebook@f7e4c07c84b6 regression. > > when `use_frameworks!` is on, xcode will search headers from framework directories, the correct imports would be `#import <React_Codegen/FBReactNativeSpec/FBReactNativeSpec.h>` (xcode will transform dash to underscore, so it is `React_Codegen` but not `React-Codegen`). in the other hand, when `use_frameworks!` is off, the correct import is `#import <React-Codegen/FBReactNativeSpec/FBReactNativeSpec.h>`. > > > this fix is specific for old architecture (fabric is off). > > when fabric is on, there are other errors from duplicated headers when copying to build folder. [the reason is that framework build would try to flatten headers](https://mkonrad.net/2015/03/29/xcode-static-libraries-preserving-header-directory-structure.html). we have `primitives.h` in different folders and they would be flattened into `React_Fabric.framework/Headers`. to be honest, i don't know how to deal with the problem in the meantime, maybe subspecs are not enough, we should separate them from subspecs to dedicated podspecs so that we can have these targets as different frameworks. in this alternative fix, i try to add `React-Codegen/React_Codegen.framework/Headers` into header search paths and make original `#import <FBReactNativeSpec/FBReactNativeSpec.h>` reachable. [this change](facebook@7a0398c) in the pr is just a workaround to solve breaking in latest main branch and this is not important to the `use_frameworks!` fix at all. this breaking was coming from facebook@1804951. ## Changelog [iOS] [Fixed] - Fix iOS build error when Podfile `use_frameworks!` is on and Fabric is off Pull Request resolved: facebook#33409 Test Plan: verify with rn-tester 1. change `fabric_enabled` to false in `packages/rn-tester/Podfile` 2. `USE_FRAMEWORKS=1 pod install` 3. build rn-tester in xcode Reviewed By: dmitryrykun Differential Revision: D34817041 Pulled By: cortinico fbshipit-source-id: 4d1a610e99a807793eb3f64461e0d735c0a9ca9c
Hello, I'm having the error you described here (react-native 0.70.6), the path problem seems to arise in multiple locations for me:
I searched for this file and it only appears in several ./ios/Build/generated/ios/FBReactNativeSpec/FBReactNativeSpec.h I thought maybe I should remove the build directory just in case, but it didn't work either and I still can't compile ❯ rm -rf yarn.lock node_modules ios/Pods ios/Podfile.lock ios/Build &&
yarn install && cd ios && pod install && .. && yarn ios:build But it didn't solve the issue. Here is my Podfile content:
I couldn't find any help about this, sorry for asking this here, but you seem to understand the issue so I guessed maybe there is something obvious I missed. Thanks. |
Summary
when
use_frameworks!
is on, there are errors like:this error may come from from f7e4c07c84b6 regression.
when
use_frameworks!
is on, xcode will search headers from framework directories, the correct imports would be#import <React_Codegen/FBReactNativeSpec/FBReactNativeSpec.h>
(xcode will transform dash to underscore, so it isReact_Codegen
but notReact-Codegen
). in the other hand, whenuse_frameworks!
is off, the correct import is#import <React-Codegen/FBReactNativeSpec/FBReactNativeSpec.h>
.to make it compatible for both modes, i would propose to use a valid module name for the codegen pod. that is theReactCodegen
. supposedly can define different module name in podspecs either byheader-dir
ormodule_name
. however, i don't recommend to use these as it has a known issue when DEFINES_MODULE=YES. that's why i use theReactCodegen
name.Update: to make it compatible for both modes, i merged
FBReactNativeSpec
andReact-Codegen
. the final name isFBReactNativeSpec.podspec
, so the final valid import will be#import <FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec.h>
. this is specific forheader_mappings_dir = '.'
mode. to keep the import as original#import <FBReactNativeSpec/FBReactNativeSpec.h>
, i removedheader_mappings_dir
in non-fabric mode. the workaround has a downside where assuming there're no other codegen libraries. otherwise, it may also break imports, e.g.#import <ScreenshotmanagerSpec/ScreenshotmanagerSpec.h> -> #import <FBReactNativeSpec/ScreenshotmanagerSpec.h>
.this fix is specific for old architecture (fabric is off).
when fabric is on, there are other errors from duplicated headers when copying to build folder. the reason is that framework build would try to flatten headers. we have
primitives.h
in different folders and they would be flattened intoReact_Fabric.framework/Headers
. to be honest, i don't know how to deal with the problem in the meantime, maybe subspecs are not enough, we should separate them from subspecs to dedicated podspecs so that we can have these targets as different frameworks.Changelog
[iOS] [Fixed] - Fix iOS build error when Podfile
use_frameworks!
is on and Fabric is offTest Plan
verify with rn-tester
fabric_enabled
to false inpackages/rn-tester/Podfile
USE_FRAMEWORKS=1 pod install