Run build commands without declared outputs (#7203)

It is possible to declare build commands without outputs and the
expectation is that those would still run. Currently, that is not the
case, since the only condition that makes llbuild run build commands is
that there's a client for the output files (either compilation or
copying of resources).

This change adds a phony output to any command that has no declared
outputs and declares these phony outputs as input for any targets that
are asking for the plugin to be applied. This will lead to these
commands running unconditionally, fixing the current silent failure to
run these.

rdar://100415491

Author: neonichu

Sources/Build/BuildDescription/TargetBuildDescription.swift

@@ -13,6 +13,7 @@
 import Basics
 import struct PackageGraph.ResolvedTarget
 import struct PackageModel.Resource
+import struct PackageModel.ToolsVersion
 import struct SPMBuildCore.BuildToolPluginInvocationResult
 import struct SPMBuildCore.BuildParameters
 
@@ -105,4 +106,13 @@ public enum TargetBuildDescription {
             return clangTargetBuildDescription.buildParameters
         }
     }
+
+    var toolsVersion: ToolsVersion {
+        switch self {
+        case .swift(let swiftTargetBuildDescription):
+            return swiftTargetBuildDescription.toolsVersion
+        case .clang(let clangTargetBuildDescription):
+            return clangTargetBuildDescription.toolsVersion
+        }
+    }
 }

Sources/Build/BuildManifest/LLBuildManifestBuilder+Clang.swift

@@ -13,6 +13,7 @@
 import struct LLBuildManifest.Node
 import struct Basics.AbsolutePath
 import struct Basics.InternalError
+import class Basics.ObservabilityScope
 import struct PackageGraph.ResolvedTarget
 import PackageModel
 
@@ -89,7 +90,7 @@ extension LLBuildManifestBuilder {
             )
         }
 
-        try addBuildToolPlugins(.clang(target))
+        let additionalInputs = try addBuildToolPlugins(.clang(target))
 
         // Create a phony node to represent the entire target.
         let targetName = target.target.getLLBuildTargetName(config: target.buildParameters.buildConfig)
@@ -98,7 +99,7 @@ extension LLBuildManifestBuilder {
         self.manifest.addNode(output, toTarget: targetName)
         self.manifest.addPhonyCmd(
             name: output.name,
-            inputs: objectFileNodes,
+            inputs: objectFileNodes + additionalInputs,
             outputs: [output]
         )
 

Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift

@@ -484,14 +484,14 @@ extension LLBuildManifestBuilder {
             }
         }
 
-        try self.addBuildToolPlugins(.swift(target))
+        let additionalInputs = try self.addBuildToolPlugins(.swift(target))
 
         // Depend on any required macro product's output.
         try target.requiredMacroProducts.forEach { macro in
             try inputs.append(.virtual(macro.getLLBuildTargetName(config: target.buildParameters.buildConfig)))
         }
 
-        return inputs
+        return inputs + additionalInputs
     }
 
     /// Adds a top-level phony command that builds the entire target.

Sources/Build/BuildManifest/LLBuildManifestBuilder.swift

@@ -194,7 +194,12 @@ extension LLBuildManifestBuilder {
 // MARK: - Compilation
 
 extension LLBuildManifestBuilder {
-    func addBuildToolPlugins(_ target: TargetBuildDescription) throws {
+    func addBuildToolPlugins(_ target: TargetBuildDescription) throws -> [Node] {
+        // For any build command that doesn't declare any outputs, we need to create a phony output to ensure they will still be run by the build system.
+        var phonyOutputs = [Node]()
+        // If we have multiple commands with no output files and no display name, this serves as a way to disambiguate the virtual nodes being created.
+        var pluginNumber = 1
+
         // Add any regular build commands created by plugins for the target.
         for result in target.buildToolPluginInvocationResults {
             // Only go through the regular build commands — prebuild commands are handled separately.
@@ -213,17 +218,32 @@ extension LLBuildManifestBuilder {
                         writableDirectories: [result.pluginOutputDirectory]
                     )
                 }
+                let additionalOutputs: [Node]
+                if command.outputFiles.isEmpty {
+                    if target.toolsVersion >= .v5_11 {
+                        additionalOutputs = [.virtual("\(target.target.c99name)-\(command.configuration.displayName ?? "\(pluginNumber)")")]
+                        phonyOutputs += additionalOutputs
+                    } else {
+                        additionalOutputs = []
+                        observabilityScope.emit(warning: "Build tool command '\(displayName)' (applied to target '\(target.target.name)') does not declare any output files and therefore will not run. You may want to consider updating the given package to tools-version 5.11 (or higher) which would run such a build tool command even without declared outputs.")
+                    }
+                    pluginNumber += 1
+                } else {
+                    additionalOutputs = []
+                }
                 self.manifest.addShellCmd(
                     name: displayName + "-" + ByteString(encodingAsUTF8: uniquedName).sha256Checksum,
                     description: displayName,
                     inputs: command.inputFiles.map { .file($0) },
-                    outputs: command.outputFiles.map { .file($0) },
+                    outputs: command.outputFiles.map { .file($0) } + additionalOutputs,
                     arguments: commandLine,
                     environment: command.configuration.environment,
                     workingDirectory: command.configuration.workingDirectory?.pathString
                 )
             }
         }
+
+        return phonyOutputs
     }
 }
 

