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

Enable dynamic dispatch for package path manifest loading #8057

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

Conversation

dmhts
Copy link
Contributor

@dmhts dmhts commented Oct 20, 2024

This PR elevates the default ManifestLoaderProtocol.load(packagePath... implementation to a protocol requirement, enabling dynamic dispatch for types conforming to the protocol.

Motivation:

The Workspace class provides a convenient way to fine-tune the manifest loading process by allowing the use of custom loaders. For example, RegistryAwareManifestLoader already enables loading manifests from registries. Similarly, it would be beneficial for other Workspace clients to implement their own manifest loaders to address their specific needs (if you'd like, I could go into the details of our specific need, but I believe it might be irrelevant in the context of this PR).

However, one of the default manifest loading implementations used by Workspace (and other clients as well) is not currently reflected in the protocol requirements, making it impossible for implementers to override due to the static dispatch. To address this, I propose elevating the default implementation to a protocol requirement. This would enable dynamic dispatch for this method, providing greater flexibility for Workspace clients.

Modifications:

This PR simply copies the signature of the default method implementation in question to the protocol body, while also fixing some outdated documentation along the way. This modification introduces no interface changes, so no adaptations are required at the calling sites.

Result:

class MyCustomManifestLoader: ManifestLoaderProtocol { 

    func load(packagePath: AbsolutePath, packageIdentity: PackageIdentity...) {
        // I'll be called now since I'm already dynamically dispatched.
    }
    
    ...

}

@jakepetroules
Copy link
Contributor

if you'd like, I could go into the details of our specific need, but I believe it might be irrelevant in the context of this PR

It would definitely be interesting to hear about use cases!

@dmhts
Copy link
Contributor Author

dmhts commented Oct 20, 2024

@jakepetroules sure!

Context:

We use some of SwiftPM's data models and tools to precompile our dependency package targets as XCFrameworks, which are later consumed via binary targets from automatically generated mirrored packages. Specifically, we use the Workspace API to load a package's module graph, which we then use in the PIFBuilder to generate the corresponding PIF file for more direct communication with the underlying XCBuild system.

Additionally, we require that the produced XCFrameworks be compatible with modules built with different Swift versions. To achieve this, we attempt to compile them with module stability enabled by default (and hence library evolution). However, packages that do not support module stability must be precompiled in an unstable, compiler-specific module format. This requires using a specific toolchain (typically in the form of an Xcode bundle) for each supported Swift (and sometimes SDK) version.

Since we need to apply different toolchains to the same package, this can lead to the same package being resolved into different module graphs. This typically occurs when the package has version-specific manifests (where each manifest may define its own set of dependencies). This situation is especially common nowadays as many 3rd party packages are in the process of gradually migrating from Swift 5.x to Swift 6.0

Existing issue and use cases:

Unfortunately, the existing version-specific manifest selection mechanism doesn't allow selecting manifests compatible with older (and still supported) toolchains. As you can see from the logic, the default implementation first always tries to look for version-specific manifests that are compatible with the hardcoded SwiftPM tools version, and if it finds one, it immediately returns. I can understand why it was implemented this way, but it limits the flexibility of the current manifest selection logic in multi-toolchain scenarios.

For our use case, we need a more flexible implementation where Workspace loads version-specific manifests based on the toolchain version provided dynamically, rather than relying on the hardcoded toolchain version in the currently used SwiftPM instance. Currently, as a workaround, we have to remove all manifests incompatible with the active toolchain to prevent the wrong ones from being selected.

Additionally, the existing default implementation assumes that Package.swift must always be located in the package root. However, this is often not the case in our scenario, as generated mirrored packages (which contain only binary targets) are meant to work only with specific toolchains (i.e., certain Xcode versions). For example, Package@swift-6.0 and Package@swift-5.10 are often the only manifests we need. Currently, we have to create a dummy Package.swift (with the v3 tools-version so it's never selected) just to satisfy the existing implementation. Of course, the alternative would be to remove the version from one of the manifest names, but that would break the mirroring semantics, where we mirror the original 3rd party packages 1-to-1 (along with their versioned manifest).

I understand that our use case is highly specific, but it already works like a charm in our pipelines. It's just with the proposed solution it would be so much cleaner and more flexible.

Solutions:

The good news is that greater flexibility can be easily achieved by a simple additive change. Alternatively, the goal could also be met by modifying the existing default implementation, but that would likely involve a more invasive change, requiring more careful examination. In contrast, the proposed solution of enabling dynamic dispatch is simpler and non-disruptive.

@dmhts
Copy link
Contributor Author

dmhts commented Oct 28, 2024

@jakepetroules Sorry for the rather long write-up. My main point was that such a small change could enable a lot of flexibility without any visible downsides. Do you think there’s a chance to get it through or should I rather close the PR?

@dmhts
Copy link
Contributor Author

dmhts commented Nov 20, 2024

Just a friendly reminder about this PR whenever you have time. If you feel it doesn’t add value, I'd close it to avoid contributing to the number of dangling PRs.

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