[Xcodeproj] Avoid generating module.modulemap if umbrella header exists, on creating Clang Module targets

Sources/Xcodeproj/XcodeProjectModel.swift

@@ -288,6 +288,13 @@ public struct Xcode {
         init(fileRef: FileReference) {
             self.fileRef = fileRef
         }
+
+        var settings = Settings()
+
+        /// A set of file settings.
+        public struct Settings {
+            var ATTRIBUTES: [String]?
+        }
     }
 
     /// A table of build settings, which for the sake of simplicity consists
@@ -321,6 +328,7 @@ public struct Xcode {
             // Note: although some of these build settings sound like booleans,
             // they are all either strings or arrays of strings, because even
             // a boolean may be a macro reference expression.
+            var CLANG_ENABLE_MODULES: String?
             var CLANG_ENABLE_OBJC_ARC: String?
             var COMBINE_HIDPI_IMAGES: String?
             var COPY_PHASE_STRIP: String?

Sources/Xcodeproj/XcodeProjectModelSerialization.swift

@@ -268,6 +268,10 @@ extension Xcode.BuildFile: PropertyListSerializable {
         if let fileRef = fileRef {
             dict["fileRef"] = .identifier(serializer.id(of: fileRef))
         }
+        let settingsDict = settings.asPropertyList()
+        if !settingsDict.isEmpty {
+            dict["settings"] = settingsDict
+        }
         return dict
     }
 }
@@ -326,8 +330,11 @@ extension Xcode.BuildSettingsTable: PropertyListSerializable {
 }
 
 
