Reviews are an important part of the Swift evolution process. All reviews should be made in this thread on the Swift forums or, if you would like to keep your feedback private, directly in email to me as the review manager.
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to the Swift Package Manager?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages, libraries, or package managers with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
+1 but why is the swift version only limited to .x releases? Why not implement a complete swift version struct that includes major, minor, patch and alpha releases? This is something that could even live in the standard library as a SematicVersion.
Is the problem being addressed significant enough to warrant a change to the Swift Package Manager?
Yes but I think it should be expanded.
Does this proposal fit well with the feel and direction of Swift?
yes.
If you have used other languages, libraries, or package managers with a similar feature, how do you feel that this proposal compares to those?
n/a
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
read proposal and did some other reading around this feature.
We're limiting the type to what is needed for the Swift language version which is not affected by patch or pre-releases. In addition to that, the Swift language version is actually not conforming to semantic versioning, e.g. "4.2" would not be valid.
I'm generally supportive of this change. One thing I am concerned about is that some of what is stated in the Motivation of the proposal is slightly inaccurate, and thus may impact the technical design here:
The compiler flag has been since enhanced to accept two components (for e.g., "4.2").
This is technically not true. Currently the compiler supports the following invocations for -swift-version:
-swift-version 3
-swift-version 4
-swift-version 4.2
-swift-version 5 (in anticipation of Swift 5)
The compiler does not support any arbitrary version, and will bail out with an error when passed a different value to -swift-version.
For example, the compiler does not support:
-swift-version 2 (predates the language compatibility mode in the compiler)
-swift-version 3.2
-swift-version 4.1
etc
In other words, there are a list of Swift versions used for gating language modes or other source compatibility changes, and only those versions are respected by the compiler when selecting a compatibility mode. This is intentional, although could be revisited.
The design in the proposal somewhat implies that any combination of major and minor release are valid values, and they are not. This almost feels like this should be an enum, with valid cases that are added over time — not just a freeform struct. That enum could also conform to ExpressibleByIntegerLiteral for those using the array literal syntax, but I wonder if we need to add ExpressibleByStringLiteral conformance if we want to constrain the set of valid versions.
An enumeration would also require an update to the API for every Swift version whereas we can possibly not touch this API again for some time with the freeform version.
Perhaps. The Swift language mode seems like a fairly first class concept on its own for the package manager, and a concept that stands on its own from just the consideration of "build settings".
True, it has a low maintenance burden on SwiftPM. That said, this would get updated infrequently — perhaps a couple commits every year. That of course makes it easy to forget to do, but updating APIs when new information needs to be incorporated is part of the business of vending APIs, and provides semantic clarity to the user on what values are accepted and what are not.
My concerns are twofold:
The vendor of the package can tell that they picked a valid Swift version.
The vendor of the package can know what versions are valid to choose from.
With respect to #1, the only way for the vendor of the package to know if they picked a valid Swift version is if they attempt to build the package. Then they will get a compiler error when they pick an invalid Swift version. #2 could also be solved this way as the compiler also emits the list of valid arguments for the -swift-version compiler flag.
This is a functional solution: package vendors can tell if their package uses a valid Swift version and if not, what are the valid options. The experience is a bit mediocre. That said, it probably is not a big deal in practice. The change to SwiftPM is also clean and simple.
The reason I suggested an enum is because from the API the package vendor can tell what are the valid Swift versions to use. It's fully documented in the definition of the SwiftVersion type.
Out of curiosity, can you elaborate a bit more on how that is specifically a concern? Under what circumstances will an enum not work for this scenario?
enum wouldn't work if the compiler has a new language version that SwiftPM doesn't know about. However, we can solve that problem with a case that looks like: custom(value: String).
I am generally in favor of enums and that is consistent with C/C++ language version settings. One question would be how to spell them. Straw man:
enum SwiftVersion {
case v3
case v4
case v4_2
case v5 // This will probably not be present in SwiftPM 4.2
case custom(value: String)
}
If we do use an enum, I wonder if we should make 5 an explicit case, yet. I understand that the Swift compiler already accepts it, but I am assuming not all potential breaking changes are known, yet. It would be unfortunate to allow publishing a package which can potentially break in the future. The freeform API has of course the same problem, but an explicit enum case would be more suggestive to package authors.
Our original support for declaring a Swift language version gave package authors the ability to have dual-language-mode packages. This allows you to have a package that can adopt newer Swift features will still being usable by clients on the older version of the tools. In many cases your package may have to conditionalize some code on the Swift language version the code is being compiled with in order to support being used with either language version, but I expect that many package authors are willing to do that to avoid leaving older clients behind.
If we switch to using a defined enum for each language version, that will break this feature, since a package will not be able to be compatible with older tools and also know about the newer enum value. But Ankit’s suggestion of defining enums as we know about them, and leaving a “custom” option for this use case does seem like a reasonable compromise to me. (Though we might want to workshop the name “custom”).
I’ll note that having precise compiler-enforced semantics is not as important in Package.swift as it is in your actual Swift code; in your actual Swift code, anything not caught by the compiler won’t be detected until runtime, while in Package.swift the “compile” pass is always immediately followed by running the code, so we can generate runtime issues for anything the compiler didn’t catch and present those runtime issues in every place you would have seem compile-time issues. That said, it is still important for other reasons to have a clear API.
I think that’s true of the minimum Swift tools version property, and for many packages the minimum tools version will equal the language version they use, so they won’t need to use an explicit language version property at all. But beyond that, I think the language version should be a build setting. In addition to wanting to be very careful about how many properties we clutter the top level of the initializer / package interface with, our eventual build setting model should be more expressive – conditional settings, ability to contextually override values, etc. Also, right now this is a top-level package property and can’t be customized per-target, while settings should be expressible on a per-target level when needed.
Exactly. For example, if you want a package that supports the Swift 4.2 tools but can also take advantage of Swift 5 language features when using the Swift 5 tools, you'd declare your supported versions as v4_2 and custom("5"), since the 4.2 tools don't define an enum value for Swift 5 (or any other possible future language version).
If you later bump your minimum tools version to 5, you could switch to using the v5 enum value now available to you (or just remove this setting value completely and let it default to Swift 5, if your language version is the same as your tools version).
The review of SE-0209 “Package Manager Swift Language Version API Update" ran from April 10…17. Feedback was positive, and the proposal is accepted with revisions. Thanks to everyone who participated!