SE-0236: Package Manager Platform Deployment Settings

packagemanager

(Boris Buegling) #1

Hello Swift community,

The review of SE-0236 “Package Manager Platform Deployment Settings" begins now and runs through November 26, 2018.

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?

More information about the Swift evolution process is available on the Swift Evolution website.

As always, thank you for participating in Swift Evolution.

Boris Buegling
Review Manager


Draft Proposal: Target Specific Build Settings
Use more recent default target for MacOS?
Destination and profile settings in SPM generated Xcode project
[Accepted with Modifications] SE-0236: Package Manager Platform Deployment Settings
(Max Howell) #2

What is your evaluation of the proposal?

I think the approach here is very sensible.

However, I think the use of .all will confuse people, it doesn’t suggest that all-other platforms are supported. I haven’t a good solution that isn't an inline closure that you can then add a switch statement to, here a default: would be very clear IMO. But I know you guys don’t want to do such things in the manifest format.

As the proposal states, .all is the same as the * in eg. @available(iOS 10, *), but in my experience this is also not well understood. The asterisk implies everything including platforms already specified to not be supported, and that is much like .all does here.

Maybe .all can be .else(Bool). Where you can specify if you support everything else… or not. Although this would imply you have to specify .else(false) while with the proposal as it stands not specifying .all would be enough. Maybe it's not so bad to require else(Bool) to be specified though. It's explicit.

I understand though that consistency with Swift’s * may be the priority and that's fine IMO.

Is the problem being addressed significant enough to warrant a change to the Swift Package Manager?

Yes, this is required for a package graph to be resolved that will actually compile.

Does this proposal fit well with the feel and direction of Swift?

It fits well with the Package.swift format.

If you have used other languages, libraries, or package managers with a similar feature, how do you feel that this proposal compares to those?

CocoaPods supports this and it works well, though partly this is also because CocoaPods has a lint step that verifies the platforms that are declared are actually supported. It's outside the scope here, but a linting step for SwiftPM would be useful to package authors since otherwise this kind of dependency specification is hard to test.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I read the proposal twice.


(Cory Benfield) #3

What is your evaluation of the proposal?

Positive. We would adopt this feature on swift-nio-transport-services.

Is the problem being addressed significant enough to warrant a change to the Swift Package Manager?

Yes. Right now Swift Package Manager projects cannot use new macOS/iOS system features without requiring unpleasant -Xswiftc/-Xcc directives, which are strongly discouraged by the Swift ecosystem. This feature needs to be addressed.

Does this proposal fit well with the feel and direction of Swift?

Yes, it is consistent with the availability syntax in the rest of the language.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I have read the proposal twice, and considered its impact on swift-nio-transport-services.


(Jordan Rose) #4

One exception to this rule is macOS, for which the minimum deployment target version will start from 10.10.

I'm a little unclear why macOS is special here. This matches Xcode's earliest supported deployment target for macOS, but the earliest supported deployment target for iOS in Xcode is iOS 8.


(Boris Buegling) #5

10.10 is what SwiftPM uses today as the macOS deployment target, so this exception means we will keep the current default. The minimum version supported by Xcode 10 would be 10.6.


(Jordan Rose) #6

10.10 is what SwiftPM uses today as the macOS deployment target, so this exception means we will keep the current default.

What's the iOS deployment target, though?

The minimum version supported by Xcode 10 would be 10.6.

Hm, I must have been over-extrapolating from the iOS 8 part. Swift itself will only run back to 10.9 / iOS 7 anyway.


(Boris Buegling) #7

It would be iOS 8 in Xcode 10, but it could be different depending on when we ship this. It'll always be whatever is the oldest version supported by current tools.

True, but we also support building ObjC :sauropod:


Draft Proposal: Platform Deployment Settings
(Braden Scothern) #8

What is your evaluation of the proposal?

+1

This approach makes sense, is easy to use, and is scalable. I do agree with @Max_Howell1 that the .all syntax is mildly confusing.

Is the problem being addressed significant enough to warrant a change to the Swift Package Manager?

Yes.

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?

