SE-0540: Default Package Settings

Evolution Proposal Reviews
21

Repetitive setting of the same settings does seem undesirable - especially since minor changes can be hard to discern.

Two things:

First: I am wondering why the defaults definition is at the bottom?

Seems to me they contain information that would be better read before reading all the targets and their possible overrides & modifications.

Secondly, “inherited” is in Xcode settings because there are more than two levels of a known fixed hierarchy and so one needs a way to indicate inheritance from wherever they are in the hierarchy above.

IIRC, in Swift Packages there is no inheriting from parent workspaces, projects, parent packages (?), command line -D Define parameters, and so on; all settings are defined in the package.swift file. As I recall, this was a specific and intentional design goal.

As a result, to me “inherited” doesn’t make as much sense in this context – and is, in fact, confusing because it begs the question “inherited from where?” as it suggests a broader possible hierarchy than Swift Packages have, IIRC.

6 Likes
22

I don't disagree! That said, declared constants and (at least notionally) constant-evaluable expressions such as + can fit, I think, within a restricted model. Partly why I'm drawing parallels with CSS variables.

23

The Package class is extended to define a set of default settings:

public final class Package {
  // ...

  public var defaultSwiftSettings: [SwiftSetting]?
  public var defaultCSettings: [CSetting]?
  public var defaultCXXSettings: [CXXSetting]?
  public var defaultLinkerSettings: [LinkerSetting]?

  public init(
    name: String,
    defaultLocalization: LanguageTag? = nil,
    platforms: [SupportedPlatform]? = nil,
    pkgConfig: String? = nil,
    providers: [SystemPackageProvider]? = nil,
    products: [Product] = [],
    traits: Set<Trait> = [],
    dependencies: [Dependency] = [],
    targets: [Target] = [],
    swiftLanguageVersions: [SwiftVersion]? = nil,
    defaultSwiftSettings: [SwiftSetting] = [],
    cLanguageStandard: CLanguageStandard? = nil,
    defaultCSettings: [CSetting] = [],
    cxxLanguageStandard: CXXLanguageStandard? = nil,
    defaultCXXSettings: [CXXSetting] = [],
    defaultLinkerSettings: [LinkerSetting] = []
  )
}

it gets to a point.

Package needs to switch to some kind of configuration builder pattern like

let package: Package = .init(name: "swift-blah") {
    $0.products = ...
    $0.targets = ...
    $0.defaultSwiftSettings = ...
}

is it distracting from this proposal, which solves a real and persistent usability problem? yes! but fixing the memberwise init sprawl that will need to be done, and the fact that we have to add another init with four additional parameters just to do this is a strong signal that this is overdue.

8 Likes
24

This order sensitivity is a convention that might be necessary when the format is simple text (a CLI or a config file with only basic variable substitution), but it's certainly not a requirement. The Package API is a much richer representation and could adopt a more readable and user-friendly alternative.

2 Likes
25

Perhaps, but the package description API we have today is already order sensitive. This is a valid Package.swift snippet and the order of the array determines which language mode the code compiles with:

swiftSettings: [
    .swiftLanguageMode(.v6),
    .swiftLanguageMode(.v5), 
], // resolves to -language-mode 5

I don't consider this a problem that needs fixing. It's a design that I think is common to many build systems (and command line tools) precisely because it allows the developer to compose build settings from multiple sources hierarchically while retaining the ability to override specific settings that need to be customized. I'd argue that it's a design that even facilitates this proposal, since ultimately the resolved settings for a target are going to be some combination of default settings and target-specific settings and there has to be some logic for how conflicts are resolved.

That all said, I do think it would be reasonable to only allow target-specific settings to be appended the default settings. That leads to the least surprising results, and developers who need extra control for some reason have the option of composing settings in other ways.

1 Like
26

