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?
Yes. Swift packages run in an increasingly wide range of platforms and configurations, and currently SwiftPM is not flexible enough to accommodate this variety of environments. Swift-the-language provides a number of tools that are not actually accessible to SwiftPM packages (such as compile time defines). This is an important step towards making it possible to build a wider range of packages and projects in SwiftPM.
Yes. This is inline with the current model used by Swift Package Manager of preferring declarative settings formats.
Compared to pip and wheel, this is substantially less flexible. Wheels allow shipping completely binary dependencies based on platform three-tuple. SwiftPM will not expose the entire three-tuple, so SwiftPM remains behind here.
I am not comparing to setup.py, as setup.py allows completely arbitrary code execution, which is already not a supported mode of operation by SwiftPM.
Regardless of this proposal being less flexible than what Python has, it is a strict improvement over the status quo. I am not interested in letting the perfect be the enemy of the good, and this proposal provides a useful base line to iterate from.
I have some reservations about the escape hatch for using unsafe flags in dependencies, outlined here.
Another point we also shouldn't forget is if we add the escape hatch, we will somewhat enable an opt-in to binary-only dependencies. Personally, I think that's fine, but we should be aware that we are doing this. Similarly, through use of -B and executable targets, there will also be the possibility for custom code in the build process.
I will reiterate my point earlier that I think using the term “unsafe” in a way that does not align with its use elsewhere in the Swift project (to mean memory safety) is unwise. A term such as “raw” or “unchecked” might be worth exploring.
A nit: it’s more idiomatic to say “define X as Y” than “define X to Y” and I would recommend the feature be named accordingly—that is: define(_:as:_:).
I agree with you that the "allow-unsafe-flags" should be explicit for the dependency you allow, but the proposal has it as a global, apply-to-all flag.
The usefulness of having this flag is to indicate that "I've reviewed how this package uses unsafe flags for the target and I'm fine with it", this is a "local/explicit" determination, but the flag as proposed is a "global/allow-all" one.
How about having an 'allow-unsafe-flags' property that you can attach to a target dependency declaration in Package.swift ? The target that adds 'allow-unsafe-flags' property for a dependency will then itself become 'unsafe' as well (meaning other targets wanting to depend on this one will need to 'allow-unsafe' as well). Attaching the property implies you understand the risks of using the 'unsafe' target dependency you want to use. If you want to make it extra safe, you could require that the unsafe flags from the target dependency are listed in the dependent target as well, so that the unsafe flags that were 'vetted' are visible. That way if the dependency adds new unsafe flags then swiftpm will reject them unless the dependent target is updated as well to match.
There are many useful 'safe' flags that are not included in this proposal, this scheme would allow packages to use them in an explicit and visible manner. The global --allow-unsafe-flags-in-dependencies is risky and I suspect people will tend to avoid it.
This seems a bit cumbersome: you don't necessarily want to use the same flags in the dependent target, so you need to have a second list per dependency?
Ultimately this is the tension we end up with a proposal like SE-0238 where every "safe" option has to be whitelisted and given API. The cost to add API for an uncommonly used option seems pretty high. I agree that having an escape hatch that is scoped to a particular dependency is a good compromise. I don't think listing the flags again in the dependent target is useful though.
Another thing I want to highlight is that it's not just about allowing projects to use uncommon options, it would be good for SwiftPM to be able to see what are the commonly used unsafe options so it can make informed decisions on what flags to include as 'safe' later on; but the proposal as it stands makes it too cumbersome to use any 'unsafe' option at all, and I suspect packages will tend to stay away from them altogether.
I was briefly discussing unsafe-flags-in-dependencies with @NeoNacho and we're tending towards allowing unsafe flags in local and branch-based dependencies without any global switch. This might be a good approach for these reasons:
The intent of unsafe escape hatch is to enable experimentation and development which aligns with the goals of branch-based dependencies.
It will enable people to control the list of dependencies that can vend unsafe flags without having to specify a list as an additional option.
It lets people bypass SwiftPM's limitation and let them do their work in a way that they can't break the package graph (win-win).
This can be used as a staging ground for adding more "safe" APIs in the build settings model.
We also agreed that the unsafe-flags-in-dependencies functionality can be discussed separately and should not block the rest of the proposal. I should also point out that implementation of this functionality is blocked on SE-0226 because of technical reasons.
The review of SE-0238: Package Manager Target Specific Build Settings ran from November 27th through December 4th, 2018. The proposal is accepted with modifications. Thanks to everyone who participated!