I have used Carthage and it uses the minimum deployment target that is part of the xcodeproj. This is a very reasonable and understandable way to add this feature to the package manager.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I read the proposal several times. I've kept up with reading the pitch thread and this one.


(Adrian Kashivskyy) #9

What is your evaluation of the proposal?

+1 for the idea but I have a couple of issues with the proposed syntax.

1. I think .linux should become a static func to be consistent with Apple platform functions.

struct SupportedPlatform {
  ...
  static func linux() -> SupportedPlatform
}
platforms: [
  .macOS(.v10_14), .iOS(.v12), .linux()
]

2. In my opinion, version arguments should be optional and default to .default. This might be achieved by adding a .default static variable to platform deployment target structures:

extension SupportedPlatform.MacOSVersion {
  static let default: MacOSVersion
  ...
}
struct SupportedPlatform {
  static func macOS(_ version: MacOSVersion = .default) -> SupportedPlatform
  static func tvOS(_ version: TVOSVersion = .default) -> SupportedPlatform
  static func iOS(_ version: IOSVersion = .default) -> SupportedPlatform
  static func watchOS(_ version: WatchOSVersion = .default) -> SupportedPlatform
  static func linux() -> SupportedPlatform
}

Then, the following declaration would mean that a package supports only macOS and iOS at their default deployment targets (such syntax is not possible with the proposed API):

platforms: [
  .macOS(), .iOS()
]

3. I agree with Max that the behavior of .all is definitely confusing. When I encountered it as the last element in the array, I thought this would be an example of invalid or ambiguous syntax.

I see 2 potential improvements of this. First one is to rename .all to something more meaningful, like .rest or .other. I don't have a strong candidate, naming is hard. I just feel .all is too bad.

The second option is for .all to represent all platforms at their default deployment targets. The user would be able to either use .all, or specify platforms explicitly.

extension Array where Element == SupportedPlatform {
  static func all() -> [SupportedPlatform]
}
class Package {
  init(
    name: String,
    platforms: [SupportedPlatform] = .all(),
    ...
  )
}

By making such a change (and assuming my previous points are taken into consideration), these equivalent declarations would mean that the package supports all platforms at default deployment targets:

platforms: .all()

platforms: [
  .macOS(), .iOS(), .watchOS(), .tvOS(), .linux()
]

platforms: [
  .macOS(.default), .iOS(.default), .watchOS(.default), .tvOS(.default), .linux()
]

This declaration would mean that the target should use macOS 10.13, iOS 12 and default deployment targets on watchOS, tvOS and Linux:

platforms: [
  .macOS(.v10_13), .iOS(.v12), .watchOS(), .tvOS(), .linux()
]

And this declaration would mean that the package only supports macOS and iOS and SPM should emit an error is this package is used on any other platform:

platforms: [
  .macOS(), .iOS()
]

Is the problem being addressed significant enough to warrant a change to the Swift Package Manager?

Yes.

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?

I used CocoaPods which allows to specify deployment targets in .podspec and Carthage which reads it from .xcodeproj. I feel the approach of SPM is the best of all of them.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Read the proposal and this thread.


(Jean-Daniel) #10

If it should be optional, why not make it a real optional, instead of adding a '.default' value ?

static func macOS(_ version: MacOSVersion? = nil) -> SupportedPlatform

(Pedro José Pereira Vieito) #11

While I think this proposal is a good addition to the Package Manager I think the syntax should be revised. In particular, I do think that the SupportedPlatform enum should be shared with the Platform one that appears in the Package Manager Target Specific Build Settings draft proposal. In both cases you should be able to set not only the OS, but also the version, architecture and/or a custom target triple. And the same syntax should be given to Linux, Apple OSes and other platforms.


(Marcel Jackwerth) #12

Is the problem being addressed significant enough to warrant a change to the Swift Package Manager?

Yes.

Does this proposal fit well with the feel and direction of Swift?

Generally, yes. But if Swift's direction is to become more independent of Apple's SDK/platform release cycles, the "version registry" unnecessarily couples the PackageDescription API with (at least) yearly Apple platform version releases.

