refactor target build settings model (#4171)

motivation: as part of auditing the code for preconditions and assets it came to light that the target build setting model can be improved to handle preconditions via the type system

changes:
* move the build settings values into the build setting kind enum as associated values (instead of floating attribute)
* adjust call sites and test

Author: tomerd

Sources/PackageLoading/ManifestJSONParser.swift

@@ -377,26 +377,53 @@ enum ManifestJSONParser {
             try Self.parseBuildSetting($0, tool: tool)
         })
     }
-
+    
     private static func parseBuildSetting(_ json: JSON, tool: TargetBuildSettingDescription.Tool) throws -> TargetBuildSettingDescription.Setting {
         let json = try json.getJSON("data")
-        let name = try TargetBuildSettingDescription.SettingName(rawValue: json.get("name"))!
+        let name = try json.get(String.self, forKey: "name")
+        let values = try json.get([String].self, forKey: "value")
         let condition = try (try? json.getJSON("condition")).flatMap(PackageConditionDescription.init(v4:))
-
-        let value = try json.get([String].self, forKey: "value")
-
+        
         // Diagnose invalid values.
-        for item in value {
+        for item in values {
             let groups = Self.invalidValueRegex.matchGroups(in: item).flatMap{ $0 }
             if !groups.isEmpty {
                 let error = "the build setting '\(name)' contains invalid component(s): \(groups.joined(separator: " "))"
                 throw ManifestParseError.runtimeManifestErrors([error])
             }
         }
-
+        
+        let kind: TargetBuildSettingDescription.Kind
+        switch name {
+        case "headerSearchPath":
+            guard let value = values.first else {
+                throw InternalError("invalid (empty) build settings value")
+            }
+            kind = .headerSearchPath(value)
+        case "define":
+            guard let value = values.first else {
+                throw InternalError("invalid (empty) build settings value")
+            }
+            kind = .define(value)
+        case "linkedLibrary":
+            guard let value = values.first else {
+                throw InternalError("invalid (empty) build settings value")
+            }
+            kind = .linkedLibrary(value)
+        case "linkedFramework":
+            guard let value = values.first else {
+                throw InternalError("invalid (empty) build settings value")
+            }
+            kind = .linkedFramework(value)
+        case "unsafeFlags":
+            kind = .unsafeFlags(values)
+        default:
+            throw InternalError("invalid build setting \(name)")
+        }
+        
         return .init(
-            tool: tool, name: name,
-            value: value,
+            tool: tool,
+            kind: kind,
             condition: condition
         )
     }

Sources/PackageLoading/PackageBuilder.swift

@@ -917,10 +917,12 @@ public final class PackageBuilder {
         // Process each setting.
         for setting in target.settings {
             let decl: BuildSettings.Declaration
+            let values: [String]
 
             // Compute appropriate declaration for the setting.
-            switch setting.name {
-            case .headerSearchPath:
+            switch setting.kind {
+            case .headerSearchPath(let value):
+                values = [value]
 
                 switch setting.tool {
                 case .c, .cxx:
@@ -930,12 +932,14 @@ public final class PackageBuilder {
                 }
 
                 // Ensure that the search path is contained within the package.
-                let subpath = try RelativePath(validating: setting.value[0])
+                let subpath = try RelativePath(validating: value)
                 guard targetRoot.appending(subpath).isDescendantOfOrEqual(to: packagePath) else {
                     throw ModuleError.invalidHeaderSearchPath(subpath.pathString)
                 }
 
-            case .define:
+            case .define(let value):
+                values = [value]
+
                 switch setting.tool {
                 case .c, .cxx:
                     decl = .GCC_PREPROCESSOR_DEFINITIONS
@@ -945,23 +949,29 @@ public final class PackageBuilder {
                     throw InternalError("unexpected tool for setting type \(setting)")
                 }
 
-            case .linkedLibrary:
+            case .linkedLibrary(let value):
+                values = [value]
+
                 switch setting.tool {
                 case .c, .cxx, .swift:
                     throw InternalError("unexpected tool for setting type \(setting)")
                 case .linker:
                     decl = .LINK_LIBRARIES
                 }
 
-            case .linkedFramework:
+            case .linkedFramework(let value):
+                values = [value]
+
                 switch setting.tool {
                 case .c, .cxx, .swift:
                     throw InternalError("unexpected tool for setting type \(setting)")
                 case .linker:
                     decl = .LINK_FRAMEWORKS
                 }
 
-            case .unsafeFlags:
+            case .unsafeFlags(let _values):
+                values = _values
+
                 switch setting.tool {
                 case .c:
                     decl = .OTHER_CFLAGS
@@ -976,7 +986,7 @@ public final class PackageBuilder {
 
             // Create an assignment for this setting.
             var assignment = BuildSettings.Assignment()
-            assignment.value = setting.value
+            assignment.values = values
             assignment.conditions = buildConditions(from: setting.condition)
 
             // Finally, add the assignment to the assignment table.

Sources/PackageModel/BuildSettings.swift

@@ -46,7 +46,7 @@ public enum BuildSettings {
     /// An individual build setting assignment.
     public struct Assignment: Codable {
         /// The assignment value.
-        public var value: [String]
+        public var values: [String]
 
         // FIXME: This should be a set but we need Equatable existential (or AnyEquatable) for that.
         /// The condition associated with this assignment.
@@ -63,7 +63,7 @@ public enum BuildSettings {
 
         public init() { 
             self._conditions = []
-            self.value = []
+            self.values = []
         }
     }
 
@@ -108,7 +108,7 @@ public enum BuildSettings {
             let values = assignments
                 .lazy
                 .filter { $0.conditions.allSatisfy { $0.satisfies(self.environment) } }
-                .flatMap { $0.value }
+                .flatMap { $0.values }
 
             return Array(values)
         }

Sources/PackageModel/Manifest/TargetBuildSettingDescription.swift

@@ -19,14 +19,14 @@ public enum TargetBuildSettingDescription {
         case linker
     }
 
-    /// The name of the build setting.
-    public enum SettingName: String, Codable, Equatable {
-        case headerSearchPath
-        case define
-        case linkedLibrary
-        case linkedFramework
-
-        case unsafeFlags
+    /// The kind of the build setting, with associate configuration
+    public enum Kind: Codable, Equatable {
+        case headerSearchPath(String)
+        case define(String)
+        case linkedLibrary(String)
+        case linkedFramework(String)
+
+        case unsafeFlags([String])
     }
 
     /// An individual build setting.
@@ -35,38 +35,19 @@ public enum TargetBuildSettingDescription {
         /// The tool associated with this setting.
         public let tool: Tool
 
-        /// The name of the setting.
-        public let name: SettingName
+        /// The kind of the setting.
+        public let kind: Kind
 
         /// The condition at which the setting should be applied.
         public let condition: PackageConditionDescription?
 
-        /// The value of the setting.
-        ///
-        /// This is kind of like an "untyped" value since the length
-        /// of the array will depend on the setting type.
-        public let value: [String]
-
         public init(
             tool: Tool,
-            name: SettingName,
-            value: [String],
-            condition: PackageConditionDescription? = nil
+            kind: Kind,
+            condition: PackageConditionDescription? = .none
         ) {
-            switch name {
-            case .headerSearchPath: fallthrough
-            case .define: fallthrough
-            case .linkedLibrary: fallthrough
-            case .linkedFramework:
-                assert(value.count == 1, "\(tool) \(name) \(value)")
-                break
-            case .unsafeFlags:
-                break
-            }
-
             self.tool = tool
-            self.name = name
-            self.value = value
+            self.kind = kind
             self.condition = condition
         }
     }

Sources/PackageModel/ManifestSourceGeneration.swift

@@ -458,17 +458,15 @@ fileprivate extension SourceCodeFragment {
     init(from setting: TargetBuildSettingDescription.Setting) {
         var params: [SourceCodeFragment] = []
 
-        switch setting.name {
-        case .headerSearchPath, .linkedLibrary, .linkedFramework:
-            assert(setting.value.count == 1)
-            params.append(SourceCodeFragment(string: setting.value[0]))
+        switch setting.kind {
+        case .headerSearchPath(let value), .linkedLibrary(let value), .linkedFramework(let value):
+            params.append(SourceCodeFragment(string: value))
             if let condition = setting.condition {
                 params.append(SourceCodeFragment(from: condition))
             }
-            self.init(enum: setting.name.rawValue, subnodes: params)
-        case .define:
-            assert(setting.value.count == 1)
-            let parts = setting.value[0].split(separator: "=", maxSplits: 1)
+            self.init(enum: setting.kind.name, subnodes: params)
+        case .define(let value):
+            let parts = value.split(separator: "=", maxSplits: 1)
             assert(parts.count == 1 || parts.count == 2)
             params.append(SourceCodeFragment(string: String(parts[0])))
             if parts.count == 2 {
@@ -477,13 +475,13 @@ fileprivate extension SourceCodeFragment {
             if let condition = setting.condition {
                 params.append(SourceCodeFragment(from: condition))
             }
-            self.init(enum: setting.name.rawValue, subnodes: params)
-        case .unsafeFlags:
-            params.append(SourceCodeFragment(strings: setting.value))
+            self.init(enum: setting.kind.name, subnodes: params)
+        case .unsafeFlags(let values):
+            params.append(SourceCodeFragment(strings: values))
             if let condition = setting.condition {
                 params.append(SourceCodeFragment(from: condition))
             }
-            self.init(enum: setting.name.rawValue, subnodes: params)
+            self.init(enum: setting.kind.name, subnodes: params)
         }
     }
 }
@@ -617,10 +615,25 @@ public struct SourceCodeFragment {
     }
 }
 
+extension TargetBuildSettingDescription.Kind {
+    fileprivate var name: String {
+        switch self {
+        case .headerSearchPath:
+            return "headerSearchPath"
+        case .define:
+            return "define"
+        case .linkedLibrary:
+            return "linkedLibrary"
+        case .linkedFramework:
+            return "linkedFramework"
+        case .unsafeFlags:
+            return "unsafeFlags"
+        }
+    }
+}
 
-fileprivate extension String {
-    
-    var quotedForPackageManifest: String {
+extension String {
+    fileprivate var quotedForPackageManifest: String {
         return "\"" + self
             .replacingOccurrences(of: "\\", with: "\\\\")
             .replacingOccurrences(of: "\"", with: "\\\"")

Sources/XCBuildSupport/PIFBuilder.swift

@@ -1440,16 +1440,16 @@ private extension BuildSettings.AssignmentTable {
                 switch declaration {
                 case .LINK_LIBRARIES:
                     setting = .OTHER_LDFLAGS
-                    value = assignment.value.map { "-l\($0)" }
+                    value = assignment.values.map { "-l\($0)" }
                 case .LINK_FRAMEWORKS:
                     setting = .OTHER_LDFLAGS
-                    value = assignment.value.flatMap { ["-framework", $0] }
+                    value = assignment.values.flatMap { ["-framework", $0] }
                 default:
                     guard let parsedSetting = PIF.BuildSettings.MultipleValueSetting(rawValue: declaration.name) else {
                         continue
                     }
                     setting = parsedSetting
-                    value = assignment.value
+                    value = assignment.values
                 }
 
                 let pifAssignment = PIFBuildSettingAssignment(

Sources/Xcodeproj/pbxproj.swift

@@ -632,7 +632,7 @@ public func xcodeProject(
                     }
                 }
                 let config = assignment.conditions.compactMap { $0 as? ConfigurationCondition }.first?.configuration
-                try appendSetting(assignment.value, forDecl: decl, to: xcodeTarget.buildSettings, config: config)
+                try appendSetting(assignment.values, forDecl: decl, to: xcodeTarget.buildSettings, config: config)
             }
         }
     }