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

[React Fabric] Updating Build Configuration (CocoaPods, .xcodeproj, gradle) #22989

Closed
fkgozali opened this issue Jan 15, 2019 · 26 comments
Closed
Labels
Help Wanted :octocat: Issues ideal for external contributors. Type: Discussion Long running discussion.

Comments

@fkgozali
Copy link
Contributor

fkgozali commented Jan 15, 2019

For Discussion

Various parts of React Fabric code are available in github, but we need to upgrade a bunch of build configuration to make it build properly on iOS and Android. This will involve reflecting the BUCK targets for various parts of Fabric into the 3 areas:

An initial version of the update could just be manually sync'ed configs for .podspec, .xcodeproj, and .gradle.

But for the future, some ideas include:

  • Have BUCK generate the relevant .podspec, .xcworkspace, or .gradle -- this may require a bunch of work via buck project
  • etc

Requirements

cc @mdvacca, @shergin

@fkgozali fkgozali added Help Wanted :octocat: Issues ideal for external contributors. Type: Discussion Long running discussion. labels Jan 15, 2019
@danibonilha
Copy link
Contributor

danibonilha commented Jan 28, 2019

Hey there! I'd like to help, so I started with the podspec, and I'd appreciate any tips, on how to test and make sure everything is working. Thanks.

@fkgozali
Copy link
Contributor Author

on how to test and make sure everything is working

First step is to have RNTester podspec able to build Fabric specs (that includes all subspecs from fabric directory).

The next step after that is to actually use Fabric in RNTester. This is still work in progress for our team, so we can tackle that slightly later. cc @shergin

@danibonilha
Copy link
Contributor

Hey, could you guys please verify if this include is correct? I'm getting an error 'Unknown type name 'CGFloat'' in this file and looking at the code '<CoreGraphics/CoreGraphics.h>' is always used with .m files and import instead of include and .cpp files.

@fkgozali
Copy link
Contributor Author

fkgozali commented Jan 29, 2019

Hey, could you guys please verify if this include is correct? I'm getting an error 'Unknown type name 'CGFloat'' in this file and looking at the code '<CoreGraphics/CoreGraphics.h>' is always used with .m files and import instead of include and .cpp files.

Does it work if you modify it to #import ? This builds fine using Buck at FB, perhaps there's a missing flag somewhere? Or perhaps you're missing the dependency on CoreGraphics iOS framework in the podspec?

If moving to #import works feel free to send a PR for it. Looking at a similar file, seems like we use #import there:

@grabbou
Copy link
Contributor

grabbou commented Jan 30, 2019

@fkgozali are there any question marks you see as far as using CocoaPods with Fabric/TurboModules? I'd love to hear more about your experience so far.

@mgriepentrog
Copy link
Contributor

I can take a look at updating the xcodeproj files.

@fkgozali
Copy link
Contributor Author

are there any question marks you see as far as using CocoaPods with Fabric/TurboModules? I'd love to hear more about your experience so far.

We don't use cocoapods/gradle internally, so I'm hoping the unknowns can be covered with the help from community on this issue.

@danibonilha
Copy link
Contributor

Hey React.podspec is up for grabs again, I'm sorry, I didn't have enough time to finish it up.

@mgriepentrog
Copy link
Contributor

I switched gears to updating the podspec (xcodeprojs are a pain). Here is what I have so far: mgriepentrog@44c5649. To find a lot of the low-hanging fruit, I'm running pod install inside of RNTester and trying to build the app with the RCTFabric subspec included. Currently encountering react-native/RNTester/Pods/Headers/Private/React/react/graphics/Geometry.h:55:3: Unknown type name 'Float'; did you mean 'float'?, but haven't had much time to investigate.

@danibonilha
Copy link
Contributor

I switched gears to updating the podspec (xcodeprojs are a pain). Here is what I have so far: mgriepentrog@44c5649. To find a lot of the low-hanging fruit, I'm running pod install inside of RNTester and trying to build the app with the RCTFabric subspec included. Currently encountering react-native/RNTester/Pods/Headers/Private/React/react/graphics/Geometry.h:55:3: Unknown type name 'Float'; did you mean 'float'?, but haven't had much time to investigate.

I got stuck on the same problem, and couldn't get pass it I added same frameworks, changed some files and then same problem again. 😟

@fkgozali
Copy link
Contributor Author

It looks like this header file wasn't included in your setup?

@ericlewis
Copy link
Contributor

ericlewis commented Feb 16, 2019

I made progress on getting the cocoapods to build and ran in to a few problems:

  • CGFloat in Float.h, same problems as above, it seems that the interop isn't working, I stubbed this with a double to continue.
  • RCTSwitchComponentView makes references to react/components/rncore/, which I could not find, nor could I find something similar.
  • Missing x86_64 Symbol Error on getDefaultComponentRegistryFactory, but that might be due to my stubbing? <- This is due to missing folly imports.

I am still working on getting it to build a little further, and and still trying to figure out what is going on with Float.h.

But the podfile seems mostly complete: ericlewis@a6510d9. It creates libReact.a.

@mgriepentrog
Copy link
Contributor

One thing I noticed, if you look at the Pods project under the Frameworks folder, the CoreGraphics framework is pointing to /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.0.sdk/System/Library/Frameworks/CoreGraphics.framework, which doesn't exist (the latest sdk version is 12.1). I'm not sure where that path is getting derived from.

@ericlewis
Copy link
Contributor

I updated those frameworks manually to point to 12.1 & was no dice.

@ericlewis
Copy link
Contributor

ericlewis commented Feb 16, 2019

Here is my WIP: https://github.com/ericlewis/react-native/commits/fabric

  • pod files seem “done”
  • renaming Float.h fixes CGFloat issue. Conflicts somewhere.

Compiling this branch's RNTester with RCTFabric & RCTFabricSample works, it links correctly! But you must remove the fabric switch component refs, bc they ref something called rncore that appears to be missing.

@ericlewis
Copy link
Contributor

Another issue too is I get a “functional file not found” in relation to what looks like libc++ headers in LayoutPrimitives.h, it also happens in SurfacePresenter with memory, tho that is fixed by a rename to memory.h.

I have not been able to figure out why this is happening, tho it is not happening in older versions of fabric using this same technique.

@fkgozali
Copy link
Contributor Author

Another issue too is I get a “functional file not found” in relation to what looks like libc++ headers in LayoutPrimitives.h, it also happens in SurfacePresenter with memory, tho that is fixed by a rename to memory.h.

cc @shergin, @JoshuaGross -- thoughts?

@ericlewis
Copy link
Contributor

@fkgozali I figured out what the issue was.. I am not sure I am using the library correctly, it happened when importing RCTSurfacePresenter.h, I believe the issue is due to AppDelegate.m not being an Objective-C++ file. It does link and build and run when I use RCTSurfaceHostingProxyRootView, but it is blank due to not having a presenter.

@ericlewis ericlewis mentioned this issue Feb 20, 2019
6 tasks
@ericlewis
Copy link
Contributor

My current progress, open issues, and questions: #23550

@fkgozali
Copy link
Contributor Author

fkgozali commented Feb 20, 2019

I believe the issue is due to AppDelegate.m not being an Objective-C++ file

Ah, yeah a bunch of files need to become .mm instead. I think that's an ok requirement though, given the increasing number of C++ we're adding. That said, if AppDelegate.m needs to be AppDelegate.mm, let's switch it over. If you find more .m files to be converted to .mm, let's note them here as well.

@ericlewis
Copy link
Contributor

Working podspecs: #23802

@cpojer
Copy link
Contributor

cpojer commented Mar 19, 2019

RNTester now works with Fabric, and we have a separate issue dealing with folly so I'm gonna go ahead and close this issue.

@fkgozali feel free to reopen or create a new issue depending on whether you wanted to keep this issue as a meta issue for all Fabric work or just scoped to making things work in RNTester.

@cpojer cpojer closed this as completed Mar 19, 2019
@fkgozali
Copy link
Contributor Author

Hold on, this is the umbrella ask for fabric build, including android, so let's keep this open. What was done was just for CocoaPods.

@fkgozali fkgozali reopened this Mar 19, 2019
@fkgozali
Copy link
Contributor Author

Also what's not yet done:

  • component registration, integrated with build system (this doesn't exist in OSS yet)

@kelset
Copy link
Contributor

kelset commented Jun 18, 2021

@fkgozali should we close this off? :D

@fkgozali
Copy link
Contributor Author

fkgozali commented Jun 18, 2021 via email

@kelset kelset closed this as completed Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted :octocat: Issues ideal for external contributors. Type: Discussion Long running discussion.
Projects
None yet
Development

No branches or pull requests

7 participants