Dynamic Library for iOS Carthage Support #168

lucasderraugh · 2015-10-15T08:56:39Z

Trying to address issue #152, this build I believe will support Carthage. The issue is that Bolts is currently a static library, whereas Carthage only supports dynamic libraries (hence why the Mac build works). I know there can be backwards compatibility issues so I've added a new target that builds dynamically. I'd really like this to get moved into production so that Facebook could support Carthage out of the box. Let me know if anything needs to be changed.

facebook-github-bot · 2015-10-15T08:57:15Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

facebook-github-bot · 2015-10-15T09:01:45Z

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

nlutsenko · 2015-10-15T21:15:24Z

Thanks for following up with this.
Let's address these few things, before we can merge it in:

Our flow is to handle all target configurations in .xcconfig files (as you've seen) - can we extract all dynamic target specific things into a new .xcconfig file?
Let's split changes around warnings and adding new target into 2 separate pull requests (I will send the warnings one in a bit), as they are not really relevant
I guess the bundle id for the new target should remain the same as for static framework, since those are the same, just different way of packaging it (I don't feel strong about this, just a suggestion).

Let me know how I can help you with this.

nlutsenko · 2015-10-15T21:40:50Z

@lucasderraugh, just opened #170 to fix all of these warnings and few more, no need to fix them here.

lucasderraugh · 2015-10-16T20:40:18Z

@nlutsenko I think I've properly setup the config file for the dynamic library. I simply duplicated the existing iOS config and changed the MACH_O_TYPE to mh_dylib. What would you like me to do with the commit for warnings?

lucasderraugh · 2015-10-20T05:19:58Z

I've rebased with the changes @nlutsenko made to warnings and removed my commit for that. I've also squashed my commits into a single commit. Let me know if that's all that needs to be done to see this off.

jsubida · 2015-10-23T17:35:20Z

Any update regarding this?

nlutsenko · 2015-10-23T17:42:26Z

Looking now.

nlutsenko · 2015-10-23T17:43:30Z

Bolts.xcodeproj/project.pbxproj

+			isa = XCBuildConfiguration;
+			baseConfigurationReference = 1D88DFF31BD18B5600329FCD /* Bolts-iOS-Static.xcconfig */;
+			buildSettings = {
+				INFOPLIST_FILE = "$(SRCROOT)/Bolts/Resources/iOS-Info.plist";


Please kill :)
The whole point of having a xcconfig file is to avoid these.

nlutsenko · 2015-10-23T17:44:24Z

Few more things here and there, overall looks great. Almost ready to ship.

jsubida · 2015-10-23T17:49:44Z

Awesome thanks!

lucasderraugh · 2015-10-23T20:39:59Z

Hopefully I deleted the right sections for this, let me know if there is anything else.

nlutsenko · 2015-10-27T00:01:34Z

@lucasderraugh, looks great. Checked out things locally and here is the list of what we need to address before merging this. Let me know if there is anything I can help you with on any of these.

Rename Bolts-iOS-Static to Bolts-iOS-Dynamic, since changing the default one to be dynamic will break people who depend on the packages that we distribute.
Change mach-o type back to staticlib for the Bolts-iOS target
Add IPHONE_OS_DEPLOYMENT_TARGET to Bolts-iOS-Dynamic and put 8.0 there, since dynamic frameworks are supported only starting from iOS 8.0.
Update project configuration and make Bolts-iOS-Dynamic target actually use the configuration file (it doesn't now)
Reorder targets in xcodeproj and .xcconfig files in Configurations group in the project so both Bolts-iOS-* targets go one after each other
???
PROFIT!

lucasderraugh · 2015-10-29T15:59:39Z

Sorry for the commit amends, wasn't sure if you strive for nice git history or not (from the looks of it you do). Regardless, I believe all these changes have been made (including a rebase).

lucasderraugh · 2015-10-29T17:32:38Z

Ok, so in the Bolts-iOS-Dynamic config file, I added the CoreGraphics and UIKit framework for linker params. This for whatever reason is breaking the OS X tests. Not sure how to fix this by using the config files exclusively.

nlutsenko · 2015-10-30T05:52:36Z

Configurations/Bolts-iOS.xcconfig

@@ -11,7 +11,6 @@
 #include "Shared/Product/Framework.xcconfig"

 PRODUCT_NAME = Bolts
-IPHONEOS_DEPLOYMENT_TARGET = 6.0


I believe this was unintended change, please revert it.

nlutsenko · 2015-10-30T05:56:28Z

@lucasderraugh

ammends

It's ok to keep separate commits until we are fully done, then we can squash everything together.
Might be easier to track progress this way, but absolutely up to you.

frameworks and OS X tests breakage

Not sure why it got broken. Try rebasing it against the current latest master.
Also, I've noticed that for some reason the xcodeproj changes included renaming of the existing Bolts-iOS target.
If nothing helps at this point - my suggestion is to try to reset all the changes for the Xcode project and recreate the target from scratch (by duplicating the existing one + renaming + changing the xcconfig file attached to this).

Takes a while to get through all of these, but I am confident we are going to make it together!

lucasderraugh · 2015-10-30T16:54:34Z

Ok, I just completely reset off of what you have on master and redid what I originally set out to do. Not sure how the OS X tests were failing previously, but the same config files seem to not interfere with it this time around 👍 .

lucasderraugh · 2015-11-19T07:44:55Z

Could this get merged in?

nlutsenko · 2015-11-19T22:08:24Z

Doing last sanity check right now and merging in...

nlutsenko · 2015-11-19T22:13:32Z

@lucasderraugh Found a small nit issue - Bolts-iOS-Dynamic.xcconfig file was by mistake added to BoltsTests-iOS target as resource. Please remove it (simply uncheckmark the target in the Utilities pane) and i'll merge it in straight away.

lucasderraugh · 2015-11-20T16:53:52Z

@nlutsenko Should be good from here.

facebook-github-bot · 2015-11-20T16:54:15Z

@lucasderraugh updated the pull request.

nlutsenko · 2015-11-20T17:37:00Z

Perfect! One last thing - could you squash both commits into just 1?

lucasderraugh · 2015-11-20T17:51:53Z

Done (I used fixup instead)

nlutsenko · 2015-11-20T17:55:23Z

Thanks for bearing with me through the whole review. So glad it is finally done!

Dynamic Library for iOS Carthage Support

facebook-github-bot · 2016-04-05T19:39:54Z

@lucasderraugh updated the pull request.

facebook-github-bot added the CLA Signed label Oct 15, 2015

nlutsenko added the needs revision label Oct 15, 2015

lucasderraugh closed this Oct 16, 2015

lucasderraugh reopened this Oct 16, 2015

nlutsenko reviewed Oct 23, 2015
View reviewed changes

nlutsenko reviewed Oct 30, 2015
View reviewed changes

Add Dynamic Library for iOS

1ee6d30

nlutsenko added a commit that referenced this pull request Nov 20, 2015

Merge pull request #168 from lucasderraugh/master

d4a82c6

Dynamic Library for iOS Carthage Support

nlutsenko merged commit d4a82c6 into BoltsFramework:master Nov 20, 2015

nlutsenko added this to the 1.5.1 milestone Nov 20, 2015

nlutsenko added accepted and removed needs revision labels Nov 20, 2015

nlutsenko self-assigned this Nov 20, 2015

nlutsenko mentioned this pull request Nov 25, 2015

builds dynamic framworks parse-community/Parse-SDK-iOS-OSX#600

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic Library for iOS Carthage Support #168

Dynamic Library for iOS Carthage Support #168

lucasderraugh commented Oct 15, 2015

facebook-github-bot commented Oct 15, 2015

facebook-github-bot commented Oct 15, 2015

nlutsenko commented Oct 15, 2015

nlutsenko commented Oct 15, 2015

lucasderraugh commented Oct 16, 2015

lucasderraugh commented Oct 20, 2015

jsubida commented Oct 23, 2015

nlutsenko commented Oct 23, 2015

nlutsenko Oct 23, 2015

nlutsenko commented Oct 23, 2015

jsubida commented Oct 23, 2015

lucasderraugh commented Oct 23, 2015

nlutsenko commented Oct 27, 2015

lucasderraugh commented Oct 29, 2015

lucasderraugh commented Oct 29, 2015

nlutsenko Oct 30, 2015

nlutsenko commented Oct 30, 2015

lucasderraugh commented Oct 30, 2015

lucasderraugh commented Nov 19, 2015

nlutsenko commented Nov 19, 2015

nlutsenko commented Nov 19, 2015

lucasderraugh commented Nov 20, 2015

facebook-github-bot commented Nov 20, 2015

nlutsenko commented Nov 20, 2015

lucasderraugh commented Nov 20, 2015

nlutsenko commented Nov 20, 2015

facebook-github-bot commented Apr 5, 2016

Dynamic Library for iOS Carthage Support #168

Dynamic Library for iOS Carthage Support #168

Conversation

lucasderraugh commented Oct 15, 2015

facebook-github-bot commented Oct 15, 2015

facebook-github-bot commented Oct 15, 2015

nlutsenko commented Oct 15, 2015

nlutsenko commented Oct 15, 2015

lucasderraugh commented Oct 16, 2015

lucasderraugh commented Oct 20, 2015

jsubida commented Oct 23, 2015

nlutsenko commented Oct 23, 2015

nlutsenko Oct 23, 2015

Choose a reason for hiding this comment

nlutsenko commented Oct 23, 2015

jsubida commented Oct 23, 2015

lucasderraugh commented Oct 23, 2015

nlutsenko commented Oct 27, 2015

lucasderraugh commented Oct 29, 2015

lucasderraugh commented Oct 29, 2015

nlutsenko Oct 30, 2015

Choose a reason for hiding this comment

nlutsenko commented Oct 30, 2015

lucasderraugh commented Oct 30, 2015

lucasderraugh commented Nov 19, 2015

nlutsenko commented Nov 19, 2015

nlutsenko commented Nov 19, 2015

lucasderraugh commented Nov 20, 2015

facebook-github-bot commented Nov 20, 2015

nlutsenko commented Nov 20, 2015

lucasderraugh commented Nov 20, 2015

nlutsenko commented Nov 20, 2015

facebook-github-bot commented Apr 5, 2016