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

Sources/Xcodeproj/XcodeProjectModel.swift

@@ -302,6 +302,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
@@ -336,6 +343,7 @@ public struct Xcode {
             // they are all either strings or arrays of strings, because even
             // a boolean may be a macro reference expression.
             var CLANG_CXX_LANGUAGE_STANDARD: String?
+            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

@@ -286,6 +286,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
     }
 }
@@ -359,8 +363,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
@@ -384,7 +391,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:
@@ -410,6 +417,9 @@ 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.
 /// FIXME:  This should possibly be done while constructing the property list.
@@ -642,3 +652,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

@@ -359,10 +359,6 @@ func xcodeProject(
     // We'll need a mapping of targets to the corresponding targets.
     var modulesToTargets: [ResolvedTarget: Xcode.Target] = [:]
 
-    // Mapping of targets to the path of their modulemap path, if they one.
-    // It also records if the modulemap is generated by SwiftPM.
-    var modulesToModuleMap: [ResolvedTarget: (path: AbsolutePath, isGenerated: Bool)] = [:]
-
     // Go through all the targets, creating targets and adding file references
     // to the group tree (the specific top-level group under which they are
     // added depends on whether or not the target is a test target).
@@ -412,6 +408,7 @@ func xcodeProject(
         targetSettings.common.INFOPLIST_FILE = infoPlistFilePath.relative(to: sourceRootDir).asString
 
         if target.type == .test {
+            targetSettings.common.CLANG_ENABLE_MODULES = "YES"
             targetSettings.common.EMBEDDED_CONTENT_CONTAINS_SWIFT = "YES"
             targetSettings.common.LD_RUNPATH_SEARCH_PATHS = ["$(inherited)", "@loader_path/../Frameworks", "@loader_path/Frameworks"]
         } else {
@@ -533,23 +530,35 @@ func xcodeProject(
             // add to the build settings.
             let moduleMapPath: AbsolutePath
 
-            // If the modulemap is generated (as opposed to user provided).
-            let isGenerated: Bool
             // If user provided the modulemap no need to generate.
             if fileSystem.isFile(clangTarget.moduleMapPath) {
                 moduleMapPath = clangTarget.moduleMapPath
-                isGenerated = false
+
+                includeGroup.addFileReference(path: moduleMapPath.asString, name: moduleMapPath.basename)
+                targetSettings.common.MODULEMAP_FILE = "$(SRCROOT)/" + moduleMapPath.relative(to: sourceRootDir).asString
             } else {
-                // Generate and drop the modulemap inside Xcodeproj folder.
-                let path = xcodeprojPath.appending(components: "GeneratedModuleMap", clangTarget.c99name)
-                var moduleMapGenerator = ModuleMapGenerator(for: clangTarget, fileSystem: fileSystem)
-                try moduleMapGenerator.generateModuleMap(inDir: path)
-                moduleMapPath = path.appending(component: moduleMapFilename)
-                isGenerated = true
+                let umbrellaHeaderName = clangTarget.c99name + ".h"
+                if includeGroup.subitems.contains(where: { $0.path == umbrellaHeaderName }) {
+                    // If an umbrella header exists, enable Xcode's builtin module's feature rather than generating
+                    // a custom module map. This increases the compatibility of generated Xcode projects.
+                    let headerPhase = xcodeTarget.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", clangTarget.c99name)
+                    var moduleMapGenerator = ModuleMapGenerator(for: clangTarget, fileSystem: fileSystem)
+                    try moduleMapGenerator.generateModuleMap(inDir: path)
+                    moduleMapPath = path.appending(component: moduleMapFilename)
+
+                    includeGroup.addFileReference(path: moduleMapPath.asString, name: moduleMapPath.basename)
+                    targetSettings.common.MODULEMAP_FILE = "$(SRCROOT)/" + moduleMapPath.relative(to: sourceRootDir).asString
+                }
+                targetSettings.common.CLANG_ENABLE_MODULES = "YES"
+                targetSettings.common.DEFINES_MODULE = "YES"
             }
-            includeGroup.addFileReference(path: moduleMapPath.asString, name: moduleMapPath.basename)
-            // Save this modulemap path mapped to target so we can later wire it up for its dependees.
-            modulesToModuleMap[target] = (moduleMapPath, isGenerated)
         }
     }
 
@@ -585,20 +594,6 @@ func xcodeProject(
             if dependency.type == .library {
                 _ = linkPhase.addBuildFile(fileRef: otherTarget.productReference!)
             }
-            // For swift targets, if a clang dependency has a modulemap, add it via -fmodule-map-file.
-            if let moduleMap = modulesToModuleMap[dependency], target.underlyingTarget is SwiftTarget {
-                assert(dependency.underlyingTarget is ClangTarget)
-                xcodeTarget.buildSettings.common.OTHER_SWIFT_FLAGS += [
-                    "-Xcc",
-                    "-fmodule-map-file=$(SRCROOT)/" + moduleMap.path.relative(to: sourceRootDir).asString,
-                ]
-                // Workaround for a interface generation bug. <rdar://problem/30071677>
-                if moduleMap.isGenerated {
-                    xcodeTarget.buildSettings.common.HEADER_SEARCH_PATHS += [
-                        "$(SRCROOT)/" + moduleMap.path.parentDirectory.relative(to: sourceRootDir).asString
-                    ]
-                }
-            }
         }
     }
 

Tests/XcodeprojTests/PackageGraphTests.swift

@@ -53,7 +53,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/Package.swift",
                 "Dependencies/Foo 1.0.0/foo.swift",
@@ -91,6 +90,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")
@@ -99,14 +100,17 @@ class PackageGraphTests: XCTestCase {
             result.check(target: "Sea2") { targetResult in
                 targetResult.check(productType: .framework)
                 targetResult.check(dependencies: ["Foo"])
-                XCTAssertEqual(targetResult.commonBuildSettings.MODULEMAP_FILE, nil)
+                XCTAssertNil(targetResult.commonBuildSettings.CLANG_ENABLE_MODULES)
+                XCTAssertEqual(targetResult.commonBuildSettings.DEFINES_MODULE, "NO")
+                XCTAssertEqual(targetResult.commonBuildSettings.MODULEMAP_FILE, "$(SRCROOT)/Sources/Sea2/include/module.modulemap")
                 XCTAssertEqual(targetResult.commonBuildSettings.SKIP_INSTALL, "YES")
                 XCTAssertEqual(targetResult.target.buildSettings.xcconfigFileRef?.path, "../Overrides.xcconfig")
             }
 
             result.check(target: "BarTests") { targetResult in
                 targetResult.check(productType: .unitTest)
                 targetResult.check(dependencies: ["Bar", "Foo"])
+                XCTAssertEqual(targetResult.commonBuildSettings.CLANG_ENABLE_MODULES, "YES")
                 XCTAssertEqual(targetResult.commonBuildSettings.LD_RUNPATH_SEARCH_PATHS ?? [], ["$(inherited)", "@loader_path/../Frameworks", "@loader_path/Frameworks"])
                 XCTAssertEqual(targetResult.target.buildSettings.xcconfigFileRef?.path, "../Overrides.xcconfig")
             }
@@ -164,16 +168,11 @@ 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)/Sources/Sea2/include/module.modulemap",
-                  "-Xcc", "-fmodule-map-file=$(SRCROOT)/build/xcodeproj/GeneratedModuleMap/Sea/module.modulemap",
-              ])
+              XCTAssertEqual(targetResult.target.buildSettings.common.OTHER_SWIFT_FLAGS ?? [], ["$(inherited)"])
               XCTAssertEqual(targetResult.target.buildSettings.common.HEADER_SEARCH_PATHS ?? [], [
                   "$(inherited)",
                   "$(SRCROOT)/Sources/Sea2/include",
-                  "$(SRCROOT)/Sources/Sea/include",
-                  "$(SRCROOT)/build/xcodeproj/GeneratedModuleMap/Sea"
+                  "$(SRCROOT)/Sources/Sea/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),
     ]
 }