SE-0480: Warning Control Settings for SwiftPM

(Taking my review manager hat off)

I want to provide feedback on the two topics that have been brought up so far and add a new one that I thought about when reviewing the proposal.

String constants

I personally agree with @Douglas_Gregor's feedback here that maintaining this list of statics is problematic w.r.t. keeping a single Package.swift manifest working with multiple compiler versions. I would suggest that the documentation of these new types and methods link to the relevant compiler documentation that includes the list of valid strings to start with.

Conditionalizing warning groups based on C-compilers

I think this is a great future direction for BuildSettingCondition to allow conditionalizing a setting based on the used C compiler.

Dev only settings

As far as I can see, this proposal is the first that introduces settings that only apply during development mode of a package i.e. not when it is used as a dependency of another package. The proposal currently explains this behavior in the remote target behavior section.

Instead of making this behavior magical by building this logic into how Swift PM treats this specific setting, it might make sense to add new parameters to the .target method e.g. devSwiftSettings, devCSettings, devCXXSettings. This would make it very clear that those settings are only applied during local development and it would pave the way for developers to declare more settings as development only. We already see usage of development only settings that are passed during CI e.g. in swift-nio's CI. Furthermore, it would play nicely with an often requested dev dependencies feature.

Interested to hear what others think about taking such a direction.

Proposed API

This is a minor feedback regarding the proposed API section. Right now the proposal makes it look like the new methods are top-level, e.g.:

public static func enableWarning(
    _ name: String,
    _ condition: BuildSettingCondition? = nil
) -> CSetting

However, the implementation shows that all those methods are extensions on the existing build setting types. Can we update the proposal to make this clearer?

7 Likes