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.

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

Add support for static frameworks in Carthage #1799

Closed
8 of 17 tasks
mfcollins3 opened this issue Nov 12, 2018 · 57 comments
Closed
8 of 17 tasks

Add support for static frameworks in Carthage #1799

mfcollins3 opened this issue Nov 12, 2018 · 57 comments

Comments

@mfcollins3
Copy link
Contributor

Short description of the issue:

RxSwift currently includes only dynamic frameworks when building with Carthage. As of Carthage 0.30.0, Carthage is capable of building static frameworks without the use of external tools. This can be achieved by duplicating the current set of RxSwift targets and changing the Mach-O Type field to Static Library instead of Dynamic Framework. The targets still build as static frameworks, however. The two sets of targets can co-exist and are output to different directories at build time (for example Carthage/Build/iOS for the dynamic framework and Carthage/Build/iOS/Static for the static framework).

It's my preference to use the static frameworks in my iOS applications instead of dynamic frameworks in order to improve launch times by reducing the number of frameworks that I am consuming that require dynamic linking.

I currently fork RxSwift and implement this on my fork. I'd be happy to contribute it back via pull request if the interest is there to add it to RxSwift.

Expected outcome:

Carthage builds produce static frameworks alongside the dynamic frameworks.

What actually happens:

Carthage is currently only producing dynamic frameworks for RxSwift when running carthage update.

Platform/Environment

  • iOS
  • macOS
  • tvOS
  • watchOS
  • playgrounds

How easy is to reproduce? (chances of successful reproduce after running the self contained code)

  • easy, 100% repro
  • sometimes, 10%-100%
  • hard, 2% - 10%
  • extremely hard, %0 - 2%

Xcode version:

  Xcode 10.0

⚠️ Fields below are optional for general issues or in case those questions aren't related to your issue, but filling them out will increase the chances of getting your issue resolved. ⚠️

Installation method:

  • CocoaPods
  • Carthage
  • Git submodules

I have multiple versions of Xcode installed:
(so we can know if this is a potential cause of your issue)

  • yes (which ones)
  • no

Level of RxSwift knowledge:
(this is so we can understand your level of knowledge
and formulate the response in an appropriate manner)

  • just starting
  • I have a small code base
  • I have a significant code base
@kzaher
Copy link
Member

kzaher commented Nov 14, 2018

Hi @mfcollins3 ,

I guess we can try to add it. It guess it depends on the PR will we be able to accept it. I'm fine with adding that to the main repo.

Feel free to make a PR and we can see how to proceed.

@mfcollins3
Copy link
Contributor Author

Thanks @kzaher. I'll put in the PR tonight.

@kzaher
Copy link
Member

kzaher commented Dec 16, 2018

Seems like the PR was closed.

I've looked at the carthage docs and it seems like it should be possible to use a small linker wrapper that generates static libraries.

https://github.com/Carthage/Carthage/blob/master/Documentation/StaticFrameworks.md

@mfcollins3
Copy link
Contributor Author

My PR was failing on Travis CI and I couldn’t figure out why. I closed it until I got a chance to work on it.

Those instructions are old. It’s much easier now. You just have to create a scheme/target with the Mach-O output type set to Static Library.

I have vacation coming up for Christmas. I’ll try to get the PR in next week.

@kzaher
Copy link
Member

kzaher commented Dec 23, 2018

Ok, I'll wait for you PR then.

@lordcodes
Copy link

Just wondered what issue it would cause with changing the existing targets to build as Static?

Would it cause issues with Cocoapods or building through other means?

@kzaher
Copy link
Member

kzaher commented Mar 8, 2019

@lordcodes Carthage integration.

@lordcodes
Copy link

@kzaher Oh okay I see, so Carthage requires separate static build targets. Thanks I wasn't aware.

@kzaher
Copy link
Member

kzaher commented Apr 9, 2019

@freak4pc We should also tackle this as a part of Swift 5.0 #1921

