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

Implementation of same-package product dependencies #7331

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stackotter
Copy link

@stackotter stackotter commented Feb 11, 2024

The implementation accompanying a pitch that would allow targets to depend on products within the same package.

Motivation:

These changes allows dynamic linking between sibling targets, alleviating code-size and type-casting issues in certain situations. They also allow for executables to implement plugin systems without resorting to nested packages. See the proposal for motivating examples.

Modifications:

  • Created the Target.Dependency.product(name:conditions:) overload
  • Added suitable representations for same-package products (a.k.a. innerProducts) throughout SwiftPM's serialization, package model, and package builder code
  • Updated PackageBuilder.swift and PackageGraph+Loading.swift to correctly construct and build package graphs containing same-package product dependencies

Result:

Targets can now depend on products within the same package by using the Target.Dependency.product(name: ..., conditions: ...) static method when creating a target's dependencies.

Tasks

  • Allow targets to depend on products within the same package
  • Check for cycles
  • Add tests
  • Ensure that plugins can't depend on same-package products when it wouldn't make sense (will have to make that more rigorous)

@MaxDesiatov
Copy link
Member

MaxDesiatov commented Feb 11, 2024

I don't think this is the right way to solve the problem. If the goal is to allow controlling how targets and products are linked, it should be done directly at the place of dependency declaration, and not in a roundabout way that allows depending on products in the same package.

Let's say I want to link a private target dynamically, is my only option then to expose publicly it in my package as a product?

Another problem with this approach is that it constrains users of a package to specific linking kind without any ability to control it at the place of dependency declaration.

A slight terminology nit: documents like the one linked from PR description are usually not called evolution proposals until they successfully pass the pitch phase and are scheduled for review by the Swift team.

I advise gathering some feedback on Swift Forums and getting to a consensus on a pitch first, before proceeding with the implementation.

@stackotter
Copy link
Author

stackotter commented Feb 12, 2024

Hey Max thanks for the feedback,

