[CocoaPods] Make podspec great again. #12089

alloy · 2017-01-27T05:50:45Z

The main changes here are:

This depends on the latest CocoaPods (1.2.0). It’s currently in RC, but if I’m not mistaken a proper release is expected soon. /cc @dantoml
Adds required header search paths for the jschelpers and cxxreact subspecs.
Makes the jschelpers and cxxreact headers private to building React Native, not visible to the user’s project.
It uses the canonical upstream Yoga v1.0.0 podspec: https://github.com/facebook/yoga/blob/master/Yoga.podspec
Consistent styling.

I have been able to get our app to build again using this artsy/emission#437. The spec has some warnings, but otherwise fully passes lint.

@rh389 @sjmueller Could you please test with your projects?

alloy · 2017-01-27T05:52:41Z

Next week I will take some time to get both Yoga and React Native to perform linting on CI, so that I can hear of these regressions when they happen. (Do you have some way with your bot to notify specific people when specific parts of the CI setup fail?)

ide · 2017-01-27T07:11:28Z

React.podspec

-    ss.frameworks    = "JavaScriptCore"
-    ss.libraries     = "stdc++"
+  s.subspec "Core" do |ss|
+    ss.dependency             "Yoga", "~> 1.0"


Can you make this use the local copy of Yoga? The RN tests in this repo (and separate tests within FB I imagine) are run against whatever copy of Yoga is synced into this repo and we want to avoid divergence between using CocoaPods vs. manually linking the framework.

@ide Yeah you’re totally right that the code should match what’s being used in the tests, but I was under the impression that Yoga moving to an official podspec was paving the way for us to make using both RN and Yoga as dependencies of our apps nicer.

I.e. I would really like to have the choice to have flattened transitive dependencies in my app, no duplications (smaller binary size), and less dealing with possible duplicate symbols and what not.

@emilsjolander do you know if that was/is not the case?

Nonetheless, as it stands, the code is indeed out of sync, I should have noted that.

The code vendored in RN is this Yoga version.

The published v1.0.0 of Yoga is this newer revision.

So if this were to be used, either the version in 1 needs to be published as e.g. Yoga v0.9, or the vendored code in RN should be updated to the version in 2.

Getting releases in sync will be very hard. @dshahidehpour, could we have the podspec in the dirsynced folder so we can reference the included copy?

Getting releases in sync will be very hard.

Oh, bummer, I thought that cutting Yoga releases wasn’t a big deal “If you need a new release to be cut please file a task.”

could we have the podspec in the dirsynced folder so we can reference the included copy?

You mean that users would add that to their Podfile separately? Because I don’t see a way to reference it from RN’s podspec 🤔

Took a quick look at using the vendored Yoga code, alas this is not going to work well right now due to CocoaPods/CocoaPods#6440

The way I would be comfortable with relying with Yoga from the CocoaPods repo is if Facebook were to use the same snapshot of Yoga (whether via CocoaPods or another mechanism e.g. Buck). I strongly prefer a systemic way that makes sure FB engineers are less likely to break open-source and vice versa, by making sure everyone is running the same copy of Yoga.

I think settling on the same systems is going to be a hard sell. If I understand it right, there’s the following types of consumers:

FB engineers, I assume they use Buck

OSS CP users

OSS npm users

What’s most important is that they can all know, at release time, what the right version is, no? So some metadata about the Yoga commit/tag that’s being used is the main requirement. This could be a file next to the vendored code, or maybe something like a git subtree (assuming that git records metadata about these like with subomdules, but I’ve never used them).

The same applies to other parts of RN, btw, like the snapshot (image) testing module. The way it’s currently included makes it hard to integrate with apps that use FBSnapshotTestcase directly.

The systems don't need to be the same, I want just the Yoga code to be the same -- I would be on board with specifying a precise commit.

robhogan · 2017-01-27T17:23:15Z

LGTM - working nicely in a mixed Swift/Obj-C project importing React as a framework, with a bunch of React subspecs and third-party native modules installed as pods. No hacks, no post-install scripts, no manual config! 🎉

Thanks for finding the time to work on this 👍

javache · 2017-01-27T17:56:03Z

Thanks for your help @alloy! We should definitely get this on CI as well, @hramos can you help with that?

javache · 2017-01-27T18:01:26Z

React.podspec

+  s.subspec "jschelpers" do |ss|
+    ss.source_files         = "ReactCommon/jschelpers/{JavaScriptCore,JSCWrapper}.{cpp,h}"
+    ss.private_header_files = "ReactCommon/jschelpers/{JavaScriptCore,JSCWrapper}.h"
+    ss.pod_target_xcconfig  = { "HEADER_SEARCH_PATHS" => "$(PODS_TARGET_SRCROOT)/ReactCommon" }


This will work for now, but it doesn't match the xcode behaviour in terms of recursion. For now, we can try not include any subfolders in these directors.

You mean you would add subfolders but not include them with the same path, but rather all under the ‘namespace’?

In our xcode setup, a header like ReactCommon/jschelpers/foo/bar.h would be includable as <jschelpers/bar.h>, which won't work this setup as I understand it.

In the short term, we can avoid any adding new subfolders under these ReactCommon targets

endocrimes · 2017-01-28T14:17:35Z

I just released 1.2.0 proper. 👍. https://github.com/CocoaPods/CocoaPods/releases/tag/1.2.0

mkonicek · 2017-01-30T18:26:02Z

Hi guys, I'm the React Native oncall this week - doing my usual mark&sweep through pull requests :) What's the conclusion here? Is this good to go, needs changes, or there is no consensus on what to do next?

bachand · 2017-01-31T08:22:15Z

React.podspec

+  s.platform                = :ios, "8.0"
+  s.pod_target_xcconfig     = { "CLANG_CXX_LANGUAGE_STANDARD" => "c++14" }
+  s.preserve_paths          = "package.json", "LICENSE", "LICENSE-CustomComponents", "PATENTS"
+  s.cocoapods_version       = ">= 1.2.0.beta.1"


@alloy is there a feature of CocoaPods 1.2.0 that is required to support this new podspec? If not, do you think we could relax the version requirement? Would be nice if it wasn't necessary to update to consume this fix!

Thanks for doing this, solves the problem we've been having!! 👍

This version of the spec uses POD_TARGET_SRCROOT, which is a CocoaPods 1.2.0 only feature. I'll have another think when finalizing it.

Alas, this is definitely the better fix for the majority of people. However, you could probably replace the $(POD_TARGET_SRCROOT) parts with $(SRCROOT) and point it to where your RN source lives relative to the Xcode source root.

robhogan · 2017-01-31T14:54:09Z

@mkonicek I think ideally we need someone at FB to modify the process by which Yoga etc are copied in from other repos so that CP can know which version to pull to match the local copy.

@alloy's idea of generating a metadata file that sits alongside any imported code specifying at least a github tag/SHA (of the 'canonical' open-source repo) sounds pretty useful to me.

d16r · 2017-01-31T15:03:31Z

@emilsjolander is in charge of the Yoga mirror at FB.

alloy · 2017-01-31T16:07:22Z

@mkonicek Yup, what @rh389 said. I'll send my thoughts on how to do this to @emilsjolander tomorrow and will report back here once we've reached consensus.

javache · 2017-01-31T19:00:19Z

For now, can we just have our own spec for Yoga as a subspec and not make use of the Yoga spec file?

rops · 2017-02-01T13:16:25Z

@alloy just tried this patch but #11721 doesn't seem to be fixed for me. I'm using latest versions of 3rd party modules ( with new imports <React/..> ) and I'm including React from pods. Building the 3rd party libraries fails at "<React/RCTBridgeModule> file not found". Any idea what's wrong? Maybe has to do with header search paths?

alloy · 2017-02-01T16:28:03Z

@javache I’ll spend some more time to see I can get that to work.

alloy · 2017-02-01T16:28:50Z

@rops Hmm, might be a different issue after all. I’ll take a good look at that one once this issue/PR is resolved.

rops · 2017-02-01T16:31:19Z

@alloy I've done some digging and it seems that the third party libraries can't find <React> because they don't have the Pods header path in their Header Search Path. Can you think of some cocoapods hackery to fix that?

alloy · 2017-02-01T16:34:57Z

@rops Let’s take this to #11721

guysegal · 2017-02-03T10:06:56Z

If this PR will be merged are we expecting to see it only in 0.43.0 (March release)?

keatongreve · 2017-02-03T17:27:55Z

@alloy This might need to be updated for some changes in master/0.42.x. I've been using the work in this PR to host versions of React in a private podspec, and while I got a working version of 0.41.0 pushed, 0.42.0-rc.1 was not successful (I didn't test rc.0).

This fixed it for me: hudl@97f6565

Other than that, this work has been super helpful for how we're using React Native, hopefully we see this merged soon.

alloy · 2017-02-05T16:34:29Z

For now, can we just have our own spec for Yoga as a subspec and not make use of the Yoga spec file?

@javache The problem here is that when building as a framework a header is expected to be part of the frameworks’s namespace, other namespaces are expected to be outside of the framework (i.e. in a different framework). Thus, an import like yoga/Yoga.h cannot exist inside the same framework. As this import exists in public headers exported by React (RCTConvert.h and RCTShadowView.h), this leads to an error when trying to build a Clang module for the framework (or otherwise would have lead to an error if the user tried to import one of these headers).