@kzaher kzaher added this to the RxSwift 5 milestone Apr 9, 2019
@kzaher kzaher removed this from the RxSwift 5 milestone Apr 9, 2019
@freak4pc
Copy link
Member

freak4pc commented Apr 9, 2019

I'm not too knowledgable with (or a fan of) Carthage. @mfcollins3 could you build a PR around this?

@kzaher
Copy link
Member

kzaher commented Apr 11, 2019

I wonder is it possible to just replace from dynamic library to static library and this is all that it takes.

@freak4pc
Copy link
Member

Would that be an issue for SPM?

@mfcollins3
Copy link
Contributor Author

@freak4pc Sorry, I forgot about this issue. I tried a while back to do this, but ran into issues getting the test suite to build properly. I’ve been using my own fork (https://github.com/nakedsoftware/RxSwift; see the carthage-static-frameworks branch) for my apps and forgot to go back to try to fix the test issue.

Basically in my fork, I just duplicated all of the targets using Xcode and made static variants of them. I modified the static targets to point to the same Info.plist, change the Product Name variable to match, and then change the Mach-O type to static library from dynamic framework. The result of doing this is that when Carthage runs, it outputs the dynamic library in Carthage/Build/<platform> and the static library in Carthage/Build/<platform>/Static. Then I’m able to link the static framework into a single dynamic framework that I use for third-party frameworks. The intent is to reduce the number of dynamic frameworks that I’m loading at launch time per Apple’s suggestions, and to make the frameworks easily usable from Swift playgrounds in Xcode.

I’m happy to create a PR from my fork, but I don’t know that I have the bandwidth immediately to figure out why the test suite fails. If someone can help me out figuring that out, I’d appreciate the assistance and getting this into the main framework so that I don’t have to manage my fork anymore.

@freak4pc
Copy link
Member

I really don't think duplicating all the targets just to get Carthage static libraries to work is a reasonable solution. If we can make the current target static and have all dependency managers work this way, that'd be the better choice, as @kzaher mentioned.

@freak4pc
Copy link
Member

I made an attempt here, lets see if CI eats it up: #1928

@mfcollins3
Copy link
Contributor Author

@freak4pc I’m fine with that since my ultimate goal is that I want it as a static framework and not a dynamic framework now that Xcode supports them. Since I’m just a consumer and not a core maintainer, that’s not my call to change the existing targets and how everyone consumes RxSwift. But you have my full support if you want to make that change.

@freak4pc
Copy link
Member

Yeah I 100% understand. I was just referring to the suggestion of making that PR, I don't think it would be a good solution since it impacts other non-Carthage consumers (which are the majority).

@freak4pc
Copy link
Member

Opened a bug in the Swift compiler: https://bugs.swift.org/browse/SR-10470

@freak4pc
Copy link
Member

I don't think we can do this until there's a newer Xcode version that resolves this known issuem, from Xcode 10.2 release notes:

Linking against a static Swift library might create a binary with missing type metadata because the object files that define the metadata inside the static archive are mistakenly considered unused. (47598583)

This can manifest as a Swift runtime error with a message such as: “failed to demangle superclass of MyClass from mangled name ‘’”.

Workaround: If you can rebuild the static library, try building it with whole module optimization enabled. Otherwise, add -all_load to the linker flags in the client binary to ensure all object files are linked into it.

The workaround doesn't seem to work (we already use WMO and there's no binary client to try -all_load on)

@freak4pc
Copy link
Member

This question will be better here I think - @lordcodes @mfcollins3 Are any of you using a Static Library with Xcode 10.2 and the Swift 5 compiler?

@mfcollins3
Copy link
Contributor Author

Yes, I just upgraded my apps over the weekend to Swift 5 and Xcode 10.2, and I upgraded my fork of RxSwift to the latest release. It's working fine for me. In my apps, I have a single Core dynamic framework that links all of my static frameworks and uses the '-all-load' and '-ObjC' arguments.

One thought on the issue above. In my workspace, I use playgrounds for prototyping ideas. I am able to use RxSwift in there because it's in my Core framework. I also have another dynamic framework for other code like Quick and Nimble that I want to run in playgrounds but not include in my app. You could try working around the problem by creating a dynamic framework for code to support your playgrounds, add the -all-load arguments, and then link the RxSwift static framework into it.

@freak4pc
Copy link
Member

The playground isn’t the problem anymore, it’s the fact since we are a provider and not a consumer, we can’t really help much with -all_load in our tests. I guess we could still release it and ask consumers to add the flag but I’m worried it would cause some confusion.

@bobgodwinx
Copy link

I guess I don't see the branch 👉🏾 fatal: Couldn't find remote ref staticlib

@freak4pc
Copy link
Member

Sorry - I guess I removed it after i failed :) it should be there again. Basically the first commit there should be enough to make it work but then the CI is the tricky part.

@mfcollins3
Copy link
Contributor Author

mfcollins3 commented Apr 17, 2019

@freak4pc @bobgodwinx I had some time today, so I pulled down the repo and checked out the staticlib branch. I first verified that I had the same issue as Travis when I ran the ./scripts/all-tests.sh iOS command. Then I managed to get it to work (for me, at least).

  1. In AllTests-iOS, add the -all_load flag to the Other Linker Flags setting.
  2. In Build Phases > Link Binary with Libraries, you have to add all five frameworks (RxSwift, RxCocoa, RxRelay, RxTest, and RxBlocking).

After I did that, the ./scripts/all-tests.sh iOS command completed successfully in a couple of minutes.

I cloned the repo a second time and reproduced this successfully to make sure that this was all that I had to do. If you want me to put in a PR to the staticlib branch, let me know and I can do it when I get home tonight.

EDIT: I didn't try the other targets, but I can later. I'm assuming they'll be the same.

@mfcollins3
Copy link
Contributor Author

I had a couple of extra minutes and was also able to get the tvOS and macOS tests to pass following the instructions above. Not sure about Linux. I don't have access to a Linux environment at the moment and haven't used the raw Swift tools or SPM.

@freak4pc
Copy link
Member

@freak4pc
Copy link
Member

@mfcollins3 From you experience, what should be the minimum Carthage version we support here? @bobgodwinx mentioned 0.31.x - are you aware of any limitation?

@mfcollins3
Copy link
Contributor Author

According to the Carthage website, it was introduced with version 0.30.0. I started using it as soon as they added support, so I can verify that it should work with 0.30.0 and above.

@freak4pc
Copy link
Member

Excellent. Thank you so much for all of your help on putting this together!

@bobgodwinx
Copy link

@freak4pc + all, there were major changes in 0.33.0 for Swift 5.0 so I think to be on the save side we should use that version in the change log because other version seems to giving some weird errors. But 0.33.0 works fine.

Secondly I see the Rx.. schemes now modified, can we test the behaviour of Cocoapods ? just to be sure?

@freak4pc
Copy link
Member

@bobgodwinx I don't mind mentioning we only support >= 0.33 on Carthage.

About CocoaPods - Pods and SwiftPM don't use Schemes at all, they simply use the source files directly.

Our CI have sanity tests for CocoaPods integration that seem to pass:

image

@freak4pc freak4pc removed the blocked label Apr 19, 2019
@kzaher
Copy link
Member

kzaher commented Apr 21, 2019

I think this should be resolved now. We'll release 5.0 hopefully soon.

@kzaher kzaher closed this as completed Apr 21, 2019
@rpassis
Copy link

rpassis commented Apr 28, 2019

Hey guys,

I just heard of this change via the RxSwift slack channel and had a few questions that I went to try and work it out for myself. Here are some of my findings and also sample projects that repros the warnings I am getting.

My test setup is as per below, all integrated using Carthage:

(1) DummyFramework - This is a dynamic framework that depends on the static RxSwift 5 build
(2) Main app - iOS app that depends on DummyFramework and also on the static RxSwift 5 build

Here's a repro of this setup https://we.tl/t-LXxWFwBu5j

It's worth noting that the setup above would be a fairly common scenario for many frameworks that consume RxSwift (i.e. RxDataSources etc)

If you build and run the main app you will notice several warnings:

Class XYZ is implemented in both `MainApp` and `DummyFramework`

Please correct me if I am wrong but doesn't this effectively mean that because RxSwift is being embedded as a static framework, and therefore part of the main executable for both MainApp and DummyFramework, each of these targets will have their own copy of the RxSwift implementations they use?

If that's the case, not only will this bloat our app sizes but also there is no way to guarantee which of the implementations will be used, or worse they may not even be the same.

Is there something I am missing here? And if not, has this been considered as part of the decision to change the framework to build as static instead of dynamic?

Thanks!
Rog

edit to add link to related issue on SO

@mfcollins3
Copy link
Contributor Author

mfcollins3 commented Apr 28, 2019

@rpassis In your specific test setup scenario, what you will want to do is remove the dependency on RxSwift from your Main app and leave it in DummyFramework. Within DummyFramework, you will want to add the -all_load (and -ObjC if using RxCocoa) flag to the Other Linker Flags setting. What this will do is embed RxSwift 5 into DummyFramework and will make it available to any consumers of DummyFramework (Main app). When you do this, your app will only link against a single copy of RxSwift and the warning will go away.

The reasoning for moving to static frameworks over dynamic frameworks is around optimizing pre-main() execution times. Dynamic frameworks are very expensive to load when it comes to application launch time. Apple's recommendation is no more than 6 dynamic frameworks (https://developer.apple.com/videos/play/wwdc2016/406/). The goal in switching to static frameworks over dynamic frameworks is to merge as many frameworks as possible into fewer dynamic frameworks (ideally one).

What I have been doing in my applications is forking RxSwift and other frameworks like Action, RxDataSources, etc., and duplicating the targets to create static variants, and then merging them into a single Core dynamic framework that is shared with my main application and its extensions. This way I am keeping the number of dynamic frameworks to load under 6, optimizing the launch time, keeping the total app size under control, and sharing the frameworks with everybody.

My initial suggestion was to have two sets of targets, one for a dynamic framework and another for a static framework. I'm not in a position where I could make the decision for everyone, so I made this suggestion to offer both. From the discussion above, there was concern about maintaining two sets of targets. The path forward is for the maintainers of libraries that build on top of RxSwift to follow RxSwift's lead and change their frameworks to be static frameworks instead of dynamic frameworks.

In my opinion, I think that it's better now that there is support for static frameworks for Swift for frameworks providers to go in that direction and leave the use of dynamic frameworks to the app developers in order to partition their app behavior as necessary while optimizing launch time as much as possible. The benefits to this are fairly well documented in several recent WWDC presentations and blog articles:

@mfcollins3
Copy link
Contributor Author

@rpassis One other thing to add for your test scenario. When linking RxSwift into your DummyFramework, you'll need to import DummyFramework first in your Swift source file in order to access the RxSwift module:

import DummyFramework
import RxSwift

@rpassis
Copy link

rpassis commented Apr 28, 2019

Practically speaking I think there is a huge downside to forcing this change on consumers of the library.

What happens if I use multiple libraries that all depend on RxSwift themselves? They would all also have their own embedded version of Rx (and they may all be different), and the same issue would surface no?

Even if I was only to look at a single library like the scenario you described, I’d still be giving control of what version of Rx is being used to a third party which is alone a big problem.

I appreciate the goal of reducing the number of dylibs needing linking at start time however I strongly believe what is being done here is incredibly detrimental to the workflow of devs in general and will really hurt adoption.

@freak4pc
Copy link
Member

