Add Carthage & CocoaPods support. #352
Assignees
DaddyFrosty
Clear assignees
DaddyFrosty
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: DaddyFrosty/yoga#352
Reference in New Issue
Block a user
No description provided.
Delete Branch "carthage-cocoapods-support"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This supersedes #309 and #305.
This is still a work-in-progress.
TODO
pod lib lintpass and make sure YogaKit can be included using CocoaPodspod lib lintto .travis.ymlFOLLOW-UP
@dshahidehpour @emilsjolander There are 4 tests that are failing in Xcode. Most of them seems to rounding errors when computing frames. I think @hartbit run into the same issue. Do you have any idea what could be the problem or how to solve this?
@guidomb The rounding failure is happening because
buck testand Xcode are slightly different testing environments and their scales seem to be different, generating different results. I'll take care of it today.Meanwhile, would you like some help with some of those other tasks? I'll migrate Travis to Xcode 8.2 today.
@dshahidehpour Thanks! I'll wait until there are any news regarding the tests. Regarding the other items no worries I can work on those once the tests are fixed and there is no rush on migrating to Xcode 8.2 for now. I can do that once everything else has bee implemented but If you need / want to do that no worries.
I put up a PR to use Xcode8.2 in #353
@guidomb I landed fixes for the stuff blocking you. I also broke the list up into two sections, feel free to move stuff around!
Awesome! I'll keep working on this by the end of my day. Thanks!
I've incorporated the latest changes in
master. The Xcode project is compiling and tests are passing. Carthage is also working. I am having problems with CocoaPods. There seems to be a problems with how header files are included. Need to do more research about it.Awesome progress @guidomb! let us know if we can help with the header import stuff, can you paste the error?
Carthage is compiling but have you tried using it in a test project? I remember the header files causing issues there too.
@guidomb I messed around with this last night for a bit too. Here are some takeaways:
.swift-versionandYogaKit.podspecto the parent directory. @emilsjolander is fine with that.pod lib lintstill wasn't happy.I was able to get it to lint with the changes outlined by @dshahidehpour and adding a header search path:
afa4ff61adAdditionally I simplified the file selection sets.
Does anyone have an application that this should integrate with that I can test out? Or should this be used inside React Native?
However, I should add that the fix depends on a change that’s in CP 1.2.0, which is currently in RC: https://github.com/CocoaPods/CocoaPods/blob/master/CHANGELOG.md#120beta1-2016-10-28
@@ -0,0 +15,4 @@DESCspec.documentation_url = 'https://facebook.github.io/yoga/docs/getting-started/'spec.source = { :git => 'https://github.com/facebook/yoga.git', :tag => "v#{spec.version}" }spec.platform = :iosWhy is this only targeting iOS, shouldn’t the core library be able to build for all platforms? (So minus the UIView stuff.)
@@ -0,0 +15,4 @@DESCspec.documentation_url = 'https://facebook.github.io/yoga/docs/getting-started/'spec.source = { :git => 'https://github.com/facebook/yoga.git', :tag => "v#{spec.version}" }spec.platform = :iosWhile the core library could technically build on all platforms, I plan to send a PR soon with an NSView extension. I guess we could all platforms then.
@@ -0,0 +15,4 @@DESCspec.documentation_url = 'https://facebook.github.io/yoga/docs/getting-started/'spec.source = { :git => 'https://github.com/facebook/yoga.git', :tag => "v#{spec.version}" }spec.platform = :iosGotcha.
So then I guess my assumption that this podspec was somehow also tied to React Native’s podspec using this is incorrect. /cc @javache
@hartbit @dshahidehpour I am migrating my current project to use the latest version of Yoga and using Carthage with the goal of testing this PR. So far the built generated by Carthage works. Had no issues with header files.
This change forced me to migrate to the new API that @hartbit introduced in #322. Just wanted to say, awesome work! The API feels way more natural in Swift now. One thing I noticed is that both
borderandpositiondon't support thehorizontalandverticaledge cases. Is this correct?@dshahidehpour @alloy How should I proceed with CocoaPods support? Should I add the changes you have suggested to this PR?
Is your project OSS and/or do you know of one in which this podspec is expected to work for me to test?
…or can you create a simple Hello World app for Yoga for me to test with?
@alloy My project is not open source (yet). I want to open source it but I need to do some work first in order for it to be ready to be published. I don't see that happening any time soon, at least not in the next few day. But I could extract some code from it in order to test CocoaPods integration. I'll try to do that later this day o tomorrow.
@guidomb Yeah just something simple is good enough. I could do it myself, but as I haven’t used the API in anger I might make it too simple.
@alloy OK I'll do that then. I'll try to do that by the end of my day (GMT -03:00) or tomorrow.
Update!
TL:DR; I'm making a Cocoapod specifically for the C-Lib. Once that lands (in a few hours) we can simply add that pod as a dependency to YogaKit's podspec and keep the imports as <yoga/*>.
The reason we are doing this is that we don't want to change the imports of core yoga files to YogaKit. Currently, that's not really possible by adding them all to the same podspec without building a static library (
pod lib lint --use-libraries). The problem with a static library is that it won't play nice with Swift Projects (source: @nlutsenko).I was surprised to learn this because the React Native library uses yoga as well, and they have yoga defined as a subspec. What I learned is that you can only build the React Native pod as a static lib, so, I don't think it works with Swift (need to confirm).
@dshahidehpour OK let me know once that is landed into
master. AFAIK it should be a problem for Swift users because we can make YogaKit podspec to create a framework. The static lib should only be for the C code. I am assuming that framework can contain a static lib. Not really sure if my assumption is correct.This blog post seems to explain how to use static libs inside frameworks.
@guidomb The C-lib podspec is landing now. When we push it, do you think that we should push it as a static lib? I'd be fine with pushing it as a regular framework.
@dshahidehpour Not really sure. I think it would be better to use frameworks because I know that should work we both Swift and Objective-C users. What do you think @alloy ?
Not entirely sure that should be necessary, but it might make things simpler 👍
Is this within another project? Because I’m definitely able to
pod lib lintboth frameworks and static libs with my patches applied.Yeah, specifically you can’t build Swift pods as static libs, and with CocoaPods you do either frameworks or static libs.
Why can it only build as a static lib? It should. Is it because of this issue? (I’m still getting up to speed after a hiatus.)
With CocoaPods you don’t make that choice as a pod author, but rather as a consumer of the pods. The default is static libraries, but if you specify
use_frameworks!in your Podfile it will build frameworks instead.Yeahp. If you run
pod lib lint React.podspecyou get the same yoga import errors that we are seeing in this diff. The errors go away when you runpod lib lint React.podspec --use-libraries.The Yoga.podspec has landed :) Lets use it as a dependency in
YogaKit's podspec. I had to runpod repo updatelocally to see it.P.S. Thank you @alloy for all your feedback and help!! We really appreciate it :)
Ok, dope. I’ll take a look linting RN’s podspec 👍 I’ll also start using Yoga’s now landed official podspec to pull Yoga into RN instead of a vendored version.
You are most welcome! I kinda feel bad for being gone for a while, I should now be back more consistently to keep RN in shape on CocoaPods.
I've rebased with
masterand addedYogaas a dependency ofYogaKit.pod lib lintpasses if the--allow-warningsflag is given. I am getting the following warningBecause the git tag hasn't been created yet.
I still need to fully test this in a project. Later today I will publish the project where I am using Yoga and I'll create a brach to include YogaKit using CocoaPods to make sure that everything is working.
@dshahidehpour remember that once this PR is landed, every time you want to cut a new release you are going to have to publish both Yoga and YogaKit pods to the trunk repository.
Also @dshahidehpour did you have a chance to take a look at my comment in https://github.com/facebook/yoga/pull/352#issuecomment-275403780
That is my doing. I based myself off the React Native properties (https://facebook.github.io/react-native/docs/layout-props.html). I think they don't exist for ambiguity reasons. The horizontal margin makes sense, but the horizontal position seems less clear.
Yeah I was getting the same warning. I had to use
--allow-warningsto create the Yoga pod. We should talk to @emilsjolander about changing the release tags. Go ahead and use the--allow-warningsflag to create your pod.It's a weird thing to warn about, seems non-important. I'll think about it for the next time. For now let's just ignore the warning. I'll push YogaKit to cocoapods once this has been merged.
Awesome work @guidomb! I'm going to import this, fixup a couple things (update spec description) and merge it! We can continue to iterate after it lands. Thank you so much!
@dshahidehpour has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@emilsjolander People would push specs and forget to push their git tags. The warning is correct in that the build would fail in that case.
@alloy Ok good to know the reason behind it. We continuously tag the project so not a problem i foresee us having. Can I remove this lint from the project without disabling all the other helpful warnings?
Hmmm, don't think so, but I'll double check when I'm back at a machine. Nonetheless, this should just pass if you push the tag before the spec during your release process.
Btw, just wanna be sure that you are indeed switching to a different versioning scheme:
2cc2a5f2ff (commitcomment-20632914)@alloy The version number of the project does not match the tag used. See the release for
1.0.0here https://github.com/facebook/yoga/releases/tag/v2017.01.23.00Ok 👌
Oops, I didn't read that well.
This warning is not about the tag not having been created yet, but rather that the tag’s name does not include the version (in this case
1.0.0), which does seem something you want to be able to do.There’s no way to currently disable just that warning. If this remains an issue with future tags, I suggest you file a new issue about it.
I think that the tag name should follow semantic versioning. Otherwise CocoaPods won't be able to checkout the corresponding revision after resolving the version that should be used after running
pod install. Not 100% sure though.No there’s no such requirement. It’s just common and allows you to not have to type out two different versions in the same file, but instead refer to the pod version for the tag’s name. Why do you think it might be required?
@guidomb just a quick heads-up. I'm going to trim this diff so it only contains the Cocoapods support.
Our internal build system is giving us some grief about the changes made for the Carthage support, so, I want to attack that stuff in another diff. Me or @emilsjolander need to create the
.xcodeprojand add Facebook as the Organization. We also to need to some work on the .plist files so they work externally, and internally at FB.@dshahidehpour Btw, want me to send you a PR to run
pod lib linton Travis CI?@alloy Yes plz! Just an FYI, because of our branch tag, we will have to add
--allow-warningsto make it pass.@dshahidehpour Aye, indeed.
For now, just want to let you know that I’m successfully able to use the core pod in React Native https://github.com/facebook/react-native/pull/12089
@dshahidehpour OK. Let me know if you need any help preparing the project to be Carthage friendly. Do you have an ETA for that?
@alloy I had the impression that tag name must matched pod version but I wasn't really sure. Had the memory of not being able to publish a pod for this reason but it's been a long time since the last time I published a pod.
@alloy @guidomb Cocoapods support has landed, so I'm going to keep this thread closed. We can talk about Carthage support in #361, or in a new pull request. Thanks!
@dshahidehpour When you say Cocoapods support has landed, any idea if this issue has been solved: https://github.com/facebook/react-native/issues/11781 ? Thank you.
@quentinfasquel No, see https://github.com/facebook/react-native/pull/12089
Thanks @alloy for your quick reply. So that means I should wait?
I found that just adding the 'Core' subspec doesn't trigger the yoga/Yoga.h issue but when I
import Reactwill encounter an issue with the C++ lib that's used:'cstdint' file not foundin JSBundleType.hPull request closed