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

Revert Static Lib to Dynamic Framework #1960

Merged
merged 2 commits into from
Apr 30, 2019
Merged

Revert Static Lib to Dynamic Framework #1960

merged 2 commits into from
Apr 30, 2019

Conversation

freak4pc
Copy link
Member

Per discussion on Twitter and GitHub previously - It seems we didn't necessarily consider all of the downsides of limiting to Static Library usage on Carthage.

The main issue is that Carthage allows literally no customiation options for this. Since Dynamic Frameworks are the "default" option, I think it makes more sense to go back to our old default and have users who require the customization do some "manual" work.

That customization could be a custom Carthage script for people who're interested in RxSwift as a Static Library. For example, something along the lines of:

carthage update RxSwift --platform iOS --no-build
sed -i -e 's/MACH_O_TYPE = mh_dylib/MACH_O_TYPE = staticlib/g' Carthage/Checkouts/RxSwift/Rx.xcodeproj/project.pbxproj
carthage build RxAlamofire --platform iOS

We can put the above script under the Carthage category in our README file for users who "insist" on using RxSwift as a Static Library as a more advanced feature, and this could create a possible all-around solution for the "common" scenario (dynamic) and more advanced scenario (static).

Hopefully Carthage will one day allow specifying these sorts of things inside the Cartfile or a similar mechanism.

Thoughts? @kzaher @tonyarnold @mfcollins3 @layoutSubviews

@@ -3896,7 +3896,7 @@
INFOPLIST_FILE = "$(SRCROOT)/RxRelay/Info.plist";
INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks";
LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks";
MACH_O_TYPE = staticlib;
MACH_O_TYPE = mh_dylib;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can safely delete the MACH_O_TYPE entries entirely. They'll default to Xcode's recommendation, which is mh_dylib.

Copy link
Member Author

@freak4pc freak4pc Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but in a way I always prefer explicitness (not that the default will change or anything but still I don't see harm in that). The second thing this helps with is the above sed script works. Even though it could replace MACH_O_TYPE = as well. Don't have a huge preference to this :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually removing it will leave no trace of it in the project.pbxproj so in a way I think leaving it explicit allows for better communication and also will allow this manual customization in the form of a bash script like the above

@freak4pc freak4pc changed the base branch from master to develop April 30, 2019 08:01
@freak4pc
Copy link
Member Author

Added possible README instructions.

@kzaher
Copy link
Member

kzaher commented Apr 30, 2019

I'm not sure I understand,

dynamic frameworks are bad because they increase your startup time. Cocoapods use static libs, Swift team added explicit support for static libs, Apple advises against large number of dynamic frameworks, Blaze uses static frameworks. What in the world is the argument for dynamic frameworks?

@freak4pc
Copy link
Member Author

I've opened it for discussion for this specific reason. I have no strong opinion on this as I prefer package managers that give developers a choice :P I'm mainly interested in a common ground that fits most of the developers.

The main issue noticed here is the "virality" of static libraries - e.g. if you have many small frameworks that all consume RxSwift using Carthage, each of these contains an individual static copy of RxSwift.

For example if you take in both RxOptional and RxSwiftExt and RxDataSources - at least to my understanding, they all depend on RxSwift which is now a Static Library.

About CocoaPods - the default is still Dynamic. (When you pod init you get a Podfile with a dynamic library) and Apple's guidance is basically correct but is not "basic knowledge".

What companies usually do is similar to what @mfcollins3 suggested - a single Dynamic Framework containing many static libraries. But again, since I'm not a consumer of Carthage I'm not necessarily sure on how critical this is or isn't (that's why @tonyarnold and the others are tagged above)

@kzaher
Copy link
Member

kzaher commented Apr 30, 2019

The main issue noticed here is the "virality" of static libraries - e.g. if you have many small frameworks that all consume RxSwift using Carthage, each of these contains an individual static copy of RxSwift.

Yes, we should change this for them to be also static. None of them will contain a copy of RxSwift as far as I can tell.

As far as I can tell nobody wants to use dynamic library. It was a crutch for the inadequacy of the Swift compiler, not a preferred solution.

