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

Dynamic linking for SPM #266

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

npvisual
Copy link
Contributor

Added .dynamic in the products definition to make sure the SPM bundle is dynamically linked.

Added `.dynamic` in the products definition to make sure the SPM bundle is dynamically linked.
@npvisual
Copy link
Contributor Author

See #265 for more information about the rational behind this PR.

@srdanrasic
Copy link
Contributor

How about we keep ReactiveKit static and add ReactiveKitDynamic product as an alternative option? I would like to keep the default one static since I think that's the most common use case.

This option allows us to keep both the statically and dynamically linked libraries. The "default" one will be (`ReactiveKit`) *should* be statically linked -- unless SPM can, in the future, manage to automatically decide what the best scenario is --  and `ReactiveKitDynamic` will be dynamically linked (always).
@npvisual
Copy link
Contributor Author

Sure thing. See the new commit. We could explicitly declare ReactiveKit as .static if you prefer.

@npvisual
Copy link
Contributor Author

All 3 options could be offered :

    products: [
        .library(name: "ReactiveKit", targets: ["ReactiveKit"])
        .library(name: "ReactiveKitStatic", type: .static, targets: ["ReactiveKit"])
        .library(name: "ReactiveKitDynamic", type: .dynamic, targets: ["ReactiveKit"])

@srdanrasic
Copy link
Contributor

Do you think it would make sense to offer all three options? We could do that.

@npvisual
Copy link
Contributor Author

The only reason I can think of is if we'd want more control down the road when Xcode can "magically" decide how to link those libraries inside frameworks and it doesn't really work as intended or there's no option to "force" one behavior or the other.

@srdanrasic
Copy link
Contributor

Forward thinking, sounds good to me :) Let's do all three options then. I'll release a new versions when you have the updated PR.

@npvisual
Copy link
Contributor Author

FYI. Still working through the kinks... Adding 2 libraries seem to be creating some issues. Trying to fix that.

@npvisual
Copy link
Contributor Author

Ok, after several tests, it seems that the main issue would be wrt Bond's dependencies.

So for example, if we implement the following strategy for Bond :

// swift-tools-version:5.0
import PackageDescription

let package = Package(
    name: "Bond",
    platforms: [
        .macOS(.v10_11), .iOS(.v9), .tvOS(.v9)
    ],
    products: [
        .library(name: "BondDynamic", type: .dynamic, targets: ["Bond"]),
        .library(name: "Bond", targets: ["Bond"])
    ],
    dependencies: [
        .package(url: "https://github.com/npvisual/ReactiveKit.git", .branch("fix/spm-dynamic")),
        .package(url: "https://github.com/tonyarnold/Differ.git", .upToNextMajor(from: "1.4.3"))
    ],
    targets: [
        .target(name: "BNDProtocolProxyBase"),
        .target(name: "Bond", dependencies: ["BNDProtocolProxyBase", "ReactiveKitDynamic", "Differ"]),
        .testTarget(name: "BondTests", dependencies: ["Bond", "ReactiveKit"])
    ]
)

Then the dependencies, for the target, point to :

  • either the dynamically linked RK library (as shown above),
  • or the statically linked RK library ("ReactiveKit" or "ReactiveKitStatic").

There's a way to impose conditions on target dependencies, but it's limited to platforms at the moment. I am just not sure if it matters, and if we can just use RK's dynamically-linked library and let SPM decide how it's gonna work when Bond is imported into an app.

We now have both explicitly declared a statically and dynamically linked library, in addition to the "undefined" RK library.
@npvisual
Copy link
Contributor Author

@srdanrasic : Here's the Bond Package.swift that I used to validate the changes (compiles and doesn't crash as long as both ReactiveKitDynamic and BondDynamic are used for the dependencies).

https://github.com/npvisual/Bond/blob/fix/spm-dynamic/Package.swift

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

Successfully merging this pull request may close these issues.

2 participants