The proposal SE-0209 "Package Manager Swift Language Version API Update" added a similar registry. However, that registry is only dependent on Swift itself: a new named/known version is added to the registry if and only if a new Swift version is released - making it intuitive when a named/known version is available.

// swift-tools-version: 5.0
.swiftLanguageVersions: [.v4_2, .v5]
// swift-tools-version: 4.2
.swiftLanguageVersions: [.v4_2, .version("5.0")]

With the platform versions it is unpredictable which platform versions are defined for a given swift-tools-version:

  • Generally, which versions are included in that list? The current proposal seems to suggest only major version releases. Should minor releases be included as well?
  • What is the approach to updating that list? A snapshot of all the known platform versions at the point in time when a new Swift version is cut? Update swift-tools-version whenever Apple announces/ships a new platform version? Neither seems great.

I'd recommend to drop the known versions from the proposal. That would leave us with a very verbose syntax (.macOS(MacOSVersion("10.3"))) though which could be improved by:

  1. changing the signature to macOS(_ version: String), ...
  2. keeping the existing methods and adding overloads macOS(_ version: String), ...
  3. making MacOSVersion conform to ExpressibleByStringLiteral (SE-0209 decided against using Expressible... though)

If you have used other languages, libraries, or package managers with a similar feature, how do you feel that this proposal compares to those?

Other Package Managers tend to rely on version strings alone. The following Podspec can be parsed by older CocoaPods versions which don't know about watchOS 5.0:

s.watchos.deployment_target = "5.0"

Also, compare this to Swift itself where the following compiles perfectly fine in Swift 3 even though iOS 12.0 wasn't "known" when Swift 3 was released:

if #available(iOS 12.0, *) { /* ... */ }

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Read it twice, slept over it, read it again.


(Marcel Jackwerth) #13

While both variants allow .macOS(), a real optional would also permit .macOS(nil) which seems ambiguous and also doesn't allow getting hold of that "default" for other purposes.

That said, .default also doesn't really give much insight in what it's value is. Based on the meaning of .all in the current draft...

[...] This predefined deployment version will be the oldest deployment target version supported by the installed SDK for a given platform. [...]

... should it be .oldestSupported?


(Marcel Jackwerth) #14

I commend finding prior art and aligning with it when possible, but #available isn't a good blueprint.

While #available looks like it solves a similar problem, it solves a different, Apple ecosystem specific problem: when a new OS is added to the family of Apple OSes (where a new OS tends to intersect a lot with the API of the other OSes) #available(..., *) makes porting an app to that new OS easier.

If Apple released iDishwasherOS, the check #available(iOS 10.0, *) would not by itself support this platform - first, one would have to create a target with that new SDK. One of these scenarios would happen when trying to compile it:

  • If iDishwasherOS supports the API guarded by #available... :ok_hand:
    The "latest" API is used on iDishwasherOS automatically.
  • If iDishwasherOS doesn't support the API guarded by #available... :boom:
    You could change it to something like #available(iOS 10.0, iDishwasherOS 2.0, *) and review it again in the future.

This isn't what platforms has to solve.

Most importantly it has to deal with OSes outside of the Apple family (e.g. Linux) which will have (nearly) no intersection with Apple APIs. Also note how #available does not support Linux and the right tool for the job is #if os(Linux) instead.


(Ankit Aggarwal) #15