In fact, the only reason that jschelpers and cxxreact can be subspecs at all is simply because I made their headers private to the project.

In conclusion the only possible interim solution I see is to add a separate podspec for the vendored Yoga code and have users add that one specifically to their Podfile. I’ll add an example to this PR.

ide · 2017-02-05T21:05:06Z

In conclusion the only possible interim solution I see is to add a separate podspec for the vendored Yoga code and have users add that one specifically to their Podfile. I’ll add an example to this PR.

Sounds good. People already have to add subspecs for basic things like Images and Networking when including RN via CocoaPods, so I find it reasonable to ask the same for Yoga. It's extra work but not much extra complexity, if that makes sense.

alloy · 2017-02-05T22:07:50Z

@ide Good point.

ide · 2017-02-06T23:43:59Z

@facebook-github-bot shipit

facebook-github-bot · 2017-02-06T23:44:11Z

@ide has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Fixes facebook#11272 Fixes facebook#11572 Fixes facebook#11781 The main changes here are: * This depends on the latest CocoaPods (1.2.0). It’s currently in RC, but if I’m not mistaken a proper release is expected soon. /cc dantoml * Adds required header search paths for the jschelpers and cxxreact subspecs. * Makes the jschelpers and cxxreact headers private to building React Native, not visible to the user’s project. * It uses the canonical upstream Yoga v1.0.0 podspec: https://github.com/facebook/yoga/blob/master/Yoga.podspec * Consistent styling. I have been able to get our app to build again using this artsy/emission#437. The spec has some warnings, but otherwise fully passes lint. rh389 sjmueller Could you please test with your projects? Closes facebook#12089 Differential Revision: D4518605 fbshipit-source-id: ecf86232d8b1af52d139eadd1acc10f5c1d42c29

alloy · 2017-02-07T12:55:15Z

Wow. You know what I only just now realised? The title of this PR… I wrote it while I was in the US and might have been watching news in the background and made a Freudian slip.

It was supposed to say “Make podspec work again.”

guysegal · 2017-02-07T13:57:17Z

It seem like that was your intention in the PR name.. Anyway you actually made podspec GREAT again.
We've been following this PR and it solves us a lot of problems!
Thank you

Summary: Fixes facebook#11272 Fixes facebook#11572 Fixes facebook#11781 The main changes here are: * This depends on the latest CocoaPods (1.2.0). It’s currently in RC, but if I’m not mistaken a proper release is expected soon. /cc dantoml * Adds required header search paths for the jschelpers and cxxreact subspecs. * Makes the jschelpers and cxxreact headers private to building React Native, not visible to the user’s project. * It uses the canonical upstream Yoga v1.0.0 podspec: https://github.com/facebook/yoga/blob/master/Yoga.podspec * Consistent styling. I have been able to get our app to build again using this artsy/emission#437. The spec has some warnings, but otherwise fully passes lint. rh389 sjmueller Could you please test with your projects? Closes facebook#12089 Differential Revision: D4518605 fbshipit-source-id: ecf86232d8b1af52d139eadd1acc10f5c1d42c29

huangcheng1 · 2017-02-09T07:16:24Z

Do you tried using Swift?

alloy · 2017-02-09T08:04:26Z

Yes, @rh389 did (see earlier comments). I also tested this as a framework build, which is all that is required for Swift interoperability.

Summary: Fixes facebook#11272 Fixes facebook#11572 Fixes facebook#11781 The main changes here are: * This depends on the latest CocoaPods (1.2.0). It’s currently in RC, but if I’m not mistaken a proper release is expected soon. /cc dantoml * Adds required header search paths for the jschelpers and cxxreact subspecs. * Makes the jschelpers and cxxreact headers private to building React Native, not visible to the user’s project. * It uses the canonical upstream Yoga v1.0.0 podspec: https://github.com/facebook/yoga/blob/master/Yoga.podspec * Consistent styling. I have been able to get our app to build again using this artsy/emission#437. The spec has some warnings, but otherwise fully passes lint. rh389 sjmueller Could you please test with your projects? Closes facebook#12089 Differential Revision: D4518605 fbshipit-source-id: ecf86232d8b1af52d139eadd1acc10f5c1d42c29

ide · 2017-02-10T09:22:31Z

This PR has no effect on Android -- please try to keep the discussion at hand focused.

