Merge pull request swiftlang#2838 from abertelrud/eng/SR-13235-libswiftpm-should-vend-modulemap-type

[SR-13235] libSwiftPM should vend a ClangTarget's module map generation information to clients

Author: abertelrud

Sources/Build/BuildPlan.swift

@@ -253,7 +253,18 @@ public final class ClangTargetBuildDescription {
 
         // Try computing modulemap path for a C library.  This also creates the file in the file system, if needed.
         if target.type == .library {
-            self.moduleMap = try computeModulemapPath()
+            // If there's a custom module map, use it as given.
+            if case .custom(let path) = clangTarget.moduleMapType {
+                self.moduleMap = path
+            }
+            // If a generated module map is needed, generate one now in our temporary directory.
+            else if let generatedModuleMapType = clangTarget.moduleMapType.generatedModuleMapType {
+                let path = tempsPath.appending(component: moduleMapFilename)
+                let moduleMapGenerator = ModuleMapGenerator(targetName: clangTarget.name, moduleName: clangTarget.c99name, publicHeadersDir: clangTarget.includeDir, fileSystem: fileSystem)
+                try moduleMapGenerator.generateModuleMap(type: generatedModuleMapType, at: path)
+                self.moduleMap = path
+            }
+            // Otherwise there is no module map, and we leave `moduleMap` unset.
         }
 
         // Do nothing if we're not generating a bundle.
@@ -399,24 +410,6 @@ public final class ClangTargetBuildDescription {
         return compilationConditions
     }
 
-
-    /// Helper function to compute the modulemap path.
-    ///
-    /// This function either returns path to user provided modulemap or tries to automatically generates it.
-    private func computeModulemapPath() throws -> AbsolutePath {
-        // If user provided the modulemap, we're done.
-        if fileSystem.isFile(clangTarget.moduleMapPath) {
-            return clangTarget.moduleMapPath
-        } else {
-            // Otherwise try to generate one.
-            var moduleMapGenerator = ModuleMapGenerator(for: clangTarget, fileSystem: fileSystem, diagnostics: diagnostics)
-            // FIXME: We should probably only warn if we're unable to generate the modulemap
-            // because the clang target is still a valid, it just can't be imported from Swift targets.
-            try moduleMapGenerator.generateModuleMap(inDir: tempsPath)
-            return tempsPath.appending(component: moduleMapFilename)
-        }
-    }
-
     /// Module cache arguments.
     private var moduleCacheArgs: [String] {
         return ["-fmodules-cache-path=\(buildParameters.moduleCache.pathString)"]

Sources/PackageLoading/ModuleMapGenerator.swift

@@ -742,6 +742,19 @@ public final class PackageBuilder {
                 buildSettings: buildSettings
             )
         } else {
+            // It's not a Swift target, so it's a Clang target (those are the only two types of source target currently supported).
+            
+            // First determine the type of module map that will be appropriate for the target based on its header layout.
+            // FIXME: We should really be checking the target type to see whether it is one that can vend headers, not just check for the existence of the public headers path.  But right now we have now way of distinguishing between, for example, a library and an executable.  The semantics here should be to only try to detect the header layout of targets that can vend public headers.
+            let moduleMapType: ModuleMapType
+            if fileSystem.exists(publicHeadersPath) {
+                let moduleMapGenerator = ModuleMapGenerator(targetName: potentialModule.name, moduleName: potentialModule.name.spm_mangledToC99ExtendedIdentifier(), publicHeadersDir: publicHeadersPath, fileSystem: fileSystem)
+                moduleMapType = moduleMapGenerator.determineModuleMapType(diagnostics: diagnostics)
+            }
+            else {
+                moduleMapType = .none
+            }
+
             return ClangTarget(
                 name: potentialModule.name,
                 bundleName: bundleName,
@@ -750,6 +763,7 @@ public final class PackageBuilder {
                 cLanguageStandard: manifest.cLanguageStandard,
                 cxxLanguageStandard: manifest.cxxLanguageStandard,
                 includeDir: publicHeadersPath,
+                moduleMapType: moduleMapType,
                 headers: headers,
                 isTest: potentialModule.isTest,
                 sources: sources,

Sources/PackageLoading/PackageBuilder.swift

@@ -356,6 +356,9 @@ public class ClangTarget: Target {
 
     /// The path to include directory.
     public let includeDir: AbsolutePath
+    
+    /// The target's module map type, which determines whether this target vends a custom module map, a generated module map, or no module map at all.
+    public let moduleMapType: ModuleMapType
 
     /// The headers present in the target.
     ///
@@ -379,6 +382,7 @@ public class ClangTarget: Target {
         cLanguageStandard: String?,
         cxxLanguageStandard: String?,
         includeDir: AbsolutePath,
+        moduleMapType: ModuleMapType,
         headers: [AbsolutePath] = [],
         isTest: Bool = false,
         sources: Sources,
@@ -392,6 +396,7 @@ public class ClangTarget: Target {
         self.cLanguageStandard = cLanguageStandard
         self.cxxLanguageStandard = cxxLanguageStandard
         self.includeDir = includeDir
+        self.moduleMapType = moduleMapType
         self.headers = headers
         super.init(
             name: name,
@@ -407,12 +412,13 @@ public class ClangTarget: Target {
     }
 
     private enum CodingKeys: String, CodingKey {
-        case includeDir, headers, isCXX, cLanguageStandard, cxxLanguageStandard
+        case includeDir, moduleMapType, headers, isCXX, cLanguageStandard, cxxLanguageStandard
     }
 
     public override func encode(to encoder: Encoder) throws {
         var container = encoder.container(keyedBy: CodingKeys.self)
         try container.encode(includeDir, forKey: .includeDir)
+        try container.encode(moduleMapType, forKey: .moduleMapType)
         try container.encode(headers, forKey: .headers)
         try container.encode(isCXX, forKey: .isCXX)
         try container.encode(cLanguageStandard, forKey: .cLanguageStandard)
@@ -423,6 +429,7 @@ public class ClangTarget: Target {
     required public init(from decoder: Decoder) throws {
         let container = try decoder.container(keyedBy: CodingKeys.self)
         self.includeDir = try container.decode(AbsolutePath.self, forKey: .includeDir)
+        self.moduleMapType = try container.decode(ModuleMapType.self, forKey: .moduleMapType)
         self.headers = try container.decode([AbsolutePath].self, forKey: .headers)
         self.isCXX = try container.decode(Bool.self, forKey: .isCXX)
         self.cLanguageStandard = try container.decodeIfPresent(String.self, forKey: .cLanguageStandard)
@@ -486,6 +493,52 @@ public class BinaryTarget: Target {
     }
 }
 
+/// A type of module map layout.  Contains all the information needed to generate or use a module map for a target that can have C-style headers.
+public enum ModuleMapType: Equatable, Codable {
+    /// No module map file.
+    case none
+    /// A custom module map file.
+    case custom(AbsolutePath)
+    /// An umbrella header included by a generated module map file.
+    case umbrellaHeader(AbsolutePath)
+    /// An umbrella directory included by a generated module map file.
+    case umbrellaDirectory(AbsolutePath)
+
+    private enum CodingKeys: String, CodingKey {
+        case none, custom, umbrellaHeader, umbrellaDirectory
+    }
+    
+    public init(from decoder: Decoder) throws {
+        let container = try decoder.container(keyedBy: CodingKeys.self)
+        if let path = try container.decodeIfPresent(AbsolutePath.self, forKey: .custom) {
+            self = .custom(path)
+        }
+        else if let path = try container.decodeIfPresent(AbsolutePath.self, forKey: .umbrellaHeader) {
+            self = .umbrellaHeader(path)
+        }
+        else if let path = try container.decodeIfPresent(AbsolutePath.self, forKey: .umbrellaDirectory) {
+            self = .umbrellaDirectory(path)
+        }
+        else {
+            self = .none
+        }
+    }
+    
+    public func encode(to encoder: Encoder) throws {
+        var container = encoder.container(keyedBy: CodingKeys.self)
+        switch self {
+        case .none:
+            break
+        case .custom(let path):
+            try container.encode(path, forKey: .custom)
+        case .umbrellaHeader(let path):
+            try container.encode(path, forKey: .umbrellaHeader)
+        case .umbrellaDirectory(let path):
+            try container.encode(path, forKey: .umbrellaDirectory)
+        }
+    }
+}
+
 extension Target: CustomStringConvertible {
     public var description: String {
         return "<\(Swift.type(of: self)): \(name)>"

Sources/PackageModel/Target.swift

@@ -153,3 +153,28 @@ extension Diagnostic.Message {
         .error("downloaded archive of binary target '\(targetName)' does not contain expected binary artifact '\(artifactName)'")
     }
 }
+
+
+extension FileSystemError: CustomStringConvertible {
+    
+    public var description: String {
+        switch self {
+        case .invalidAccess:
+            return "invalid access"
+        case .ioError:
+            return "encountered I/O error"
+        case .isDirectory:
+            return "is a directory"
+        case .noEntry:
+            return "doesn't exist in file system"
+        case .notDirectory:
+            return "is not a directory"
+        case .unsupported:
+            return "unsupported operation"
+        case .unknownOSError:
+            return "unknown system error"
+        case .alreadyExistsAtDestination:
+            return "already exists in file system"
+        }
+    }
+}

Sources/Workspace/Diagnostics.swift

@@ -588,8 +588,8 @@ public func xcodeProject(
             var isGenerated = false
 
             // If user provided the modulemap no need to generate.
-            if fileSystem.isFile(clangTarget.moduleMapPath) {
-                moduleMapPath = clangTarget.moduleMapPath
+            if case .custom(let path) = clangTarget.moduleMapType {
+                moduleMapPath = path
             } else if includeGroup.subitems.contains(where: { $0.path == clangTarget.c99name + ".h" }) {
                 // 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.
@@ -600,12 +600,13 @@ public func xcodeProject(
                 }
                 targetSettings.common.CLANG_ENABLE_MODULES = "YES"
                 targetSettings.common.DEFINES_MODULE = "YES"
-            } else {
-                // Generate and drop the modulemap inside Xcodeproj folder.
-                let path = xcodeprojPath.appending(components: "GeneratedModuleMap", clangTarget.c99name)
-                var moduleMapGenerator = ModuleMapGenerator(for: clangTarget, fileSystem: fileSystem, diagnostics: diagnostics)
-                try moduleMapGenerator.generateModuleMap(inDir: path)
-                moduleMapPath = path.appending(component: moduleMapFilename)
+            } else if let generatedModuleMapType = clangTarget.moduleMapType.generatedModuleMapType {
+                // Generate and drop the modulemap inside the .xcodeproj wrapper.
+                let path = xcodeprojPath.appending(components: "GeneratedModuleMap", clangTarget.c99name, moduleMapFilename)
+                try fileSystem.createDirectory(path.parentDirectory, recursive: true)
+                let moduleMapGenerator = ModuleMapGenerator(targetName: clangTarget.name, moduleName: clangTarget.c99name, publicHeadersDir: clangTarget.includeDir, fileSystem: fileSystem)
+                try moduleMapGenerator.generateModuleMap(type: generatedModuleMapType, at: path)
+                moduleMapPath = path
                 isGenerated = true
             }
 

Sources/Xcodeproj/pbxproj.swift

@@ -128,7 +128,7 @@ class ModuleMapGeneration: XCTestCase {
         ModuleMapTester("Foo", in: fs) { result in
             result.checkNotCreated()
             result.checkDiagnostics { result in
-                result.check(diagnostic: "target 'Foo' failed modulemap generation: umbrella header found at '/include/Foo/Foo.h', but more than one directory exists next to its parent directory: /include/Bar, /include/Foo; consider reducing them to one", behavior: .error)
+                result.check(diagnostic: "target 'Foo' has invalid header layout: umbrella header found at '/include/Foo/Foo.h', but more than one directory exists next to its parent directory: /include/Bar; consider reducing them to one", behavior: .error)
             }
         }
 
@@ -138,25 +138,32 @@ class ModuleMapGeneration: XCTestCase {
         ModuleMapTester("Foo", in: fs) { result in
             result.checkNotCreated()
             result.checkDiagnostics { result in
-                result.check(diagnostic: "target 'Foo' failed modulemap generation: umbrella header found at '/include/Foo.h', but directories exist next to it: /include/Bar; consider removing them", behavior: .error)
+                result.check(diagnostic: "target 'Foo' has invalid header layout: umbrella header found at '/include/Foo.h', but directories exist next to it: /include/Bar; consider removing them", behavior: .error)
             }
         }
     }
 }
 
-func ModuleMapTester(_ name: String, includeDir: String = "include", in fileSystem: FileSystem, _ body: (ModuleMapResult) -> Void) {
-    let includeDir = AbsolutePath.root.appending(component: includeDir)
-    let target = ClangTarget(name: name, cLanguageStandard: nil, cxxLanguageStandard: nil, includeDir: includeDir, isTest: false, sources: Sources(paths: [], root: .root))
+/// Helper function to test module map generation.  Given a target name and optionally the name of a public-headers directory, this function determines the module map type of the public-headers directory by examining the contents of a file system and invokes a given block to check the module result (including any diagnostics).
+func ModuleMapTester(_ targetName: String, includeDir: String = "include", in fileSystem: FileSystem, _ body: (ModuleMapResult) -> Void) {
+    // Create a module map generator, and determine the type of module map to use for the header directory.  This may emit diagnostics.
     let diagnostics = DiagnosticsEngine()
-    var generator = ModuleMapGenerator(for: target, fileSystem: fileSystem, diagnostics: diagnostics)
-    do {
-        try generator.generateModuleMap(inDir: .root)
-    } catch {
-        //
+    let moduleMapGenerator = ModuleMapGenerator(targetName: targetName, moduleName: targetName.spm_mangledToC99ExtendedIdentifier(), publicHeadersDir: AbsolutePath.root.appending(component: includeDir), fileSystem: fileSystem)
+    let moduleMapType = moduleMapGenerator.determineModuleMapType(diagnostics: diagnostics)
+    
+    // Generate a module map and capture any emitted diagnostics.
+    let generatedModuleMapPath = AbsolutePath.root.appending(components: "module.modulemap")
+    diagnostics.wrap {
+        if let generatedModuleMapType = moduleMapType.generatedModuleMapType {
+            try moduleMapGenerator.generateModuleMap(type: generatedModuleMapType, at: generatedModuleMapPath)
+        }
     }
-    let genPath = AbsolutePath.root.appending(components: "module.modulemap")
-    let result = ModuleMapResult(diagnostics: diagnostics, path: genPath, fs: fileSystem)
+    
+    // Invoke the closure to check the results.
+    let result = ModuleMapResult(diagnostics: diagnostics, path: generatedModuleMapPath, fs: fileSystem)
     body(result)
+    
+    // Check for any unexpected diagnostics (the ones the closure didn't check for).
     result.validateDiagnostics()
 }
 

Tests/PackageLoadingTests/ModuleMapGenerationTests.swift

@@ -211,6 +211,7 @@ class PackageBuilderTests: XCTestCase {
             package.checkModule("clib") { module in
                 module.check(c99name: "clib", type: .library)
                 module.checkSources(root: "/Sources/clib", paths: "clib.c")
+                module.check(moduleMapType: .custom(AbsolutePath("/Sources/clib/include/module.modulemap")))
             }
         }
     }
@@ -242,6 +243,7 @@ class PackageBuilderTests: XCTestCase {
             package.checkModule("clib") { module in
                 module.check(c99name: "clib", type: .library)
                 module.checkSources(root: "/Sources", paths: "clib/clib.c", "clib/clib2.c", "clib/nested/nested.c")
+                module.check(moduleMapType: .umbrellaHeader(AbsolutePath("/Sources/clib/clib.h")))
             }
         }
     }
@@ -614,12 +616,14 @@ class PackageBuilderTests: XCTestCase {
                 module.check(c99name: "Foo", type: .library)
                 module.checkSources(root: "/Sources/Foo", paths: "Foo.c")
                 module.check(includeDir: "/Sources/Foo/inc")
+                module.check(moduleMapType: .custom(AbsolutePath("/Sources/Foo/inc/module.modulemap")))
             }
 
             package.checkModule("Bar") { module in
                 module.check(c99name: "Bar", type: .library)
                 module.checkSources(root: "/Sources/Bar", paths: "Bar.c")
                 module.check(includeDir: "/Sources/Bar/include")
+                module.check(moduleMapType: .custom(AbsolutePath("/Sources/Bar/include/module.modulemap")))
             }
         }
     }
@@ -1712,6 +1716,7 @@ class PackageBuilderTests: XCTestCase {
             package.checkModule("lib") { module in
                 module.checkSources(root: "/Sources/lib", paths: "lib.c")
                 module.check(includeDir: "/Sources/lib/include")
+                module.check(moduleMapType: .umbrellaHeader(AbsolutePath("/Sources/lib/include/lib.h")))
             }
         }
     }
@@ -1736,6 +1741,7 @@ class PackageBuilderTests: XCTestCase {
             package.checkModule("lib") { module in
                 module.checkSources(root: "/Sources/lib", paths: "movie.mkv", "lib.c")
                 module.check(includeDir: "/Sources/lib/include")
+                module.check(moduleMapType: .umbrellaHeader(AbsolutePath("/Sources/lib/include/lib.h")))
             }
         }
     }
@@ -2181,6 +2187,13 @@ final class PackageBuilderTester {
             XCTAssertEqual(target.includeDir.pathString, includeDir, file: file, line: line)
         }
 
+        func check(moduleMapType: ModuleMapType, file: StaticString = #file, line: UInt = #line) {
+            guard case let target as ClangTarget = target else {
+                return XCTFail("Module map type is being checked on a non-Clang target", file: file, line: line)
+            }
+            XCTAssertEqual(target.moduleMapType, moduleMapType, file: file, line: line)
+        }
+
         func check(c99name: String? = nil, type: PackageModel.Target.Kind? = nil, file: StaticString = #file, line: UInt = #line) {
             if let c99name = c99name {
                 XCTAssertEqual(target.c99name, c99name, file: file, line: line)