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
Fix swift-syntax dependency issue #5534
Conversation
Generated by 🚫 Danger |
Package.swift
Outdated
@@ -24,7 +24,7 @@ let package = Package( | |||
], | |||
dependencies: [ | |||
.package(url: "https://github.com/apple/swift-argument-parser.git", .upToNextMinor(from: "1.2.1")), | |||
.package(url: "https://github.com/apple/swift-syntax.git", exact: "510.0.0"), | |||
.package(url: "https://github.com/apple/swift-syntax.git", "510.0.0"..<"511.0.0"), |
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.
We can use "509.0.0"..<"511.0.0" as the swift-syntax doc is suggesting to support a wider consumer.
But since we have already bumped the version requirement in #5479, we can just keep using "510.0.0"..<"511.0.0" here.
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.
Isn't this the same as .upToNextMajor(from: "510.0.0")
?
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.
Nope, we prefer to manually to specify the range here. Say we'd like to support multi major version, then it would be "508.0.0"..<"511.0.0".
Use a version range gives us more freedom here.
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 can read the articles I linked in the issue for more detail.
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.
The thing is that SwiftLint relies on SwiftSyntax heavily and every major release of SwiftSyntax comes with many changes that either cause deprecation warnings or even behavior changes.
I'm not inclined to analyze reported issues down to the SwiftSyntax version used. Every SwiftLint release could be bound to a major SwiftSyntax release up to the next major version to allow for some flexibility. That's why .upToNextMajor
makes the most sense to me.
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'm not inclined to analyze reported issues down to the SwiftSyntax version used. Every SwiftLint release could be bound to major SwiftSyntax release up to the next major version to allow for some flexibility. That's why .upToNextMajor makes the most sense to me.
Got your idea here. But as we usually support 2 or 3 major Swift version. I believe it makes sense we support 2 or 3 corresponding SwiftSyntax here.
https://www.pointfree.co/blog/posts/116-being-a-good-citizen-in-the-land-of-swiftsyntax
For example, imagine there is an app called DemoApp. It has 3 macro package (A, B and C) dependency. (use 5.11/511.0.0 to represent 6.0/600.0.0 below for simplicity)
Package A is a good citizen and the requirement for SwiftSyntax is "508.0.0"..<"511.0.0" which means it support 5.8, 5.9 and 5.10 release of SwiftSyntax.
Package B is "509.0.0"..<"511.0.0" which means it support 5.9 and 5.10 release of SwiftSyntax.
Package C is "508.0.0"..<"510.0.0" which means it support 5.8 and 5.9 release of SwiftSyntax.
So we'll resolve to use the latest 509.x.x version here.
But if Package A use "upToNextMajor(510.0.0)" here we then have to downgrade Package A to a lower version which use "upToNextMajor(509.0.0)".
So generally I think it would be great if Package A can pay some extra effort to be a good citizen in the land of swiftsyntax.
IMO, supporting multi version of SwiftSyntax is like supporting multi version of Swift. Unless you like to use the latest feature, most of the case the extra maintain effort would be small and trivial.
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.
The thing is that SwiftLint relies on SwiftSyntax heavily and every major release of SwiftSyntax comes with many changes that either cause deprecation warnings or even behavior changes.
I'm not inclined to analyze reported issues down to the SwiftSyntax version used. Every SwiftLint release could be bound to a major SwiftSyntax release up to the next major version to allow for some flexibility. That's why
.upToNextMajor
makes the most sense to me.
Got it. If the upstream think maintaining multi version of SwiftSyntax is an issue.
We can bump to use .upToNextMajor(from: "510.0.0")
or from: "510.0.0"
here.
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.
We can bump to use .upToNextMajor(from: "510.0.0") or from: "510.0.0" here.
That would be best for now. Actually, SwiftLint is not like a library providing some macros. Its features and especially its performance is closely bound to SwiftSyntax. So ideally, we could keep this dependency in a narrow range.
In the long run, the SPM plugin should move into its own repository so that it can depend on a binary release of SwiftLint and the dependency to SwiftSyntax isn't visible to people using the plugin at all. Unfortunately, providing all products in one Package.swift
isn't working well at the moment with SPM.
It's definitely valid to bring this up. Thank you for that. Yet, this needs some discussion among the maintainers first, because it's somehow opposed to the way SwiftLint currently handles this dependency. |
fcde94c
to
c90133b
Compare
Close #5533