Sources/PackageModel/ToolsVersion.swift

@@ -31,6 +31,7 @@ public struct ToolsVersion: Equatable, Hashable, Codable, Sendable {
     public static let v5_8 = ToolsVersion(version: "5.8.0")
     public static let v5_9 = ToolsVersion(version: "5.9.0")
     public static let v5_10 = ToolsVersion(version: "5.10.0")
+    public static let v5_11 = ToolsVersion(version: "5.11.0")
     public static let vNext = ToolsVersion(version: "999.0.0")
 
     /// The current tools version in use.

Tests/FunctionalTests/PluginTests.swift

@@ -167,7 +167,67 @@ class PluginTests: XCTestCase {
             XCTAssert(stdout.contains("Build of product 'MyLocalTool' complete!"), "stdout:\n\(stdout)")
         }
     }
-    
+
+    func testBuildToolWithoutOutputs() throws {
+        // Only run the test if the environment in which we're running actually supports Swift concurrency (which the plugin APIs require).
+        try XCTSkipIf(!UserToolchain.default.supportsSwiftConcurrency(), "skipping because test environment doesn't support concurrency")
+
+        func createPackageUnderTest(packageDir: AbsolutePath, toolsVersion: ToolsVersion) throws {
+            let manifestFile = packageDir.appending("Package.swift")
+            try localFileSystem.createDirectory(manifestFile.parentDirectory, recursive: true)
+            try localFileSystem.writeFileContents(
+                manifestFile,
+                string: """
+                // swift-tools-version: \(toolsVersion.description)
+                import PackageDescription
+                let package = Package(name: "MyPackage",
+                    targets: [
+                        .target(name: "SomeTarget", plugins: ["Plugin"]),
+                        .plugin(name: "Plugin", capability: .buildTool),
+                    ])
+                """)
+
+            let targetSourceFile = packageDir.appending(components: "Sources", "SomeTarget", "dummy.swift")
+            try localFileSystem.createDirectory(targetSourceFile.parentDirectory, recursive: true)
+            try localFileSystem.writeFileContents(targetSourceFile, string: "")
+
+            let pluginSourceFile = packageDir.appending(components: "Plugins", "Plugin", "plugin.swift")
+            try localFileSystem.createDirectory(pluginSourceFile.parentDirectory, recursive: true)
+            try localFileSystem.writeFileContents(pluginSourceFile, string: """
+            import PackagePlugin
+            @main
+            struct Plugin: BuildToolPlugin {
+                func createBuildCommands(context: PluginContext, target: Target) async throws -> [Command] {
+                    return [
+                        .buildCommand(
+                            displayName: "empty",
+                            executable: .init("/usr/bin/touch"),
+                            arguments: [context.pluginWorkDirectory.appending("best.txt")],
+                            inputFiles: [],
+                            outputFiles: []
+                        )
+                    ]
+                }
+            }
+            """)
+        }
+
+        try testWithTemporaryDirectory { tmpPath in
+            let packageDir = tmpPath.appending(components: "MyPackage")
+            let pathOfGeneratedFile = packageDir.appending(components: [".build", "plugins", "outputs", "mypackage", "SomeTarget", "Plugin", "best.txt"])
+
+            try createPackageUnderTest(packageDir: packageDir, toolsVersion: .v5_9)
+            let (_, stderr) = try executeSwiftBuild(packageDir)
+            XCTAssertTrue(stderr.contains("warning: Build tool command 'empty' (applied to target 'SomeTarget') does not declare any output files"), "expected warning not emitted")
+            XCTAssertFalse(localFileSystem.exists(pathOfGeneratedFile), "plugin generated file unexpectedly exists at \(pathOfGeneratedFile.pathString)")
+
+            try createPackageUnderTest(packageDir: packageDir, toolsVersion: .v5_11)
+            let (_, stderr2) = try executeSwiftBuild(packageDir)
+            XCTAssertEqual("", stderr2)
+            XCTAssertTrue(localFileSystem.exists(pathOfGeneratedFile), "plugin did not run, generated file does not exist at \(pathOfGeneratedFile.pathString)")
+        }
+    }
+
     func testCommandPluginInvocation() async throws {
         try XCTSkipIf(true, "test is disabled because it isn't stable, see rdar://117870608")