-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Use libSwiftPM instead of custom model types #194
Conversation
@@ -76,7 +80,7 @@ let package = Package( | |||
name: "SwiftToolchain", | |||
dependencies: [ | |||
.product(name: "AsyncHTTPClient", package: "async-http-client"), | |||
.product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"), |
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.
This product dependency needs to be preserved, otherwise it causes linking issues on CI.
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 think it's a difference in resolution between 5.2 and 5.3. Without removing these lines it doesn't resolve on 5.3, and the 5.3 test fails for other reasons.
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.
Fair enough, shouldn't be a problem after #195 is merged, where we have to drop support for 5.2.
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 don't have access to develop on 5.2 ATM and making guess PRs until it works
is not great. I suggest we merge #195 and move on.
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.
Uh, I forgot about this bit. I can test with 5.2 on my side.
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 mean moving forward, since our dependency graph is large and there are (undocumented?) differences between the versions, plus 5.3 is stable and we have 5.4 to worry about - it's a good time to cut the chord IMO.
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.
Sure, it's the lack of output redirection with WAMR that I'm worried about. I'll try to get that sorted soon and we'll merge #195 after that.
@yonihemi would you mind if I pick this up? |
Could you merge |
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.
Today is the day! 👏
@yonihemi ready to merge 🎉 |
Resolves #120.
Notes:
main
branch (and so TSC also has to point tomain
to resolve correctly) but if @swiftwasm/carton-team thinks it might be risky we can point torelease/5.3
(TSC as well) instead. I prefer main so we don't get stuck if swiftwasm advances in some 5.4 direction and we need the newer resolution.Manifest
is the PackageDescription specified in Package.swift vsPackageModel.Package
which includes resolved dependencies. We'll use this when tackling Support resources for package dependencies #175 and Sync JavaScriptKit runtime library with Package.resolved #155.