Revert "Generate Clang module maps during the build" (#7046)

neonichu · web-flow · commit 173355bf6030 · 2023-10-31T09:44:39.000-07:00
Reverts #6983 We're seeing errors like ``` fatal error: module map file '/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/build/buildbot_incremental/swiftdriver-macosx-x86_64/x86_64-apple-macosx/release/llbuildBasic.build/module.modulemap' not found ``` on various CI jobs and should revert this to unblock.
diff --git a/Package.swift b/Package.swift
@@ -167,10 +167,7 @@ let package = Package(
         .target(
             /** The llbuild manifest model */
             name: "LLBuildManifest",
-            dependencies: [
-                "Basics",
-                "PackageLoading",
-            ],
+            dependencies: ["Basics"],
             exclude: ["CMakeLists.txt"]
         ),
 
diff --git a/Sources/Build/BuildDescription/ClangTargetBuildDescription.swift b/Sources/Build/BuildDescription/ClangTargetBuildDescription.swift
@@ -155,9 +155,17 @@ public final class ClangTargetBuildDescription {
             if case .custom(let path) = clangTarget.moduleMapType {
                 self.moduleMap = path
             }
-            // If a generated module map is needed, set the path accordingly.
-            else if clangTarget.moduleMapType.generatedModuleMapType != nil {
-                self.moduleMap = tempsPath.appending(component: moduleMapFilename)
+            // 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.
         }
diff --git a/Sources/Build/BuildManifest/LLBuildManifestBuilder+Clang.swift b/Sources/Build/BuildManifest/LLBuildManifestBuilder+Clang.swift
@@ -36,30 +36,7 @@ extension LLBuildManifestBuilder {
             inputs.append(resourcesNode)
         }
 
-        var modulesReadyInputs = [Node]()
-
-        // If the given target needs a generated module map, set up the dependency and required task to write out the module map.
-        if let type = target.clangTarget.moduleMapType.generatedModuleMapType, let moduleMapPath = target.moduleMap {
-            modulesReadyInputs.append(.file(moduleMapPath))
-
-            self.manifest.addWriteClangModuleMapCommand(
-                targetName: target.clangTarget.name,
-                moduleName: target.clangTarget.c99name,
-                publicHeadersDir: target.clangTarget.includeDir,
-                type: type,
-                outputPath: moduleMapPath
-            )
-        }
-
-        let modulesReady = self.addPhonyCommand(
-            targetName: target.target.getLLBuildModulesReadyCmdName(config: self.buildConfig),
-            inputs: modulesReadyInputs
-        )
-        inputs.append(modulesReady)
-
         func addStaticTargetInputs(_ target: ResolvedTarget) {
-            inputs.append(.virtual(target.getLLBuildModulesReadyCmdName(config: self.buildConfig)))
-
             if case .swift(let desc)? = self.plan.targetMap[target], target.type == .library {
                 inputs.append(file: desc.moduleOutputPath)
             }
@@ -139,9 +116,14 @@ extension LLBuildManifestBuilder {
         try addBuildToolPlugins(.clang(target))
 
         // Create a phony node to represent the entire target.
-        let output = self.addPhonyCommand(
-            targetName: target.target.getLLBuildTargetName(config: self.buildConfig),
-            inputs: objectFileNodes
+        let targetName = target.target.getLLBuildTargetName(config: self.buildConfig)
+        let output: Node = .virtual(targetName)
+
+        self.manifest.addNode(output, toTarget: targetName)
+        self.manifest.addPhonyCmd(
+            name: output.name,
+            inputs: objectFileNodes,
+            outputs: [output]
         )
 
         if self.plan.graph.isInRootPackages(target.target, satisfying: self.buildEnvironment) {
diff --git a/Sources/Build/BuildManifest/LLBuildManifestBuilder+Resources.swift b/Sources/Build/BuildManifest/LLBuildManifestBuilder+Resources.swift
@@ -45,9 +45,9 @@ extension LLBuildManifestBuilder {
             outputs.append(output)
         }
 
-        return self.addPhonyCommand(
-            targetName: target.target.getLLBuildResourcesCmdName(config: self.buildConfig),
-            inputs: outputs
-        )
+        let cmdName = target.target.getLLBuildResourcesCmdName(config: self.buildConfig)
+        self.manifest.addPhonyCmd(name: cmdName, inputs: outputs, outputs: [.virtual(cmdName)])
+
+        return .virtual(cmdName)
     }
 }
diff --git a/Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift b/Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift
@@ -431,7 +431,6 @@ extension LLBuildManifestBuilder {
             case .swift(let target)?:
                 inputs.append(file: target.moduleOutputPath)
             case .clang(let target)?:
-                inputs.append(.virtual(target.target.getLLBuildModulesReadyCmdName(config: self.buildConfig)))
                 for object in try target.objects {
                     inputs.append(file: object)
                 }
@@ -488,11 +487,15 @@ extension LLBuildManifestBuilder {
     /// Adds a top-level phony command that builds the entire target.
     private func addTargetCmd(_ target: SwiftTargetBuildDescription, cmdOutputs: [Node]) {
         // Create a phony node to represent the entire target.
-        let targetOutput = self.addPhonyCommand(
-            targetName: target.target.getLLBuildTargetName(config: self.buildConfig),
-            inputs: cmdOutputs
+        let targetName = target.target.getLLBuildTargetName(config: self.buildConfig)
+        let targetOutput: Node = .virtual(targetName)
+
+        self.manifest.addNode(targetOutput, toTarget: targetName)
+        self.manifest.addPhonyCmd(
+            name: targetOutput.name,
+            inputs: cmdOutputs,
+            outputs: [targetOutput]
         )
-
         if self.plan.graph.isInRootPackages(target.target, satisfying: self.buildEnvironment) {
             if !target.isTestTarget {
                 self.addNode(targetOutput, toTarget: .main)
diff --git a/Sources/Build/BuildManifest/LLBuildManifestBuilder.swift b/Sources/Build/BuildManifest/LLBuildManifestBuilder.swift
@@ -292,10 +292,6 @@ extension ResolvedTarget {
     public func getLLBuildResourcesCmdName(config: String) -> String {
         "\(name)-\(config).module-resources"
     }
-
-    public func getLLBuildModulesReadyCmdName(config: String) -> String {
-        "\(name)-\(config).modules-ready"
-    }
 }
 
 extension ResolvedProduct {
@@ -349,18 +345,6 @@ extension LLBuildManifestBuilder {
     func destinationPath(forBinaryAt path: AbsolutePath) -> AbsolutePath {
         self.plan.buildParameters.buildPath.appending(component: path.basename)
     }
-
-    /// Adds a phony command and a corresponding virtual node as output.
-    func addPhonyCommand(targetName: String, inputs: [Node]) -> Node {
-        let output: Node = .virtual(targetName)
-        self.manifest.addNode(output, toTarget: targetName)
-        self.manifest.addPhonyCmd(
-            name: output.name,
-            inputs: inputs,
-            outputs: [output]
-        )
-        return output
-    }
 }
 
 extension Sequence where Element: Hashable {
diff --git a/Sources/LLBuildManifest/CMakeLists.txt b/Sources/LLBuildManifest/CMakeLists.txt
@@ -15,7 +15,6 @@ add_library(LLBuildManifest STATIC
   Tools.swift)
 target_link_libraries(LLBuildManifest PUBLIC
   TSCBasic
-  PackageLoading
   Basics)
 
 # NOTE(compnerd) workaround for CMake not setting up include flags yet
diff --git a/Sources/LLBuildManifest/LLBuildManifest.swift b/Sources/LLBuildManifest/LLBuildManifest.swift
@@ -12,7 +12,6 @@
 
 import Basics
 import Foundation
-import PackageLoading
 
 import class TSCBasic.Process
 
@@ -24,7 +23,6 @@ public protocol AuxiliaryFileType {
 
 public enum WriteAuxiliary {
     public static let fileTypes: [AuxiliaryFileType.Type] = [
-        ClangModuleMap.self,
         EntitlementPlist.self,
         LinkFileList.self,
         SourcesFileList.self,
@@ -56,58 +54,6 @@ public enum WriteAuxiliary {
         }
     }
 
-    public struct ClangModuleMap: AuxiliaryFileType {
-        public static let name = "modulemap"
-
-        private enum GeneratedModuleMapType: String {
-            case umbrellaDirectory
-            case umbrellaHeader
-        }
-
-        public static func computeInputs(
-            targetName: String, 
-            moduleName: String, 
-            publicHeadersDir: AbsolutePath, 
-            type: PackageLoading.GeneratedModuleMapType
-        ) -> [Node] {
-            let typeNodes: [Node]
-            switch type {
-            case .umbrellaDirectory(let path):
-                typeNodes = [.virtual(GeneratedModuleMapType.umbrellaDirectory.rawValue), .directory(path)]
-            case .umbrellaHeader(let path):
-                typeNodes = [.virtual(GeneratedModuleMapType.umbrellaHeader.rawValue), .file(path)]
-            }
-            return [.virtual(Self.name), .virtual(targetName), .virtual(moduleName), .directory(publicHeadersDir)] + typeNodes
-        }
-
-        public static func getFileContents(inputs: [Node]) throws -> String {
-            guard inputs.count == 5 else {
-                throw StringError("invalid module map generation task, inputs: \(inputs)")
-            }
-
-            let generator = ModuleMapGenerator(
-                targetName: inputs[0].extractedVirtualNodeName,
-                moduleName: inputs[1].extractedVirtualNodeName,
-                publicHeadersDir: try AbsolutePath(validating: inputs[2].name),
-                fileSystem: localFileSystem
-            )
-
-            let declaredType = inputs[3].extractedVirtualNodeName
-            let path = try AbsolutePath(validating: inputs[4].name)
-            let type: PackageLoading.GeneratedModuleMapType
-            switch declaredType {
-            case GeneratedModuleMapType.umbrellaDirectory.rawValue:
-                type = .umbrellaDirectory(path)
-            case GeneratedModuleMapType.umbrellaHeader.rawValue:
-                type = .umbrellaHeader(path)
-            default:
-                throw StringError("invalid module map type in generation task: \(declaredType)")
-            }
-
-            return try generator.generateModuleMap(type: type)
-        }
-    }
-
     public struct LinkFileList: AuxiliaryFileType {
         public static let name = "link-file-list"
 
@@ -334,24 +280,6 @@ public struct LLBuildManifest {
         commands[name] = Command(name: name, tool: tool)
     }
 
-    public mutating func addWriteClangModuleMapCommand(
-        targetName: String,
-        moduleName: String,
-        publicHeadersDir: AbsolutePath,
-        type: GeneratedModuleMapType,
-        outputPath: AbsolutePath
-    ) {
-        let inputs = WriteAuxiliary.ClangModuleMap.computeInputs(
-            targetName: targetName,
-            moduleName: moduleName,
-            publicHeadersDir: publicHeadersDir,
-            type: type
-        )
-        let tool = WriteAuxiliaryFile(inputs: inputs, outputFilePath: outputPath)
-        let name = outputPath.pathString
-        commands[name] = Command(name: name, tool: tool)
-    }
-
     public mutating func addPkgStructureCmd(
         name: String,
         inputs: [Node],
diff --git a/Sources/PackageLoading/ModuleMapGenerator.swift b/Sources/PackageLoading/ModuleMapGenerator.swift
@@ -173,8 +173,8 @@ public struct ModuleMapGenerator {
         return .umbrellaDirectory(publicHeadersDir)
     }
 
-    /// Generates a module map based of the specified type, throwing an error if anything goes wrong.
-    public func generateModuleMap(type: GeneratedModuleMapType) throws -> String {
+    /// Generates a module map based of the specified type, throwing an error if anything goes wrong.  Any diagnostics are added to the receiver's diagnostics engine.
+    public func generateModuleMap(type: GeneratedModuleMapType, at path: AbsolutePath) throws {
         var moduleMap = "module \(moduleName) {\n"
         switch type {
         case .umbrellaHeader(let hdr):
@@ -189,7 +189,16 @@ public struct ModuleMapGenerator {
 
             """
         )
-        return moduleMap
+
+        // FIXME: This doesn't belong here.
+        try fileSystem.createDirectory(path.parentDirectory, recursive: true)
+
+        // If the file exists with the identical contents, we don't need to rewrite it.
+        // Otherwise, compiler will recompile even if nothing else has changed.
+        if let contents = try? fileSystem.readFileContents(path).validDescription, contents == moduleMap {
+            return
+        }
+        try fileSystem.writeFileContents(path, string: moduleMap)
     }
 }
 
diff --git a/Tests/BuildTests/BuildPlanTests.swift b/Tests/BuildTests/BuildPlanTests.swift
@@ -4034,7 +4034,7 @@ final class BuildPlanTests: XCTestCase {
         XCTAssertMatch(contents, .contains("""
               "\(buildPath.appending(components: "Bar.build", "main.m.o").escapedPathString)":
                 tool: clang
-                inputs: ["<Bar-debug.modules-ready>","<Foo-debug.modules-ready>","\(buildPath.appending(components: "Foo.swiftmodule").escapedPathString)","\(PkgA.appending(components: "Sources", "Bar", "main.m").escapedPathString)"]
+                inputs: ["\(buildPath.appending(components: "Foo.swiftmodule").escapedPathString)","\(PkgA.appending(components: "Sources", "Bar", "main.m").escapedPathString)"]
                 outputs: ["\(buildPath.appending(components: "Bar.build", "main.m.o").escapedPathString)"]
                 description: "Compiling Bar main.m"
             """))
@@ -4108,7 +4108,7 @@ final class BuildPlanTests: XCTestCase {
         XCTAssertMatch(contents, .contains("""
                "\(buildPath.appending(components: "Bar.build", "main.m.o").escapedPathString)":
                  tool: clang
-                 inputs: ["<Bar-debug.modules-ready>","<Foo-debug.modules-ready>","\(buildPath.appending(components: "Foo.swiftmodule").escapedPathString)","\(PkgA.appending(components: "Sources", "Bar", "main.m").escapedPathString)"]
+                 inputs: ["\(buildPath.appending(components: "Foo.swiftmodule").escapedPathString)","\(PkgA.appending(components: "Sources", "Bar", "main.m").escapedPathString)"]
                  outputs: ["\(buildPath.appending(components: "Bar.build", "main.m.o").escapedPathString)"]
                  description: "Compiling Bar main.m"
              """))
@@ -4188,7 +4188,7 @@ final class BuildPlanTests: XCTestCase {
         XCTAssertMatch(contents, .contains("""
                "\(buildPath.appending(components: "Bar.build", "main.m.o").escapedPathString)":
                  tool: clang
-                 inputs: ["<Bar-debug.modules-ready>","\(buildPath.appending(components: "\(dynamicLibraryPrefix)Foo\(dynamicLibraryExtension)").escapedPathString)","\(PkgA.appending(components: "Sources", "Bar", "main.m").escapedPathString)"]
+                 inputs: ["\(buildPath.appending(components: "\(dynamicLibraryPrefix)Foo\(dynamicLibraryExtension)").escapedPathString)","\(PkgA.appending(components: "Sources", "Bar", "main.m").escapedPathString)"]
                  outputs: ["\(buildPath.appending(components: "Bar.build", "main.m.o").escapedPathString)"]
                  description: "Compiling Bar main.m"
              """))
diff --git a/Tests/PackageLoadingTests/ModuleMapGenerationTests.swift b/Tests/PackageLoadingTests/ModuleMapGenerationTests.swift
@@ -190,11 +190,10 @@ func ModuleMapTester(_ targetName: String, includeDir: String = "include", in fi
     let generatedModuleMapPath = AbsolutePath.root.appending(components: "module.modulemap")
     observability.topScope.trap {
         if let generatedModuleMapType = moduleMapType.generatedModuleMapType {
-            let contents = try moduleMapGenerator.generateModuleMap(type: generatedModuleMapType)
-            try fileSystem.writeIfChanged(path: generatedModuleMapPath, string: contents)
+            try moduleMapGenerator.generateModuleMap(type: generatedModuleMapType, at: generatedModuleMapPath)
         }
     }
-
+    
     // Invoke the closure to check the results.
     let result = ModuleMapResult(diagnostics: observability.diagnostics, path: generatedModuleMapPath, fs: fileSystem)
     body(result)

Original file line number	Diff line number	Diff line change
`@@ -45,9 +45,9 @@ extension LLBuildManifestBuilder {`
`45`	`45`	`outputs.append(output)`
`46`	`46`	`}`
`47`	`47`
`48`		`- return self.addPhonyCommand(`
`49`		`- targetName: target.target.getLLBuildResourcesCmdName(config: self.buildConfig),`
`50`		`- inputs: outputs`
`51`		`- )`
	`48`	`+ let cmdName = target.target.getLLBuildResourcesCmdName(config: self.buildConfig)`
	`49`	`+ self.manifest.addPhonyCmd(name: cmdName, inputs: outputs, outputs: [.virtual(cmdName)])`
	`50`	`+`
	`51`	`+ return .virtual(cmdName)`
`52`	`52`	`}`
`53`	`53`	`}`