Skip to content

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