[In reply to SE-0540: Default Package Settings - #13 by cocoaphony]

The question is valid, but I did just mean it exactly how I said it. Although that might not be the optimal design, but yeah, that was the intention.

So setting_1 == .define("ENABLE_SOMETHING", .when(configuration: .release))
So the APIs will look for exact matches and won't go after "if .define was declared with no .when in defaults, and now we have .excluding .define with .when(.release), then that must mean the user does want the .define in .when(.debug)!".
Off-hand I'll just make the impl not do that, to be less surprising to users.

Note that .excluding(.define("ENABLE_SOMETHING"), .when(configuration: .release)) would be invalid.
It should instead be .excluding(.define("ENABLE_SOMETHING", .when(configuration: .release))) where .when(configuration: .release) in inside .define. This is how it currently is in other places.

Generally speaking I don't claim to have come up with the most optimal API shape. For the most part, what I'm saying is that it should be possible to just get rid of that 1 setting, if necessary.

Otherwise I personally will (and this is not a threat, just what will make sense to me) be back to using top-level array definitions so I can express allDefaultSettingExceptOne: [SwiftSetting] then allDefaultSettings: [SwiftSetting] { allDefaultSettingExceptOne + [.thatOtherSetting] }.
Perhaps I will also use the proposed "default" arguments as well, but I'll still end up having a bunch of top-level variables so half the issue that I think this proposal is trying to solve, won't be solved for me where the packages I work on have that 1 target that can't quite accept all swift settings the others do, or such.

27

While it's a relatively small ergonomic improvement, I generally support this proposal. It's fairly common to want to share settings like .define, .headerSearchPath, strictMemorySafety, etc. across multiple targets in a package. I think there's value in allowing package authors to express this declaratively in a format that tools can more easily edit and work with.

I also think that it's important to make setting order explicit when expressing this declaratively. For options like header search paths, the relative order of settings has a meaningful impact on build behavior, and it's important that the package author controls the order of those settings. I think the proposed .inherited does a good job of indicating to the user that the order may be significant (because it can appear anywhere in the settings list) and giving them control over that ordering (even if the right thing to do most of the time is to place it at the start of the list). I don't really have a strong opinion on the specific naming.

2 Likes
28

I rarely update settings in my Package manifest, but I do support this proposal.

Once thing that is not mentioned in the proposal is the behaviour, and traceability, should the same settings be set multiple times in a target.

Should SwiftPM emit a warning info with how they are pulled in? e.g: the default settings and the target setting. including line number (if possible)?

1 Like
29

It is perhaps a little easy to miss, but this actually is covered in the proposal. SwiftPM has no notion (or very little) of settings validation and pretty much defers all of this to the underlying tools.

This inheritance mechanism matches the existing behavior of the settings definition APIs. This means that duplicates and invalid combinations are permitted. These situations are handled either by later stages of package validation or by the build tools themselves. In many cases, this results in "last entry wins" semantics.

1 Like
30

With all the different Swift settings, I support this proposal as it simplifies declaring them for multiple targets that is cohesive across different packages.

From project experience, I also noticed the visual difference from a "clean" package manifest to one that defines just one more variable introduces a lot of complexity to maintain.

That said, I also feel that calling it "default" implies that the need to call inherited() is a bit counterintuitive.

1 Like
31

I'm in favor of this proposal. I have personally worked on many packages that took the post processing approach to add the default settings to all targets. IMO, we should solve this and make it more ergonomic to express. At the same time, I believe it is outside the scope of this proposal to either overhaul the current manifest APIs (init based configuration) or resolve what happens when defining the same setting multiple times. In my opinion, this proposal strikes a nice balance between solving the ergonomic need to avoid repeating settings, using post processing, or defining and passing setting arrays while also allowing inheritance to provide additional settings to specific targets. I think this solves 95% of the use-cases. There will still be cases where developers want to inherit all default settings except X that won't be solved with this and I think that's okay.

I wish the proposal would add more to the Proposed Design section. In particular, this section should show how the new APIs are used so that the Detailed Design section can focus on the raw API changes.

Naming wise I think .inherited is okay but I believe we should align with the package trait APIs that established .defaults for exactly the same pattern. When customizing the traits of a dependency developers can pass [.defaults, "NonDefaultTrait"] to inherit the default traits and add more.

2 Likes
32

Thanks all for the feedback so far, it's been great. I'm planning to revise the proposal to incorporate some of it, I'm just waiting to hear how exactly I should do that during an active review before proceeding.