Thank you everyone for the feedback so far! I want to summarize a few things based on the feedback and the things I discovered while working on the implementation of this proposal.

  • It might be obvious to many people but I want to explicitly call out that SwiftPM will skip building targets that don't support the current platform SwiftPM is building for. For e.g., consider the following case:

    Root depends on A
    A depends on B
    Root supports macOS and Linux
    A supports only macOS
    B supports macOS and Linux

    In such a package graph, SwiftPM will build A and B when building for macOS and skip both of them when building for Linux.

  • It is currently unclear which deployment target values will be provided by the typed API. The list of typed deployment target values for each SDK will be based on the DEPLOYMENT_TARGET_SUGGESTED_VALUES in the corresponding SDKSettings.plist file. New values will be added once an entry appears in a new version of Xcode. It was also suggested that we drop typed values but I think it is very helpful to have them so users don't have to search the documentation in order to figure out valid values. Also, generally libraries tend to keep some older deployment target which should be already available as a typed API in their current tools version.

  • For specifying custom deployment versions, the proposal mentions that an initializer will be provided. However, that will be too verbose and we can just provide string overload instead. This would make the API .macOS("10.13") instead of .macOS(MacOSVersion("10.13")).

  • Some people have asked for ability to specify Apple platforms without having to specify a deployment target version. This does make sense for packages that want to restrict to certain Apple platforms but I think it might be better to just pick a deployment target version at that point. If we do think that it is very desirable to have a way to avoid specifying the deployment target version then I would prefer something like .oldestSupported instead of nil or .default.

  • The default value of supported platforms array should be nil and not [.all] so SwiftPM can detect if something was specified by the user. This is also consistent with other APIs in the PackageDescription library.

  • Lastly, many people have pointed out that .all is vague. It would be great if we can figure out a better spelling. Some possibilities: .supportsUnspecifiedPlatforms, .supportsUnlistedPlatforms, .useDefaultForUnspecifiedPlatforms.


(Cory Benfield) #16

Oh, this was not obvious to me, and could definitely do with being explicitly called out in the proposal. This is an extremely useful behaviour, as it allows for some amount of platform-specific dependencies in cases where we have packages that can only be built on some platforms. Previously this kind of behaviour required wrapping each file in the package in #if os(), which was pretty rough.

This isn't quite the same as platform-specific dependencies, of course, as even if you only import modules on one platform you may still need to build them on all platforms. Nonetheless, it's a solid improvement that provides some increased utility here.


(Mickaël Rémond) #17

I found the proposal while investigating if I could get compile time -Xswiftc "-target"...
I find this relevant and I find it a good solution to avoid compile time issue in project using your library.

Yes, I think the problem is significant to make some libraries using newest features directly usable. Requiring special config flag has an unpleasant effect of turning possible users of your library away.

This is important for the adoption of newest platform features. Without this information embedded in your package description, you need to rely on documentation that are too easily ignored.

if that feature becomes available, I will immediately use it in my libraries using Apple Network.framework.

Yes. It is pushing forward making the dependency more self describing and easier to use.

It was a careful read, but from the perspective of a user of that feature. I do not have deep knowledge of the Swift PM internals.

I am not convinced that we need to support an "all" value, as it can be confusing, but I could not find other issues.


(Max Howell) #18

I still think .else, sure you can specify it anywhere, but similarly you can specify eg. macOS twice, it doesn't make sense to do so, there's a requirement to the composer of the manifest to leave it in a state that is readable.

let package = Package(
    name: "Foo",
    platforms: [
       .macOS(.v10_13), .iOS(.v12), .else(.supported),
    ],
)
let package = Package(
    name: "Foo",
    platforms: [
       .macOS(.v10_13), .iOS(.v12), .else(.unsupported),
    ],
)

Which would also allow for this:

let package = Package(
    name: "Foo",
    platforms: [
       .macOS(.v10_13), .iOS(.v12), .else(.untested),
    ],
)

Which is probably a more sensible default (especially considering new platforms could show up over the years). Could have SwiftPM thus emit a warning if you try to build on an untested platform.

Finally, maybe it should be an error not to include .else. If you're being explicit, (by specifying platforms rather than leaving it nil) it has value to the wider community to insist the package-author continue to be explicit.


(Pedro José Pereira Vieito) #19

+1. Otherwise, the default should probably be interpreted as .else(.untested).


(Boris Buegling) #20

I think this sounds good in theory, but:

  • the distinction between .supported and .untested seems interesting, but there is no way for the author to actually say "supported" because platforms can be added at a later time. So really .untested is the only sensible thing to ever specify
  • personally, I think that having an array already implies that everything that is not in the array is unsupported

So with both of these, only .else(untested) is really necessary and does seem better than .all. I prefer the more explicit .useDefaultForUnspecifiedPlatforms that @Aciid proposed, though.