If we are reverting this I would want to be sure is it because there are some good technical arguments. If we are going to do it because the ecosystem is lazy and doesn't want to transition in general to dynamic frameworks, I'm also fine if that's the case.

If it is because some people are unhappy because they are using RxSwift in team environment and are now starting holy wars again how we are horrible and every change is just a fuel for the fire (in this case I'm not sure I care much).

But anyway, this is the place where people can voice their opinion and concerns. I don't stalk people on Twitter. Make up your mind and voice your reasons here. I'm fine with reverting if there are good technical arguments or people voice that the ecosystem doesn't want to do this.

@freak4pc
Copy link
Member Author

If we are reverting this I would want to be sure is it because there are some good technical arguments. If we are going to do it because the ecosystem is lazy and doesn't want to transition in general to dynamic frameworks, I'm also fine if that's the case.

Agreed - this PR was opened just to allow choosing either option whenever those technical arguments are made.

Care sharing your thoughts @tonyarnold @layoutSubviews ?

@tonyarnold
Copy link
Contributor

The default for all of our existing Xcode-based tooling supports dynamic frameworks out of the box - more so for Carthage than others, as it is just reusing Xcode's project setup. Yes, Xcode also supports static archives, but that's not the default, nor how Apple themselves distribute library-based dependencies (frameworks!).

As far as I can tell nobody wants to use dynamic library. It was a crutch for the inadequacy of the Swift compiler, not a preferred solution.

I'm not sure where this comment is coming from at all. Dynamic frameworks are (and have been) the basis of all distributable frameworks on Apple's platforms since Mac OS X was first released. Limitations in iOS have meant that people have come up with workarounds like using static frameworks (which aren't a supported configuration).

You've both stated you don't use, nor particularly care about Carthage as a tool - that's fine, and reasonable - but as a user of Carthage, I'm telling you that the change you've made here negatively impacted my projects that use RxSwift.

Look, it's your project, so do as you please, but you're asking a community of people who rely on your library to change the default setup of entire projects to support a change Apple themselves have said will be unnecessary in future (see the earlier comments in #1799 about the improvements to dyld in iOS 11 and later).

because the ecosystem is lazy

Ouch. Definitely not it.

@bobgodwinx
Copy link

I would say the people advocating for dynamic libs have a point. Although Static Libs are my favourites, there seems to be a huge technical debt in configuration and setup when you have multiple frameworks that use RxSwift which could lead to bloating the entire App. However I am thinking of a solution with the Scheme here is the hack I found for now but it is too geeky.. :

@tonyarnold
Copy link
Contributor

But anyway, this is the place where people can voice their opinion and concerns. I don't stalk people on Twitter. Make up your mind and voice your reasons here.

@kzaher I'm sorry if I've offended you in any way. Please note that I did voice my opinion and concerns both here, and on Twitter (and continue to do so).

I value the work that you and the team have done with RxSwift, regardless of my disagreement with the change that kicked this all off.

I've given my feedback on this issue, so unless you want more information from me I'm going to leave you folks to make up your minds about this issue and butt out.

@freak4pc
Copy link
Member Author

freak4pc commented Apr 30, 2019

I think that regardless of the written content we find, we also need to consider the ecosystem of other dependency developers.

Looking at other large iOS frameworks: ReactiveSwift, Alamofire, SnapKit, Kingfisher, CryptoSwift - are all dynamic.

Furthermore, the more I tried, I couldn't find a single known iOS dependency that sets MACH_O_TYPE to Static.

Also, When you remove the MACH_O_TYPE configuration in Xcode, the default you get is dynamic as well.

Basically everything here makes me lean towards going back to dynamic - it seems to be the widespread used community standard (for Cocoa in general) and it's also the default in the tooling we have around Apple-related development (Xcode, CocoaPods, etc.)

Even if a Static Library could possibly be more optimized - it's something for advanced developers to pick and do on their own (perhaps with the aforementioned script I provided), and probably shouldn't be the default. It was definitely not something embraced by the Apple developer community, so it seems.

@rpassis
Copy link

rpassis commented Apr 30, 2019

I personally feel if the discussion here is around performance it needs to be backed by numbers. Just saying static is better because it’s faster is not a convincing argument in any way. The counter point of reducing link time is increasing load time because we are bloating the executable.

This is a huge inconvenience for non cocoapods users (not just carthage but manual integrators too) for no demonstrable gains.

It will also affect the entire ecosystem that provides frameworks that depend on Rx (and not all Rx libraries live inside the RxCommunity repo).

I think you guys might be underestimating the impact this will have on current usage and adoption of the framework and I am a strong NO for this change.

@kzaher
Copy link
Member

kzaher commented Apr 30, 2019

I personally feel if the discussion here is around performance it needs to be backed by numbers.

Agree.

Just saying static is better because it’s faster is not a convincing argument in any way.

Well it is faster, can't tell by how much since nobody on the internet hasn't published any recent meaningful benchmarks.

The counter point of reducing link time is increasing load time because we are bloating the executable.

If you don't need something to run your app. Don't link it into your binary. Dynamic linking will still load all of those unused symbols in the memory as far as I can tell, the symbols just won't be resolved eagerly, so it should be as bad.

This is a huge inconvenience for non cocoapods users (not just carthage but manual integrators too) for no demonstrable gains.

@tonyarnold Can you please provide links that show that Apple wants the entire ecosystem to move to dynamic libraries and the performance impact? I didn't get that from you.

2016
https://developer.apple.com/videos/play/wwdc2016/406/
Don't use dylibs.

2017
https://developer.apple.com/videos/play/wwdc2017/413
2:52 You should use fewer dylibs.

https://devstreaming-cdn.apple.com/videos/wwdc/2017/413fmx92zo14voet8/413/413_app_startup_time_past_present_and_future.pdf?dl=1
Slide 13:
Do less!
• Embed fewer dylibs

Why did Apple add official support for static libs in the Swift compiler?

Why did Carthage add support for static frameworks? https://github.com/Carthage/Carthage/releases/tag/0.30.1

Are there any benchmarks? Videos don't provide any data. Not even biased benchmarks made by the engineers working on this.

I can respect opinions, but I would appreciate links to the apple domain more.

Having said that, I personally don't care much. If Carthage users want to use dylibs, I can change it. I don't really care that much do Carthage users want to do it because it's easier, or they think that the performance/convenience tradeoff isn't worth it, or some other reason.

Ouch. Definitely not it.

So far I haven't seen benchmarks or links telling otherwise, so it would seem to me that it is. Again, maybe it's not, just saying that you haven't provided any evidence for the opposite.

I think I've been thinking about this in the wrong way. If we leave static libraries, people will complain and we'll have to respond. For the dynamic one people didn't complain much, so let's go with the dynamic ones.

@kzaher kzaher merged commit 3ae39ab into develop Apr 30, 2019
@freak4pc freak4pc deleted the dynamic-framework branch April 30, 2019 11:27
@freak4pc
Copy link
Member Author

freak4pc commented Apr 30, 2019

I guess we'll have to cut a 5.0.1 ? (or reset 5.0.0 tag but I remember you prefer not doing that)

@kzaher
Copy link
Member

kzaher commented Apr 30, 2019

Again, we shouldn't be changing tags.

@kzaher
Copy link
Member

kzaher commented Apr 30, 2019

I'll make 5.0.1.

@Canariseven
Copy link

I have a little problem, I can't detect the tag 5.0.1
what happened ?

Cartfile ->
github "ReactiveX/RxSwift" ~> 5.0

Terminal ->
*** Checking out RxSwift at "3.0.0.alpha.1"

@rpassis
Copy link

rpassis commented Apr 30, 2019

Are you running carthage update? It seems to work fine here

github "ReactiveX/RxSwift" ~> 5.0
$ carthage update RxSwift --platform ios          
*** Fetching RxSwift
*** Checking out RxSwift at "5.0.1"
*** xcodebuild output can be found in /var/folders/73/lnfrs9hx1nn6khr3r3lzh5600000gp/T/carthage-xcodebuild.ADcoWR.log
*** Downloading RxSwift.framework binary at "ShaiTheBravest"

@Canariseven
Copy link

Canariseven commented Apr 30, 2019

Yes

$ carthage update
*** Fetching SwifterSwift
*** Fetching BkoolUtilitiesiOSLib
*** Fetching RxSwift
*** Fetching BkoolHTTPFetcher-IOS
*** Checking out RxSwift at "3.0.0.alpha.1"
*** Checking out SwifterSwift at "4.6.0"
*** Checking out BkoolHTTPFetcher-IOS at "1.1.1"
*** Checking out BkoolUtilitiesiOSLib at "1.2.1"
*** xcodebuild output can be found in /var/folders/sq/vzy_95nj6bl86k1rp_nghbqr0000gp/T/carthage-xcodebuild.fPHO04.log
^C
$ carthage version
0.33.0

@Canariseven
Copy link

Canariseven commented Apr 30, 2019

I will try to try erasing the carthage cache

Imposible
if I modify the cartfile with ->

github "ReactiveX/RxSwift" ~> 4.5

The result is:

*** Checking out RxSwift at "4.5.0"

But
If modify the cartfile to

github "ReactiveX/RxSwift" ~> 5.0

Result:

*** Checking out RxSwift at "3.0.0.alpha.1"

@Canariseven
Copy link

Sorry I am very stupid, I had dependence on another personal liberty, that had not updated his Cartfile, then there was a conflict.

My deepest apologies. And above all, thank you very much for this great tool!

@keith
Copy link
Contributor

keith commented Apr 30, 2019

For future reference here's some solid data on how this affects app launch time https://blog.automatic.com/how-we-cut-our-ios-apps-launch-time-in-half-with-this-one-cool-trick-7aca2011e2ea

In our experience ours also dropped at least this much by switching entirely to static frameworks

@bobgodwinx
Copy link

@freak4pc + @kzaher IMO I don’t think we should be flip flopping like this. I would wait for at least 2 weeks to hear all the complains while searching for an optimal solution. There was an issue where we discussed the static libs and people should have brought up all their opinions there and not afterwards. All I am say is that let’s wait and find try find a solution first.. just like we did on the previous issue where it was long discussed. I know people will always complain but wait a moment before cutting 5.0.1

Sent with GitHawk

@freak4pc
Copy link
Member Author

@bobgodwinx Appreciate your opinion but I’ll have to respectfully disagree - it was sort of a mistake to switch cold turkey to a non-default build option, but it would’ve possibly been hard finding that error without the release.

Right now there was reason to wait anymore. We’ve discussed plenty and reverting make sense while still offering a script to go static if needed - which takes care of both scenarios.

@orj
Copy link

orj commented Apr 30, 2019

Dynamic libraries (frameworks) have a variety of benefits over static frameworks.

  • Reduction in memory usage: Static linking duplicates code in to code segments.
  • Reduced disk space.
  • Shared memory: The dynamic linker can share VM pages between apps that link a dynamic framework. Frameworks in /System and /Library do not get loaded more than once into memory. In user app space a framework in a app bundle can be shared between extensions or subprocesses too reducing memory usage.
  • Guaranteed canonical type linkage: If your app has two dynamic frameworks that link against the same static framework you end up with 2 versions of that static code in your app’s memory space and type/symbol resolution is random and/can get over written as dyld loads further .frameworks. This causes all sorts of problems with ObjectiveC categories not sure how it affects swift exactly but I assume the ObjC bridged types will get buggered up. Dyld will fortunately bitch quite loudly when it notices this.

The canonical way of sharing code across processes on modern systems has always been dynamic libraries. Unix .so, Windows .dll, Mac .frameworks (actually .so’s with a custom container file structure).

Static libraries (basically just simple archives of .o files - static frameworks are no different) are a legacy of times before we had the benefit of dynamic linking.

One of the major reasons behind Swift’s push for ABI and module stability is to reap all the benefits of dynamic linking.

Yes there are some performance trade offs. Dynamic libraries are a space vs time trade off. But they also come with a bunch of other benefits too around tooling.

The reason behind preferencing static frameworks over dynamic in iOS are rapidly receding into the past (as Apple CPUs get faster and dyld is improved).

The idea of switching RxSwift to a static framework by default was an absolute unequivocal mistake.

@freak4pc
Copy link
Member Author

freak4pc commented Apr 30, 2019

The idea of switching RxSwift to a static framework by default was an absolute unequivocal mistake.

As much as I enjoy a mocking post-mortem criticism from people who've never contributed code to this repo, never participated in a single discussion (even on the specific topic of static libraries that was discussed for over 2.5 weeks here before this release), etc ... maybe just ... don't ?

People make mistakes, and that's fine. We're not employees of RxSwift but contributors who take a lot of time from their day to maintain this project. Please be respectful of that.

I still don't think it's a mistake like a preference, but the preference of a library should be the consensus of the community and not the preference of a few people. We've taken responsibility and reverted the code and pushed a new release in basically under 12 hours since the initial complaint (which was on Twitter, and not even on a GitHub issue). Can you show me an (even paid or commercial) project that does that so quickly?

The rest of your post was very informative and interesting. Thanks for the information.

I invite you to be an active member of this community, contribute to discussions, code, etc. and help steer the boat the next time a similar "mistake" could possibly happen, but providing such an aggressive sentence as an outsider is absolutely ridiculous and disrespectful in my eyes.

@orj
Copy link

orj commented Apr 30, 2019

Sorry @freak4pc I wasn’t trying to mock. I unreservedly apologise if it was taken that way. I very much appreciate the work of the RxSwift maintainers.

You are right. People make mistakes. I made one right there in my choice of words.

I also very much appreciate how responsive the RxSwift community has been to reversing this change.

@orj
Copy link

orj commented May 1, 2019

@freak4pc you are right that it can be a preference to use static linking, and in fact at times it would be the appropriate behaviour. For example if you were building a command line util that you don’t want to have any external dependencies (beyond the system frameworks) - the latest builds of Carthage appear to now statically link CarthageKit (and RecativeCocoa) for example. Or if you need a simplified deployment artefact (GoLang preferences static linking but it is easier in that community as all dependencies are sources).

And you could do this with macOS/iOS targets too but it would require build tooling support. Cocoapods provides this support somewhat. But not everyone uses that.

@freak4pc
Copy link
Member Author

freak4pc commented May 1, 2019

Thanks for the clarification, I appreciate that. And yup I’m aware of that, this is one of the reasons Carthage frustrates me as a package manager so much. The amount of configurability without additional tooling or script hackery is close to none.

Anyways I’m glad we were able to find a solution that fits all shoes (seemingly) :-)

@kzaher
Copy link
Member

kzaher commented May 1, 2019

@orj WOW

Reduction in memory usage: Static linking duplicates code in to code segments.

This is not true. If the linkers are dumb, sure, but they can't be. If this was true it would mean that singletons and static variables would be impossible, which isn't the case. I'm linking about 20000 static libraries in the current app, if that was the case, imagine how easy would it be to get a promo by just changing the linker to not be a complete idiot.

Reduced disk space.

How on earth. If you have everything statically linked, then you can statically build graphs and remove unused symbols, which you can't do for dynamic libraries because anyone could use any entry point.

Shared memory: The dynamic linker can share VM pages between apps that link a dynamic framework. Frameworks in /System and /Library do not get loaded more than once into memory. In user app space a framework in a app bundle can be shared between extensions or subprocesses too reducing memory usage.

You are not working on shared frameworks on the iOS level. You are misunderstanding the question. The question isn't should iOS use dynamic frameworks, but should your app use dynamic frameworks.

Guaranteed canonical type linkage: If your app has two dynamic frameworks that link against the same static framework you end up with 2 versions of that static code in your app’s memory space and type/symbol resolution is random and/can get over written as dyld loads further .frameworks. This causes all sorts of problems with ObjectiveC categories not sure how it affects swift exactly but I assume the ObjC bridged types will get buggered up. Dyld will fortunately bitch quite loudly when it notices this.

You should not do this ever, otherwise your app will probably crash. This is the main reason why having a dumb package manager like Carthage is a bad idea. Consumer should decide how they want to consume the library not the library authors. Library authors should decide on the API and code only. Maybe I'm crazy, but hey. That's just me. All package managers except Carthage do this (CocoaPods, Swift Package Manager, Blaze/Bazel).

The canonical way of sharing code across processes on modern systems has always been dynamic libraries. Unix .so, Windows .dll, Mac .frameworks (actually .so’s with a custom container file structure).

You are not working on iOS or Windows or any other OS level. Stop fantasizing. And yes, that was a huge dumb idea in retrospect. Ever heard of dll hell? Docker? Containers? What problems do you think they are trying to solve?

Static libraries (basically just simple archives of .o files - static frameworks are no different) are a legacy of times before we had the benefit of dynamic linking.

No, not true. Do you see any binaries shipping with thousands of dynamic libraries? Well why that might be... Is it because people do small projects only or they use static linking.

One of the major reasons behind Swift’s push for ABI and module stability is to reap all the benefits of dynamic linking.

Well why did they add support for static libraries after x years then? Why did Carthage start supporting it? You would think they would be satisfied with just dynamic ones, wouldn't you say? Why are all unicorn companies in favor of static linking? Are they all delusional?

Yes there are some performance trade offs. Dynamic libraries are a space vs time trade off. But they also come with a bunch of other benefits too around tooling.

No, it's not space time tradeoff, it's developer convenience vs time tradeoff.

The reason behind preferencing static frameworks over dynamic in iOS are rapidly receding into the past (as Apple CPUs get faster and dyld is improved).

I'll trust that Apple is serious about linking when they optimize ld64 or at least publish an easily buildable open source version. That linker is couple of times slower than others. Yes, dynamic linkers should get better, but that doesn't mean it's 0 cost operation, or that static linking is somehow relic of the past.

The idea of switching RxSwift to a static framework by default was an absolute unequivocal mistake.

Yes, it was, but it wasn't because of any of the reasons you are saying. It was dumb because Carthage consumers don't have any control over how the library is consumed without ugly hacks and the ecosystem is built around dynamic libraries and the network effects are preventing the change.

We have ~10 issues at any time in our repo. #1799 Was opened half a year ago and it was clear for a long time that we'll do this. I first thought that there might be some backslash, but didn't see anything like any kind of serious backslash. I was also prepared to migrate entire RxSwiftCommunity to static libs. I didn't see a huge problem with migrating to static, and if somebody used RxSwift, they could just pack all of this into a single dynamic library and just do import RxShared; import RxSwift. That was basically all they had to do on their end. We have to migrate dozens of projects and change dozens of CI pipelines. I've already started to propagate the work on RxDataSources. Look at 5.0.0 release..

We've reverted the change because of the reason I've just said, not because any of the BS reasons you've mentioned. Indirectly we've helped you and your company, which is great. You are welcome.

We've first invested my time and @freak4pc 's time into supporting a package manager which non of us personally use. Then we've honestly tried to help that community using the project more efficiently. Then we've realized that the community wouldn't want to change, which is fine. I'm lazy, I don't treat laziness as something bad. I don't want to do some work if I don't really have to. Then me and @freak4pc have again invested our time into helping out people using Carthage.

So after all of this work, after we've maintained the project for you and had best intentions, after we've help you and your company, you come here with BS reasons and start pissing on us and basically claiming we are morons.

Thanks a lot. Thanks for wasting my time on writing this response.

What is wrong with you people. Or better yet, what is wrong with me for getting into all of this shit. Yes, I was dumb for that, but I learn. Never again.

@freak4pc
Copy link
Member Author

freak4pc commented May 1, 2019

I think this is escalating beyond the scope of what we want to discuss in this repo for now. As I mentioned, I invite each and every one of you to become more involved in the ongoing discussions on the future of this library, if it's something you're curious about and interested in.

Since the issue is resolved I'm gonna lock this for the time being.

@ReactiveX ReactiveX locked as too heated and limited conversation to collaborators May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants