Add Carthage & CocoaPods support. #352
Conversation
|
@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 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! |
guidomb
force-pushed
the
carthage-cocoapods-support
branch
from
b9f5f34 to
f75f717
Compare
|
I've incorporated the latest changes in |
|
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.
… On 25 Jan 2017, at 00:56, Dustin Shahidehpour ***@***.***> wrote:
Awesome progress @guidomb! let us know if we can help with the header import stuff, can you paste the error?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@guidomb I messed around with this last night for a bit too. Here are some takeaways:
|
|
I was able to get it to lint with the changes outlined by @dshahidehpour and adding a header search path: afa4ff6 Additionally 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 |
| DESC | ||
| spec.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 = :ios |
There was a problem hiding this comment.
Why is this only targeting iOS, shouldn’t the core library be able to build for all platforms? (So minus the UIView stuff.)
There was a problem hiding this comment.
While 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.
There was a problem hiding this comment.
Gotcha.
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 |
|
@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 ( 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 |
|
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 ? |
Yoga now exposes its C API as CocoaPod. YogaKit dependends on it.
guidomb
force-pushed
the
carthage-cocoapods-support
branch
from
f75f717 to
95df27d
Compare
|
I've rebased with Because 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 #352 (comment)
|
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 |
|
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: 2cc2a5f#commitcomment-20632914 |
|
@alloy The version number of the project does not match the tag used. See the release for |
|
Ok 👌 |
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 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 |
|
@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 |
|
@dshahidehpour Btw, want me to send you a PR to run |
|
@alloy Yes plz! Just an FYI, because of our branch tag, we will have to add |
|
@dshahidehpour Aye, indeed. For now, just want to let you know that I’m successfully able to use the core pod in React Native facebook/react-native#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. |
|
@dshahidehpour When you say Cocoapods support has landed, any idea if this issue has been solved: facebook/react-native#11781 ? Thank you. |
|
Thanks @alloy for your quick reply. So that means I should wait? |
7 participants
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