This is tricky due to the fact Carthage is an all-or-nothing monster in a way. There’s no great way to force Carthage to go either dynamic or static so either side we choose will be unuseful for the other who might need to fork this.

I’m not a huge proponent or user of Carthage so I have little say here. If there is huge community objection to switching to Static by default, maybe we should reconsider it. @kzaher

@rpassis
Copy link

rpassis commented Apr 28, 2019

I should add this is not a Carthage issue, it is just a fact of how static frameworks are built as part of the main executable instead of as a separate framework that gets linked at load time.

Carthage will just build whatever you tell it to so the idea of forcing it to go static or dynamic doesn’t make sense since that decision is made by each framework configuration.

I don’t fully understand how Cocoapods packages the frameworks together and resolves dependencies but I think this could well be a problem there too.

@freak4pc
Copy link
Member

It’s not a problem on either CocoaPods or SPM as both of these disregard the original project and only deal with the sources and their own specification file.

@tonyarnold
Copy link
Contributor

If there is huge community objection to switching to Static by default, maybe we should reconsider it.

Please reconsider this change. It is impacting projects that use sub frameworks to organise their code, and the change is unnecessary on macOS where dynamic libraries don't have the same impact that they do on their smaller OS cousins.

Perhaps you could provide this as an option via a script, as suggested on the Carthage Static Frameworks page?

@mfcollins3
Copy link
Contributor Author

@freak4pc @kzaher I’m perfectly ok with the decision if you want to roll this back. I understand the controversial nature of this proposal and that there are very strong opinions on both sides, which are both perfectly valid. This probably needs to be a coordinated decision from the greater community. I’m fine maintaining my own fork like I have been doing for how I want to use RxSwift.

@tonyarnold
Copy link
Contributor

@mfcollins3 did your fork make this change directly in the pbxproj? Perhaps there's another way, such as moving the configuration to xcconfig files, and allowing you to override the specific settings via xcodebuildf -xcconfig MyRepo/MyOverrides.xcconfig or similar?

@layoutSubviews
Copy link

@mfcollins3 Thanks for your constructive response.
Ideally this project could accomodate both sides, maybe by splitting targets in two, or following @tonyarnold's advice?

Anecdotally, I have already spent several hours trying to upgrade to RxSwift 5 on my Rx-heavy, Module-heavy project and am still nowhere near having it build cleanly, and this despite having added a ton of build support to accomodate the static libraries as documented above.
I can't imagine how newcomers or big codebases alike will deal with this change without feeling like giving up on RxSwift entirely.

@mfcollins3
Copy link
Contributor Author

@mfcollins3 did your fork make this change directly in the pbxproj? Perhaps there's another way, such as moving the configuration to xcconfig files, and allowing you to override the specific settings via xcodebuildf -xcconfig MyRepo/MyOverrides.xcconfig or similar?

My fork duplicates the targets for the dynamic frameworks and changes the Mach-O setting to Static Library instead of Dynamic framework. When I build, I get both types, but I just use the static frameworks. I’ve found use for the dynamic frameworks in some special cases (unit test suites mostly), so I keep both around.

For other frameworks, I fork those as well and do the same. I update their Cartfiles to point to my fork instead of the main RxSwift repo.

@freak4pc
Copy link
Member

freak4pc commented Apr 30, 2019

I think using a separate xcconfig file could be a good middle ground. Thanks for all your valuable feedback here. I’ll try to whip up a quick PR later today so we have content to discuss.

@orj
Copy link

orj commented Apr 30, 2019

@mfcollins3 please also note that work is underway at apple to make the dyld loader much faster than it currently is (caching the linker fixup addresses). 2 years ago they introduced optimisations for Apple's frameworks/apps (which I believe was deployed in 11.x). I was under the impression that dyld was going to get this improvement for 3rd party apps at some point. Perhaps this year at WWDC?

See: https://developer.apple.com/videos/play/wwdc2017/413/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants