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.

Sign up for GitHub

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

Jump to bottom

Integrate Flipper in RNTester on iOS #27762

Closed
wants to merge 1 commit into from
Closed

Integrate Flipper in RNTester on iOS #27762

wants to merge 1 commit into from

Conversation

priteshrnandgaonkar
Copy link
Contributor

Summary: Integrates Flipper in RNTester application and fixes the integration issues with use_frameworks! keyword.

Differential Revision: D19391739

@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. p: Facebook Partner: Facebook Partner labels Jan 14, 2020
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D19391739

@pull-bot
Copy link

pull-bot commented Jan 14, 2020 •
edited
Loading

Messages
📖

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.
📖 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against eef0942

@priteshrnandgaonkar @facebook-github-bot
Integrate Flipper in RNTester application (facebook#27762)
eef0942 
Summary:
Pull Request resolved: facebook#27762

Integrates Flipper in RNTester application and fixes the integration issues with `use_frameworks!` keyword.

Changelog: Flipper integration in RNTester application.

Differential Revision: D19391739

fbshipit-source-id: 99e2fa1435b5a3d6959b35133e1d391f315a953a
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D19391739

@facebook-github-bot
Copy link
Contributor

Hi priteshrnandgaonkar! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but we do not have a signature on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@safaiyeh safaiyeh mentioned this pull request Jan 15, 2020
Closed
alloy
alloy reviewed Jan 16, 2020
View reviewed changes
RNTester/Podfile
end
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

@priteshrnandgaonkar Could you link me to a write-up, if one exists, on the need for these deps to be linked statically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Go through the iOS setup mentioned here

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I’ve seen those before, but they do not describe why these dependencies need to be linked statically and why a monkey-patch of CocoaPods is required. I would like to understand those technical reasons, as I would like to see if there’s a way to avoid the need to monkey-patch.

Copy link
Contributor Author

@priteshrnandgaonkar priteshrnandgaonkar Jan 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The reason being, Flipper relies on CocoaLibEvent which has bunch of statically linked binaries libevent.a, libevevnt_core.a etc. due to which the SubPods relying on it has to be built as a Static Framework, which is Flipper-Folly. So ideally speaking building just CocoaLibEvent, Flipper-Folly as a static framework should work, but for some reason the dynamic framework failed to link Flipper-Folly, which I was not able to solve. Thus I built all the Flipper and Flipper related deps as a Static Framework and it gave no errors.

Even I feel that we shouldn't have a monkey-patch, if you can solve this problem, then it would be great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, when I’m back from vacation I’ll take a look-see 👍

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@rickhanlonii rickhanlonii changed the title Integrate Flipper in RNTester application Integrate Flipper in RNTester on iOS Jan 17, 2020
@rickhanlonii rickhanlonii mentioned this pull request Jan 29, 2020
Closed
30 tasks
@alloy
Copy link
Contributor

alloy commented Feb 20, 2020

@priteshrnandgaonkar This has already landed in master, right? cb80e3b

@priteshrnandgaonkar
Copy link
Contributor Author

Yes, I will close the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alloy alloy alloy left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@priteshrnandgaonkar @facebook-github-bot @pull-bot @alloy