-
-
Notifications
You must be signed in to change notification settings - Fork 39
fix: Attempt to work around PackagePlugin.Path deprecation #125
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
Conversation
/// See: https://github.com/swiftlang/swift-package-manager/blob/735ddd97fa651729921315c8e46bd790429362cb/Sources/PackagePlugin/PackageModel.swift#L184-L186/// | ||
/// The workaround defines a custom protocol that adds the missing property, and then introduces | ||
/// a new initializer that accepts the actual target protocol and attempts to downcast. | ||
protocol CompatSourceModuleTarget: SourceModuleTarget { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use an extension to define this property for older versions instead? Won't that be much simpler than current implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean an extension that adds it to SourceModuleTarget
and then does the downcast to call into the underlying property? Otherwise, I'm not sure I'm following what you're suggesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking an implementation of directoryURL that is used for older swift versions. This could live as an extension while restricted with compiler directive.
This would avoid breaking change for older versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, but wouldn't I need this in the protocol that's being used internally? The underlying types expose directoryURL
already in Swift 6, but I don't know how I could access the implementation without downcasting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulgessinger I was thinking extension with compiler directive like this:
#if swift(<6.1)
extension SourceModuleTarget {
var directoryURL: URL {
// create directory url for older versions
}
}
#endif
This way newer properties can be used providing backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, on second look, it seems like PackagePlugin.Target.directoryURL
still does not exist in PackageDescription 6.1, but directory
is deprecated.
Let's try removing the #if swift(<6.1)
guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulgessinger are you working on the build issue fixes? Do you need any help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get it to work in all swift versions. The availability in combination with the deprecation warnings are making this very difficult. Also I don't have a good way to test multiple swift versions locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get it to work in all swift versions. The availability in combination with the deprecation warnings are making this very difficult.
The approach seems to be good to me. It's more of figuring out the exact versions and availability constraints for these API.
Also I don't have a good way to test multiple swift versions locally.
May be you can use swiftly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swiftly unfortunately just segfaults for me when I run anything with a swift version installed through it.
var buildCommands = allTargets.flatMap { target in | ||
return target.sourceFiles(withSuffix: "swift").map { file in | ||
let moduleName = target.moduleName | ||
let fileName = file.path.stem | ||
let fileName = file.url.lastPathComponent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this change break compatibility with older Swift version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think so. That would be a tradeoff. Maybe it's possible to partition the implementation into different functions that are called based on the Swift version to avoid the deprecated behavior only in Swift 6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a code path that converts the Path
to a URL
in Swift <6
870af8b
to
14af844
Compare
@paulgessinger I have done these changes as part of #140. Unfortunately I could not cherry pick your changes due to some issues in GitHub Codespace. Thanks for your effort on taking a crack at this. |
Currently, the build of this package (and dependees) produces a number of warnings like:
This (to my understanding) is because the type
PackagePlugin.Path
is deprecated in favor ofFoundation.URL
, but the replacements accessor properties are not universally implemented until (I think) version 6.1.In this PR, I'm attempting to remove these warnings. In
Plugin.swift
this is relatively straightforward, but inSwiftPackageTarget.swift
it's a bit tricky:The procotol
SourceModuleTarget
does not currently include the neededvar directoryURL: URL
, even though both (and I think those are the only two types that conform to this protocol)ClangSourceModuleTarget
andSwiftSourceModuleTarget
have this property. I'm adding here a workaround that tries to downcast the incomingSourceModuleTarget
to either of these types, and then packages it in a temporary helper protocolCompatSourceModuleTarget
that exposes thedirectoryURL
property.I'm not sure if this is the best way to this, but I thought I'd open a PR to discuss it. I can't run the tests included in the repo (even on
main
they fail for me), so I don't know if the changes are fully correct, aside from compiling without warnings.Let me know what you think!