The review of SE-0301, begins now and runs through February 15, 2021.
Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager or direct message in the Swift forums).
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries 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?
Thank you for helping improve the Swift programing language and ecosystem.
In SwiftNIO, we're generating ad-hoc packages in bash a bunch in our integration test suites a bunch and it's very painful. This will make this so much better, so definitely +1 .
I think this is a great enhancement. It fits well with SwiftPM, addresses a substantial pain point, and does so in a way that will be generally understandable and useable. +1.
If we are to draw a line between “declarative” and “non-declarative” Swift packages, we should make explicit what that entails and provide a mechanism to verify that a package is declarative. Maybe something like swift package manifest-is-declarative. This would enable projects to validate in CI that the proposed command line operations will work.
We should consider having getters for all these items. It would be nice to have symmetry, and we can later add more complex operations like extracting the pinned revision of a particular dependency.
This is feasible and in general I think some kind of package linter would be a good future extension, but I'm somewhat concerned it could be confusing to users in the short term depending on how it's presented. For example, one user may only care about adding new dependencies via the CLI, and it's possible their dependencies list is declarative, but some other part of the manifest isn't. Is a global linting operation still going to be useful to them? I don't have a good answer to this yet.
swift package dump-package includes all or most of this information. If there's anything missing, I think it's best to add it there instead of introducing new subcommands.
It should be very helpful for a tool of mine that creates playgrounds with dependencies and it would enable a feature we’ve considered for the Swift Package Index where we’d provide a copy-pasteable command to add a dependency from a package page.
What is your evaluation of the proposal?
Great! +1
Is the problem being addressed significant enough to warrant a change to Swift?
Yes
Does this proposal fit well with the feel and direction of Swift?
Yes
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
Proposal includes examples from other package managers.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Read the proposal end-to-end and this thread.
Happy to see this has made it to the review stage, very much a +1 from me.
One area I’m left slightly unsure about is around updating binary targets as they don’t seem to fit into the automatic upgrade command mentioned in Future Directions. Presumably these would essentially require running the add-target command again to pass in the latest URL and checksum. It’s probably out of scope for this initial proposal, but thought I’d mention it for the future.
Yeah, what I had in mind for an upgrade command was that it wouldn't touch anything other than dependencies. Updating targets would probably require a different command and/or API, but I think it's an interesting potential future direction. I could see it being useful for certain continuous integration workflows.
dependencies : A comma separated list of target dependency names.
swift package add-target MyExecutable --type executable --dependencies MyLibrary,ArgumentParser
For options that can take more than 1 value, would it be better to use the .upToNextOption parsing strategy? Then the user would be able to separate values by space, like in most existing commands:
swift package add-target MyExecutable --type executable --dependencies MyLibrary ArgumentParser
I took a look through the existing SwiftPM commands and couldn't find any similar options, so I guess SwiftPM doesn't have any existing precedent to follow (the compiler uses spaces for input files though, which is kinda similar). I'm open to changing it if folks think spaces would be a better delimiter.
Adding to my previous comment, another thing I just realised is that package names can have spaces in them. So it seems to me that it's more ergnomic to write a command like this:
swift package add-product MyLibrary --targets "target foo" "target bar"
than like this:
swift package add-product MyLibrary --targets "target foo,target bar"
Also, I think separating option values by space can help the user avoid typos where a space is typed after a comma out of habit.
Update 2021-02-12:
Another point in favor for space-separated array of strings: Package names can have commas in them. It can be problematic to parse them in a comma-delimited string:
# target names are "target,foo" and "target,,,bar"
swift package add-product MyLibrary --targets target,foo,target,,,bar
Unlike space-separated array of strings, you can't get around this by using " or \.
I am +1 for the proposal, but do think the multiple-value options should be space-delimited.
I agree with @wowbagger that a space-delimited list of targets and dependencies would be better. It avoids the naming edge cases mentioned by @wowbagger.
I think even more importantly, many other command line tools use space-separated values, so that will likely be more familiar to users than comma-separated values.
One other approach not mentioned is how the swift command itself deals with multiple values. It allows for using the same option flag multiple times rather than taking multiple values with a single option flag, which is another approach.
So, instead of --targets Foo,Bar it would be --target Foo --target Bar.
For me, the space-separated values are the most readable, the most similar to other command line tool usage and much more compact than the repeated option flags approach.
The example uses these commands to update the Package.swift file:
swift package add-dependency https://github.com/apple/swift-argument-parser
swift package add-target MyLibraryTests --type test --dependencies MyLibrary
swift package add-target MyExecutable --type executable --dependencies MyLibrary,ArgumentParser
swift package add-product MyLibrary --targets MyLibrary
In this example the target MyLibrary is used before it is being defined. Does referring to an
unknown dependency cause warnings in step 2 and 3 or is this silently ignored?
My preference would be a warning since this could be the result of a simple typo.
// swift-tools-version:5.3
import PackageDescription
// Description of my package
let package = Package(
name: "MyPackage",
targets: [
.target(
name: "MyLibrary", // Utilities
dependencies: []
),
]
)
On lines 2 & 3, MyLibrary refers to the existing target with that name from the original manifest, so there's no problem.
If I instead started with an empty manifest like this:
// swift-tools-version:5.3
import PackageDescription
// Description of my package
let package = Package(
name: "MyPackage"
)
Running the same commands would report errors for lines 2, 3, and 4, and the only edit applied would be the one adding swift-argument-parser to the dependencies list. That's because if, for example, we applied the edit requested in line 2, the manifest would no longer load. By only applying edits which result in a loadable manifest, we can guarantee that using the new commands on a manifest which is initially machine-editable will preserve that property.
.