I only created the implementation last night (even though it hasn't been pitched since the initial forum thread) so that I could test it out on my own projects and see if it solved my issues in practice.

I don't feel like this solution really restricts users of a package to a single linking kind anymore than they already would be, because the issues the pitch is addressing only arises when a package is already imposing dynamic linking of products for one reason or another (e.g. a plugin system as discussed in the pitch).

I have thought about the alternative solution of allowing targets to control the linking mode when depending on another target, however that alone wouldn't solve the issues that this pitch is trying to address;

let package = Package(
    name: "Library",
    products: [
        .library(name: "LibA", type: .dynamic, targets: ["A"]),
        .library(name: "LibB", type: .dynamic, targets: ["B1", "B2]),
    ],
    targets: [
        .target(name: "A", dependencies: [
            .dynamic("B1") // Ignore the syntax, just for demonstration
        ]),
        .target(name: "B1"),
        .target(name: "B2"),
    ]
)

In the above example, LibA is a dynamic library liblibA.dylib linked to libB1.dylib, while LibB is a dynamic library liblibB.dylib containing both B1 and B2 (statically). The code of B1 would still be duplicated between libB1.dylib and liblibB.dylib, causing the type-casting and code-size issues that this pitch is trying to address. It could potentially be made to work if you also allowed the linking mode used by products when linking against targets, however that starts getting a bit tedious. I'll be interested to see what people think about that option (I'll add it as an alternative considered).

The reason that allowing same-package product dependencies works to solve all of the cases that motivated this pitch (of which private target linking wasn't one, but more on that next), is that these issues only arise when your package is a dependency of another package, and someone is depending on more than one product from your package where one of them is a dynamic library and both of them contain some code in common. That's why in my eyes the solution will likely involve products in some way shape or form.

Controlling the linking of private targets is an important oversight, thanks for bringing that up. Since a private target wouldn't be directly included in any products, you would likely be able to avoid the limitation with target-target linking that the manifest above demonstrates, so it could be addressed by additionally introducing a way to control linking between targets. Now that I think about it, having both solutions (target-target linking control & target-product same-package dependencies) would provide even more linking flexibility and could provide the tools to solve almost any linking problems that one might face.

Apologies if all that would've been better suited to a pitch thread, I'll create one once I've updated the pitch with the points discussed above.

@stackotter
Copy link
Author

I realised that the link to the original pitch in the proposal draft is probably easily missed. Here's the original forum pitch thread that sparked this proposal draft: https://forums.swift.org/t/pitch-swiftpm-allow-targets-to-depend-on-products-in-the-same-package/57717

@MaxDesiatov
Copy link
Member

let package = Package(
    name: "Library",
    products: [
        .library(name: "LibA", type: .dynamic, targets: ["A"]),
        .library(name: "LibB", type: .dynamic, targets: ["B1", "B2]),
    ],
    targets: [
        .target(name: "A", dependencies: [
            .dynamic("B1") // Ignore the syntax, just for demonstration
        ]),
        .target(name: "B1"),
        .target(name: "B2"),
    ]
)

In the above example, LibA is a dynamic library liblibA.dylib linked to libB1.dylib, while LibB is a dynamic library liblibB.dylib containing both B1 and B2 (statically). The code of B1 would still be duplicated between libB1.dylib and liblibB.dylib, causing the type-casting and code-size issues that this pitch is trying to address.

My main question here would be then: why do LibA and LibB require to be declared as .dynamic products in the first place? I don't see a clear use case for that in the presence of specifying linking kind at the place of dependency declaration. Products should just stick to the default .auto declaration instead:

let package = Package(
    name: "Library",
    products: [
        .library(name: "LibA", targets: ["A"]),
        .library(name: "LibB", targets: ["B1", "B2]),
    ],
    targets: [
        .target(name: "A", dependencies: [
            .dynamic("B1") // Ignore the syntax, just for demonstration
        ]),
        .target(name: "B1"),
        .target(name: "B2"),
    ]
)

As specified this way, we can infer the right way to link B1 based on a computed products and targets graph in each case. swift build --product LibB would build a statically linked B1 and B2, swift build --product LibA would dynamically link B1, and plain swift build without products specified would dynamically link B1 into both LibA and LibB, unless we provide a way to override that as well.

But the main question is: why do you need to link dynamically in the first place? Dynamic linking is ok for system libraries that ship in an SDK that multiple apps can reuse at the same time. If you're building a single app with specific versions of your packages that can't be shared with other apps (and this is always the cases for apps on the iOS App Store), just link everything statically. That will not only avoid the duplication, but will also be beneficial for final binary size and app startup time. And that's the default for .auto anyway.

@stackotter
Copy link
Author

In my specific use case, the reason is that my app has a plugin system, and plugins written in Swift need to link against the same copy of the plugin API as the app itself (otherwise objects passed between the two just don't work correctly whatsoever, e.g. a plugin wouldn't be able to use an any Event passed to it by the app since type-casting would just fail for every concrete Event type), so dynamic linking between the app and its plugin API is required (static linking to the plugin API would give the plugin and the app their own separate copies of the plugin API). I wouldn't be using dynamic linking if I didn't have to, I generally dislike it a lot.

If it was only a thin plugin API that had to be in a separate repo/package, none of this would be an issue for me, but the dylib with the plugin API isn't just the plugin API, it necessarily includes basically the whole implementation of the app except for the UI (plugins can interact with a lot), and many changes that I make span across both the top-level package and the nested package (making splitting them across repos a non-option for maintainability).

Additionally, there are multiple other people in the original pitch thread who claim that they've wanted this for quite a while and that they want a way to fix their dynamic linking issues without resorting to multiple nested Package.swift's (which then requires @_exported if the common code is intended to be used by consumers) or splitting their project into separate repos (which often makes development/contributing tedious, and doesn't make sense). However, none of them were super specific about their use-cases so I can't tell you exactly why they're using dynamic linking in the first place.

@MaxDesiatov
Copy link
Member

MaxDesiatov commented Feb 13, 2024

I understand there is a need for dynamic linking in certain cases, but I'll reiterate that this should be solved by providing additional linking controls, which is orthogonal to the way that dependencies are specified within a package.

Expressing a dependency on products within the same package would have a similar effect only because of the limited way linking controls can be specified right now. We should lift those limitations on linking controls first of all. Not sure it's worth spending time instead on workarounds that bring their own limitations, like the aforementioned inability to keep dynamically linked targets private.

@stackotter
Copy link
Author

I agree that a more flexible solution is better. If people aren't opposed to a more overarching set of changes regarding linking behaviour then I'd be happy to explore that avenue instead. I think the original idea was to keep the proposal simple and self-contained (I wasn't the original pitcher so I can't say for sure).

If the same-package products idea gets scrapped we could instead (as you've mentioned) introduce controls for configuring linking modes of targets depending on targets (and possibly products depending on targets). I think you'd often end up with more dynamic libraries than strictly required (e.g. in the example above with A, B1, and B2, you'd end up with B1 and B2 as separate dylibs), but that may not be a big issue in practice.

Aside

The following isn't necessary to solve the code size and type-casting issues (as long as you're happy with B1 and B2 being in separate dylibs), but to be able to express the linking setup that same-package product dependencies achieve in the A, B1, B2 example above, you'd probably end up needing a way to depend on bundles of targets like so,

.target(
    name: "A",
    dependencies: [
        .dynamicBundle(["B1", "B2"]) // ignore naming for now
    ]
)

which ends up looking a lot like depending on an anonymous product. But I'll drop that idea completely for now.

Exploring a new solution

Here's a small example setup where I've explored a new solution and annotated some potential pain points.

// The app's Package.swift
let package = Package(
    name: "App",
    products: [
        .executable(name: "App", targets: ["App"])
        // If `AppCore` and `AppRenderer` were to be part of the same product,
        // SwiftPM would somehow need to get told that `AppCore` and `AppRenderer`
        // must be separate dylibs when compiling the composite product for
        // dynamic linking. That's only required because `App` depends on both
        // targets as separate dylibs and the library is intended for plugins
        // to use (which need to link the same way as `App`), it isn't a rule in
        // general so it can't and shouldn't be automatically inferred. I haven't
        // yet thought of a nice API for specifying when building the product
        // as a dynamic library, that certain targets should be separate dylibs. We could
        // go with an API such as `[.dynamic("AppCore"), .dynamic("AppRenderer")]`,
        // but that implies that they're dynamically linked even under static linking,
        // which isn't desirable.
        //
        // Requiring that the issue is worked around by splitting the product into two
        // products like so could be considered an acceptable compromise for a
        // first set of changes (or forever if it doesn't bother enough people).
        .library(name: "AppCore", dependencies: ["AppCore"]),
        .library(name: "AppRenderer", dependencies: ["AppRenderer"]),
    ],
    targets: [
        // Both of App's dependencies must always be dynamically linked to,
        // so no need to design a flexible auto linking API for this part.
        .executableTarget(
            name: "App",
            dependencies: [
                .dynamic("AppCore"),
                .dynamic("AppRenderer")
            ]
        ),
        // This CLI doesn't need the renderer, so rendering is split out into
        // a separate target from `AppCore` (`AppRenderer`).
        .executableTarget(
            name: "CLI",
            dependencies: ["AppCore"]
        ),
        // In this example it's possible to infer that `AppCore` must be dynamically
        // linked to `AppRenderer` when building `App` (or building the `AppRenderer`
        // product as a dylib), but in more complicated cases I would likely have to
        // resort to using `.dynamic("AppCore")`, which would enforce dynamic linking
        // even when not necessary (not ideal).
        .target(
            name: "AppRenderer",
            dependences: ["AppCore"]
        ),
        .target(name: "AppCore")
    ]
)

// A plugin's Package.swift
let package = Package(
    name: "Plugin",
    products: [
        // In this case it makes sense to enforce dynamic linking since the plugin
        // is only ever built to be loaded by the app, and is as such always compiled
        // as a dylib. But perhaps some SwiftPM command line flags like so could be
        // useful in other use cases (the existing swiftc -emit-library flag seems
        // unreliable with SwiftPM from some initial Googling);
        // $ swift build --product Plugin --linking-mode dynamic
        .library(name: "Plugin", .dynamic, targets: ["Plugin"])
    ],
    dependencies: [
        .package(path: "../App")
    ],
    targets: [
        // Targets can specify their own linking modes for dependencies, as long as the
        // products they depend on don't explicitly specify their own linking mode.
        .target(
            name: "Plugin",
            dependencies: [
                .product(name: "AppCore", package: "App", .dynamic),
                .product(name: "AppRenderer", package: "App", .dynamic)
            ]
        )
    ]
)

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.

None yet

2 participants