Summary: Fixes #11272 Fixes #11572 Fixes #11781 The main changes here are: * This depends on the latest CocoaPods (1.2.0). It’s currently in RC, but if I’m not mistaken a proper release is expected soon. /cc dantoml * Adds required header search paths for the jschelpers and cxxreact subspecs. * Makes the jschelpers and cxxreact headers private to building React Native, not visible to the user’s project. * It uses the canonical upstream Yoga v1.0.0 podspec: https://github.com/facebook/yoga/blob/master/Yoga.podspec * Consistent styling. I have been able to get our app to build again using this artsy/emission#437. The spec has some warnings, but otherwise fully passes lint. rh389 sjmueller Could you please test with your projects? Closes #12089 Differential Revision: D4518605 fbshipit-source-id: ecf86232d8b1af52d139eadd1acc10f5c1d42c29

Summary: Fixes facebook#11272 Fixes facebook#11572 Fixes facebook#11781 The main changes here are: * This depends on the latest CocoaPods (1.2.0). It’s currently in RC, but if I’m not mistaken a proper release is expected soon. /cc dantoml * Adds required header search paths for the jschelpers and cxxreact subspecs. * Makes the jschelpers and cxxreact headers private to building React Native, not visible to the user’s project. * It uses the canonical upstream Yoga v1.0.0 podspec: https://github.com/facebook/yoga/blob/master/Yoga.podspec * Consistent styling. I have been able to get our app to build again using this artsy/emission#437. The spec has some warnings, but otherwise fully passes lint. rh389 sjmueller Could you please test with your projects? Closes facebook#12089 Differential Revision: D4518605 fbshipit-source-id: ecf86232d8b1af52d139eadd1acc10f5c1d42c29

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. GH Review: review-needed labels Jan 27, 2017

This was referenced Jan 27, 2017

Add Carthage & CocoaPods support. facebook/yoga#352

Closed

[WIP] Upgrade to latest React Native artsy/emission#437

Closed

RCTConvert.h yoga/Yoga.h file not found #11781

Closed

ide reviewed Jan 27, 2017

View reviewed changes

javache reviewed Jan 27, 2017

View reviewed changes

bachand added a commit to airbnb/react-native that referenced this pull request Jan 31, 2017

Integrate facebook#12089

58a0125

bachand reviewed Jan 31, 2017

View reviewed changes

robhogan mentioned this pull request Feb 2, 2017

[0.40] Warning: Swift Projects Fail #11572

Closed

alloy force-pushed the podspec branch from 58b8fa9 to 9e0a343 Compare February 5, 2017 22:00

facebook-github-bot closed this in 031cb20 Feb 7, 2017

This comment has been minimized.

Sign in to view

vongohren mentioned this pull request Feb 8, 2017

[Master] - Cannot resolv ReactNativeHost & ReactApplication #12286

Closed

This comment has been minimized.

Sign in to view

alloy mentioned this pull request Feb 9, 2017

[0.42] Commits to cherry pick into stable #12172

Closed

This comment has been minimized.

Sign in to view

HazAT mentioned this pull request Mar 21, 2017

Cocoapods support getsentry/sentry-react-native#16

Merged

mrbrentkelly mentioned this pull request Mar 8, 2021

2.0.0 stable build error on iOS - hidden_headers/Logger.h not found software-mansion/react-native-reanimated#1805

Closed

[CocoaPods] Make podspec great again. #12089

[CocoaPods] Make podspec great again. #12089

Conversation

alloy commented Jan 27, 2017 • edited Loading

alloy commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ide Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

robhogan commented Jan 27, 2017

javache commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endocrimes commented Jan 28, 2017

mkonicek commented Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhogan commented Jan 31, 2017

d16r commented Jan 31, 2017

alloy commented Jan 31, 2017

javache commented Jan 31, 2017

rops commented Feb 1, 2017

alloy commented Feb 1, 2017

alloy commented Feb 1, 2017

rops commented Feb 1, 2017 • edited Loading

alloy commented Feb 1, 2017

guysegal commented Feb 3, 2017

keatongreve commented Feb 3, 2017

alloy commented Feb 5, 2017

ide commented Feb 5, 2017

alloy commented Feb 5, 2017

ide commented Feb 6, 2017

facebook-github-bot commented Feb 6, 2017

alloy commented Feb 7, 2017

guysegal commented Feb 7, 2017 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

huangcheng1 commented Feb 9, 2017

alloy commented Feb 9, 2017

This comment has been minimized.

This comment has been minimized.

ide commented Feb 10, 2017

This comment has been minimized.

This comment has been minimized.

alloy commented Jan 27, 2017 •

edited

Loading

ide Jan 28, 2017 •

edited

Loading

mkonicek commented Jan 30, 2017 •

edited

Loading

rops commented Feb 1, 2017 •

edited

Loading

guysegal commented Feb 7, 2017 •

edited

Loading