-extension Xcode.BuildSettingsTable.BuildSettings {
-
+protocol PropertyListDictionaryConvertible {
+    func asPropertyList() -> PropertyList
+}
+
+extension PropertyListDictionaryConvertible {
     /// Returns a property list representation of the build settings, in which
     /// every struct field is represented as a dictionary entry.  Fields of
     /// type `String` are represented as `PropertyList.string` values; fields
@@ -351,7 +358,7 @@ extension Xcode.BuildSettingsTable.BuildSettings {
         let mirror = Mirror(reflecting: self)
         for child in mirror.children {
             guard let name = child.label else {
-                preconditionFailure("unnamed build settings are not supported")
+                preconditionFailure("unnamed settings are not supported")
             }
             switch child.value {
               case nil:
@@ -377,6 +384,8 @@ extension Xcode.BuildSettingsTable.BuildSettings {
     }
 }
 
+extension Xcode.BuildFile.Settings: PropertyListDictionaryConvertible {}
+extension Xcode.BuildSettingsTable.BuildSettings: PropertyListDictionaryConvertible {}
 
 /// Private helper function that combines a base property list and an overlay
 /// property list, respecting the semantics of `$(inherited)` as we go.
@@ -619,3 +628,14 @@ extension PropertyList: CustomStringConvertible {
         return generatePlistRepresentation(plist: self, indentation: Indentation())
     }
 }
+
+extension PropertyList {
+    var isEmpty: Bool {
+        switch self {
+        case let .identifier(string): return string.isEmpty
+        case let .string(string): return string.isEmpty
+        case let .array(array): return array.isEmpty
+        case let .dictionary(dictionary): return dictionary.isEmpty
+        }
+    }
+}

Sources/Xcodeproj/pbxproj().swift

@@ -357,6 +357,7 @@ func xcodeProject(
         }
 
         if module.isTest {
+            targetSettings.common.CLANG_ENABLE_MODULES = "YES"
             targetSettings.common.EMBEDDED_CONTENT_CONTAINS_SWIFT = "YES"
             targetSettings.common.LD_RUNPATH_SEARCH_PATHS = ["@loader_path/../Frameworks", "@loader_path/Frameworks"]
         }
@@ -478,17 +479,34 @@ func xcodeProject(
             if fileSystem.isFile(clangModule.moduleMapPath) {
                 moduleMapPath = clangModule.moduleMapPath
                 isGenerated = false
+
+                includeGroup.addFileReference(path: moduleMapPath.asString, name: moduleMapPath.basename)
+                // Save this modulemap path mapped to module so we can later wire it up for its dependees.
+                modulesToModuleMap[module] = (moduleMapPath, isGenerated)
             } else {
-                // Generate and drop the modulemap inside Xcodeproj folder.
-                let path = xcodeprojPath.appending(components: "GeneratedModuleMap", clangModule.c99name)
-                var moduleMapGenerator = ModuleMapGenerator(for: clangModule, fileSystem: fileSystem)
-                try moduleMapGenerator.generateModuleMap(inDir: path)
-                moduleMapPath = path.appending(component: moduleMapFilename)
-                isGenerated = true
+                let umbrellaHeaderName = clangModule.c99name + ".h"
+                if includeGroup.subitems.contains(where: { $0.path == umbrellaHeaderName }) {
+                    // if umbrellaHeader exists, we can use module.
+                    targetSettings.common.CLANG_ENABLE_MODULES = "YES"
+                    targetSettings.common.DEFINES_MODULE = "YES"
+                    let headerPhase = target.addHeadersBuildPhase()
+                    for case let header as Xcode.FileReference in includeGroup.subitems {
+                        let buildFile = headerPhase.addBuildFile(fileRef: header)
+                        buildFile.settings.ATTRIBUTES = ["Public"]
+                    }
+                } else {
+                    // Generate and drop the modulemap inside Xcodeproj folder.
+                    let path = xcodeprojPath.appending(components: "GeneratedModuleMap", clangModule.c99name)
+                    var moduleMapGenerator = ModuleMapGenerator(for: clangModule, fileSystem: fileSystem)
+                    try moduleMapGenerator.generateModuleMap(inDir: path)
+                    moduleMapPath = path.appending(component: moduleMapFilename)
+                    isGenerated = true
+
+                    includeGroup.addFileReference(path: moduleMapPath.asString, name: moduleMapPath.basename)
+                    // Save this modulemap path mapped to module so we can later wire it up for its dependees.
+                    modulesToModuleMap[module] = (moduleMapPath, isGenerated)
+                }
             }
-            includeGroup.addFileReference(path: moduleMapPath.asString, name: moduleMapPath.basename)
-            // Save this modulemap path mapped to module so we can later wire it up for its dependees.
-            modulesToModuleMap[module] = (moduleMapPath, isGenerated)
         }
     }
 

Tests/XcodeprojTests/PackageGraphTests.swift

@@ -52,7 +52,6 @@ class PackageGraphTests: XCTestCase {
                 "Sources/Bar/bar.swift",
                 "Sources/Sea/Sea.c",
                 "Sources/Sea/include/Sea.h",
-                "Sources/Sea/include/module.modulemap",
                 "Tests/BarTests/barTests.swift",
                 "Dependencies/Foo 1.0.0/foo.swift",
                 "Products/Foo.framework",
@@ -89,6 +88,8 @@ class PackageGraphTests: XCTestCase {
             result.check(target: "Sea") { targetResult in
                 targetResult.check(productType: .framework)
                 targetResult.check(dependencies: ["Foo"])
+                XCTAssertEqual(targetResult.commonBuildSettings.CLANG_ENABLE_MODULES, "YES")
+                XCTAssertEqual(targetResult.commonBuildSettings.DEFINES_MODULE, "YES")
                 XCTAssertEqual(targetResult.commonBuildSettings.MODULEMAP_FILE, nil)
                 XCTAssertEqual(targetResult.commonBuildSettings.SKIP_INSTALL, "YES")
                 XCTAssertEqual(targetResult.target.buildSettings.xcconfigFileRef?.path, "../Overrides.xcconfig")
@@ -97,6 +98,8 @@ class PackageGraphTests: XCTestCase {
             result.check(target: "Sea2") { targetResult in
                 targetResult.check(productType: .framework)
                 targetResult.check(dependencies: ["Foo"])
+                XCTAssertNil(targetResult.commonBuildSettings.CLANG_ENABLE_MODULES)
+                XCTAssertEqual(targetResult.commonBuildSettings.DEFINES_MODULE, "NO")
                 XCTAssertEqual(targetResult.commonBuildSettings.MODULEMAP_FILE, nil)
                 XCTAssertEqual(targetResult.commonBuildSettings.SKIP_INSTALL, "YES")
                 XCTAssertEqual(targetResult.target.buildSettings.xcconfigFileRef?.path, "../Overrides.xcconfig")
@@ -105,6 +108,7 @@ class PackageGraphTests: XCTestCase {
             result.check(target: "BarTests") { targetResult in
                 targetResult.check(productType: .unitTest)
                 targetResult.check(dependencies: ["Foo", "Bar"])
+                XCTAssertEqual(targetResult.commonBuildSettings.CLANG_ENABLE_MODULES, "YES")
                 XCTAssertEqual(targetResult.commonBuildSettings.LD_RUNPATH_SEARCH_PATHS ?? [], ["@loader_path/../Frameworks", "@loader_path/Frameworks"])
                 XCTAssertEqual(targetResult.target.buildSettings.xcconfigFileRef?.path, "../Overrides.xcconfig")
             }
@@ -128,15 +132,13 @@ class PackageGraphTests: XCTestCase {
       XcodeProjectTester(project) { result in
           result.check(target: "swift") { targetResult in
               XCTAssertEqual(targetResult.target.buildSettings.common.OTHER_SWIFT_FLAGS ?? [], [
-                  "$(inherited)", "-Xcc",
-                  "-fmodule-map-file=$(SRCROOT)/build/xcodeproj/GeneratedModuleMap/Sea/module.modulemap", 
+                  "$(inherited)",
                   "-Xcc", "-fmodule-map-file=$(SRCROOT)/Sources/Sea2/include/module.modulemap"
               ])
               XCTAssertEqual(targetResult.target.buildSettings.common.HEADER_SEARCH_PATHS ?? [], [
                   "$(inherited)",
                   "$(SRCROOT)/Sources/Sea/include",
-                  "$(SRCROOT)/Sources/Sea2/include",
-                  "$(SRCROOT)/build/xcodeproj/GeneratedModuleMap/Sea"
+                  "$(SRCROOT)/Sources/Sea2/include"
               ])
           }
           result.check(target: "Sea") { targetResult in

Tests/XcodeprojTests/XcodeProjectModelSerializationTests.swift

@@ -123,9 +123,47 @@ class XcodeProjectModelSerializationTests: XCTestCase {
         }
         XCTAssertEqual(otherSwiftFlags, otherSwiftFlagValues)
     }
+
+    func testBuildFileSettingsSerialization() {
+
+        // Create build file settings.
+        var buildFileSettings = Xcode.BuildFile.Settings()
+
+        let attributeValues = ["Public"]
+        buildFileSettings.ATTRIBUTES = attributeValues
+
+        // Serialize it to a property list.
+        let plist = buildFileSettings.asPropertyList()
+
+        // Assert things about plist
+        guard case let .dictionary(buildFileSettingsDict) = plist else {
+            XCTFail("build file settings plist must be a dictionary")
+            return
+        }
+
+        guard let attributesPlist = buildFileSettingsDict["ATTRIBUTES"] else {
+            XCTFail("build file settings plist must contain ATTRIBUTES")
+            return
+        }
+
+        guard case let .array(attributePlists) = attributesPlist else {
+            XCTFail("attributes plist must be an array")
+            return
+        }
+
+        let attributes = attributePlists.flatMap { attributePlist -> String? in
+            guard case let .string(attribute) = attributePlist else {
+                XCTFail("attribute plist must be a string")
+                return nil
+            }
+            return attribute
+        }
+        XCTAssertEqual(attributes, attributeValues)
+    }
 
     static var allTests = [
         ("testBasicProjectSerialization", testBasicProjectSerialization),
         ("testBuildSettingsSerialization", testBuildSettingsSerialization),
+        ("testBuildFileSettingsSerialization", testBuildFileSettingsSerialization),
     ]
 }