From ce95ffb31a0f3e239021fff0721bf91f17e2f3d0 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Mon, 10 Jun 2024 00:10:12 -0700 Subject: [PATCH] [Build/PackageGraph] Switch `BuildSubset.{product, target}` and `ModulesGraph.{product, target}(...)` to use optional destination This is mostly an NFC change. It makes more sense to default `destination` to "undefined" when applicable because it's not always possible to know intended destination based on user input. --- Sources/Build/BuildOperation.swift | 36 +++++++++++--- .../Commands/Utilities/PluginDelegate.swift | 2 +- Sources/PackageGraph/ModulesGraph.swift | 44 +++++++++-------- .../BuildSystem/BuildSystem.swift | 10 ++-- .../SPMTestSupport/MockPackageGraphs.swift | 20 ++++++++ .../SPMTestSupport/PackageGraphTester.swift | 4 +- Tests/BuildTests/BuildOperationTests.swift | 38 +++++++++++++++ .../CrossCompilationBuildPlanTests.swift | 2 +- .../CrossCompilationPackageGraphTests.swift | 47 ++++++++++++++++++- 9 files changed, 168 insertions(+), 35 deletions(-) diff --git a/Sources/Build/BuildOperation.swift b/Sources/Build/BuildOperation.swift index 2c71c86188d..dc7ce21a185 100644 --- a/Sources/Build/BuildOperation.swift +++ b/Sources/Build/BuildOperation.swift @@ -516,7 +516,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS } /// Compute the llbuild target name using the given subset. - private func computeLLBuildTargetName(for subset: BuildSubset) throws -> String { + func computeLLBuildTargetName(for subset: BuildSubset) throws -> String { switch subset { case .allExcludingTests: return LLBuildManifestBuilder.TargetKind.main.targetName @@ -526,9 +526,15 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS // FIXME: This is super unfortunate that we might need to load the package graph. let graph = try getPackageGraph() + let buildTriple: BuildTriple? = if let destination { + destination == .host ? .tools : .destination + } else { + nil + } + let product = graph.product( for: productName, - destination: destination == .host ? .tools : .destination + destination: buildTriple ) guard let product else { @@ -556,9 +562,15 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS // FIXME: This is super unfortunate that we might need to load the package graph. let graph = try getPackageGraph() + let buildTriple: BuildTriple? = if let destination { + destination == .host ? .tools : .destination + } else { + nil + } + let target = graph.target( for: targetName, - destination: destination == .host ? .tools : .destination + destination: buildTriple ) guard let target else { @@ -669,7 +681,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS // Emit warnings about any unhandled files in authored packages. We do this after applying build tool plugins, once we know what files they handled. // rdar://113256834 This fix works for the plugins that do not have PreBuildCommands. let targetsToConsider: [ResolvedModule] - if let subset = subset, let recursiveDependencies = try + if let subset = subset, let recursiveDependencies = try subset.recursiveDependencies(for: graph, observabilityScope: observabilityScope) { targetsToConsider = recursiveDependencies } else { @@ -939,18 +951,30 @@ extension BuildSubset { case .allExcludingTests: return graph.reachableTargets.filter { $0.type != .test } case .product(let productName, let destination): + let buildTriple: BuildTriple? = if let destination { + destination == .host ? .tools : .destination + } else { + nil + } + guard let product = graph.product( for: productName, - destination: destination == .host ? .tools : .destination + destination: buildTriple ) else { observabilityScope.emit(error: "no product named '\(productName)'") return nil } return try product.recursiveTargetDependencies() case .target(let targetName, let destination): + let buildTriple: BuildTriple? = if let destination { + destination == .host ? .tools : .destination + } else { + nil + } + guard let target = graph.target( for: targetName, - destination: destination == .host ? .tools : .destination + destination: buildTriple ) else { observabilityScope.emit(error: "no target named '\(targetName)'") return nil diff --git a/Sources/Commands/Utilities/PluginDelegate.swift b/Sources/Commands/Utilities/PluginDelegate.swift index 25dc4b99853..7184667c6bd 100644 --- a/Sources/Commands/Utilities/PluginDelegate.swift +++ b/Sources/Commands/Utilities/PluginDelegate.swift @@ -385,7 +385,7 @@ final class PluginDelegate: PluginInvocationDelegate { // Find the target in the build operation's package graph; it's an error if we don't find it. let packageGraph = try buildSystem.getPackageGraph() - guard let target = packageGraph.target(for: targetName, destination: .destination) else { + guard let target = packageGraph.target(for: targetName) else { throw StringError("could not find a target named “\(targetName)”") } diff --git a/Sources/PackageGraph/ModulesGraph.swift b/Sources/PackageGraph/ModulesGraph.swift index 8d0804aa79a..3fdf2d8eda6 100644 --- a/Sources/PackageGraph/ModulesGraph.swift +++ b/Sources/PackageGraph/ModulesGraph.swift @@ -138,55 +138,59 @@ public struct ModulesGraph { package.dependencies.compactMap { self.package(for: $0) } } - public func product(for name: String, destination: BuildTriple) -> ResolvedProduct? { + /// Find a product given a name and an optional destination. If a destination is not specified + /// this method uses `.destination` and falls back to `.tools` for macros, plugins, and tests. + public func product(for name: String, destination: BuildTriple? = .none) -> ResolvedProduct? { func findProduct(name: String, destination: BuildTriple) -> ResolvedProduct? { self.allProducts.first { $0.name == name && $0.buildTriple == destination } } - if let product = findProduct(name: name, destination: destination) { - return product + if let destination { + return findProduct(name: name, destination: destination) } - // FIXME: This is a temporary workaround and needs to be handled by the callers. + if let product = findProduct(name: name, destination: .destination) { + return product + } // It's possible to request a build of a macro, a plugin, or a test via `swift build` // which won't have the right destination set because it's impossible to indicate it. // // Same happens with `--test-product` - if one of the test targets directly references // a macro then all if its targets and the product itself become `host`. - if destination == .destination { - if let toolsProduct = findProduct(name: name, destination: .tools), - toolsProduct.type == .macro || toolsProduct.type == .plugin || toolsProduct.type == .test - { - return toolsProduct - } + if let toolsProduct = findProduct(name: name, destination: .tools), + toolsProduct.type == .macro || toolsProduct.type == .plugin || toolsProduct.type == .test + { + return toolsProduct } return nil } - public func target(for name: String, destination: BuildTriple) -> ResolvedModule? { + /// Find a target given a name and an optional destination. If a destination is not specified + /// this method uses `.destination` and falls back to `.tools` for macros, plugins, and tests. + public func target(for name: String, destination: BuildTriple? = .none) -> ResolvedModule? { func findModule(name: String, destination: BuildTriple) -> ResolvedModule? { self.allTargets.first { $0.name == name && $0.buildTriple == destination } } - if let module = findModule(name: name, destination: destination) { - return module + if let destination { + return findModule(name: name, destination: destination) } - // FIXME: This is a temporary workaround and needs to be handled by the callers. + if let module = findModule(name: name, destination: .destination) { + return module + } // It's possible to request a build of a macro, a plugin or a test via `swift build` // which won't have the right destination set because it's impossible to indicate it. // // Same happens with `--test-product` - if one of the test targets directly references // a macro then all if its targets and the product itself become `host`. - if destination == .destination { - if let toolsModule = findModule(name: name, destination: .tools), - toolsModule.type == .macro || toolsModule.type == .plugin || toolsModule.type == .test - { - return toolsModule - } + if let toolsModule = findModule(name: name, destination: .tools), + toolsModule.type == .macro || toolsModule.type == .plugin || toolsModule.type == .test + { + return toolsModule } return nil diff --git a/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift b/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift index 424b4b45434..37d770a8bd3 100644 --- a/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift +++ b/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift @@ -24,11 +24,13 @@ public enum BuildSubset { /// Represents the subset of all products and targets. case allIncludingTests - /// Represents a specific product. - case product(String, for: BuildParameters.Destination = .target) + /// Represents a specific product. Allows to set a specific + /// destination if it's known. + case product(String, for: BuildParameters.Destination? = .none) - /// Represents a specific target. - case target(String, for: BuildParameters.Destination = .target) + /// Represents a specific target. Allows to set a specific + /// destination if it's known. + case target(String, for: BuildParameters.Destination? = .none) } /// A protocol that represents a build system used by SwiftPM for all build operations. This allows factoring out the diff --git a/Sources/SPMTestSupport/MockPackageGraphs.swift b/Sources/SPMTestSupport/MockPackageGraphs.swift index 1833db62c82..8fa8e67e0b2 100644 --- a/Sources/SPMTestSupport/MockPackageGraphs.swift +++ b/Sources/SPMTestSupport/MockPackageGraphs.swift @@ -127,9 +127,11 @@ package func macrosPackageGraph() throws -> MockPackageGraph { package func macrosTestsPackageGraph() throws -> MockPackageGraph { let fs = InMemoryFileSystem(emptyFiles: + "/swift-mmio/Plugins/MMIOPlugin/source.swift", "/swift-mmio/Sources/MMIO/source.swift", "/swift-mmio/Sources/MMIOMacros/source.swift", "/swift-mmio/Sources/MMIOMacrosTests/source.swift", + "/swift-mmio/Sources/MMIOMacro+PluginTests/source.swift", "/swift-syntax/Sources/SwiftSyntax/source.swift", "/swift-syntax/Sources/SwiftSyntaxMacrosTestSupport/source.swift", "/swift-syntax/Sources/SwiftSyntaxMacros/source.swift", @@ -156,6 +158,11 @@ package func macrosTestsPackageGraph() throws -> MockPackageGraph { name: "MMIO", type: .library(.automatic), targets: ["MMIO"] + ), + ProductDescription( + name: "MMIOPlugin", + type: .plugin, + targets: ["MMIOPlugin"] ) ], targets: [ @@ -171,6 +178,11 @@ package func macrosTestsPackageGraph() throws -> MockPackageGraph { ], type: .macro ), + TargetDescription( + name: "MMIOPlugin", + type: .plugin, + pluginCapability: .buildTool + ), TargetDescription( name: "MMIOMacrosTests", dependencies: [ @@ -178,6 +190,14 @@ package func macrosTestsPackageGraph() throws -> MockPackageGraph { .product(name: "SwiftSyntaxMacrosTestSupport", package: "swift-syntax") ], type: .test + ), + TargetDescription( + name: "MMIOMacro+PluginTests", + dependencies: [ + .target(name: "MMIOPlugin"), + .target(name: "MMIOMacros") + ], + type: .test ) ] ), diff --git a/Sources/SPMTestSupport/PackageGraphTester.swift b/Sources/SPMTestSupport/PackageGraphTester.swift index b5442d300b4..d524cfbdaf7 100644 --- a/Sources/SPMTestSupport/PackageGraphTester.swift +++ b/Sources/SPMTestSupport/PackageGraphTester.swift @@ -88,7 +88,7 @@ public final class PackageGraphResult { package func checkTarget( _ name: String, - destination: BuildTriple = .destination, + destination: BuildTriple? = .none, file: StaticString = #file, line: UInt = #line, body: (ResolvedTargetResult) -> Void @@ -113,7 +113,7 @@ public final class PackageGraphResult { public func checkProduct( _ name: String, - destination: BuildTriple = .destination, + destination: BuildTriple? = .none, file: StaticString = #file, line: UInt = #line, body: (ResolvedProductResult) -> Void diff --git a/Tests/BuildTests/BuildOperationTests.swift b/Tests/BuildTests/BuildOperationTests.swift index b454d8fb0d0..9a9c06e0939 100644 --- a/Tests/BuildTests/BuildOperationTests.swift +++ b/Tests/BuildTests/BuildOperationTests.swift @@ -18,6 +18,7 @@ import LLBuildManifest @_spi(DontAdoptOutsideOfSwiftPMExposedForBenchmarksAndTestsOnly) import PackageGraph import SPMBuildCore +@_spi(SwiftPMInternal) import SPMTestSupport import XCTest @@ -176,4 +177,41 @@ final class BuildOperationTests: XCTestCase { } } } + + func testHostProductsAndTargetsWithoutExplicitDestination() throws { + let mock = try macrosTestsPackageGraph() + + let op = mockBuildOperation( + productsBuildParameters: mockBuildParameters(destination: .target), + toolsBuildParameters: mockBuildParameters(destination: .host), + packageGraphLoader: { mock.graph }, + scratchDirectory: AbsolutePath("/.build/\(hostTriple)"), + fs: mock.fileSystem, + observabilityScope: mock.observabilityScope + ) + + XCTAssertEqual( + "MMIOMacros-\(hostTriple)-debug-tool.exe", + try op.computeLLBuildTargetName(for: .product("MMIOMacros")) + ) + + for target in ["MMIOMacros", "MMIOPlugin", "MMIOMacrosTests", "MMIOMacro+PluginTests"] { + XCTAssertEqual( + "\(target)-\(hostTriple)-debug-tool.module", + try op.computeLLBuildTargetName(for: .target(target)) + ) + } + + let dependencies = try BuildSubset.target("MMIOMacro+PluginTests").recursiveDependencies( + for: mock.graph, + observabilityScope: mock.observabilityScope + ) + + XCTAssertNotNil(dependencies) + XCTAssertTrue(dependencies!.count > 0) + + for dependency in dependencies! { + XCTAssertEqual(dependency.buildTriple, .tools) + } + } } diff --git a/Tests/BuildTests/CrossCompilationBuildPlanTests.swift b/Tests/BuildTests/CrossCompilationBuildPlanTests.swift index d322aca607b..1671f3044e5 100644 --- a/Tests/BuildTests/CrossCompilationBuildPlanTests.swift +++ b/Tests/BuildTests/CrossCompilationBuildPlanTests.swift @@ -300,7 +300,7 @@ final class CrossCompilationBuildPlanTests: XCTestCase { ) let result = try BuildPlanResult(plan: plan) result.checkProductsCount(2) - result.checkTargetsCount(15) + result.checkTargetsCount(16) XCTAssertTrue(try result.allTargets(named: "SwiftSyntax") .map { try $0.swiftTarget() } diff --git a/Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift b/Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift index 4c9b083aeea..501bdb19e61 100644 --- a/Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift +++ b/Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift @@ -82,6 +82,10 @@ final class CrossCompilationPackageGraphTests: XCTestCase { } } + result.checkProduct("MMIOMacros") { result in + result.check(buildTriple: .tools) + } + result.checkTargets("SwiftSyntax") { results in XCTAssertEqual(results.count, 2) @@ -99,6 +103,7 @@ final class CrossCompilationPackageGraphTests: XCTestCase { result.check( targets: "MMIO", "MMIOMacros", + "MMIOPlugin", "SwiftCompilerPlugin", "SwiftCompilerPlugin", "SwiftCompilerPluginMessageHandling", @@ -110,7 +115,7 @@ final class CrossCompilationPackageGraphTests: XCTestCase { "SwiftSyntaxMacrosTestSupport", "SwiftSyntaxMacrosTestSupport" ) - result.check(testModules: "MMIOMacrosTests") + result.check(testModules: "MMIOMacrosTests", "MMIOMacro+PluginTests") result.checkTarget("MMIO") { result in result.check(buildTriple: .destination) result.check(dependencies: "MMIOMacros") @@ -118,6 +123,7 @@ final class CrossCompilationPackageGraphTests: XCTestCase { result.checkTargets("MMIOMacros") { results in XCTAssertEqual(results.count, 1) } + result.checkTarget("MMIOMacrosTests", destination: .tools) { result in result.check(buildTriple: .tools) result.checkDependency("MMIOMacros") { result in @@ -145,6 +151,14 @@ final class CrossCompilationPackageGraphTests: XCTestCase { } } + result.checkTarget("MMIOMacros") { result in + result.check(buildTriple: .tools) + } + + result.checkTarget("MMIOMacrosTests") { result in + result.check(buildTriple: .tools) + } + result.checkTargets("SwiftSyntax") { results in XCTAssertEqual(results.count, 2) @@ -153,4 +167,35 @@ final class CrossCompilationPackageGraphTests: XCTestCase { } } } + + func testPlugins() throws { + let graph = try macrosTestsPackageGraph().graph + PackageGraphTester(graph) { result in + result.checkProduct("MMIOPlugin", destination: .tools) { result in + result.check(buildTriple: .tools) + } + + result.checkProduct("MMIOPlugin") { result in + result.check(buildTriple: .tools) + } + + result.checkTarget("MMIOPlugin", destination: .tools) { result in + result.check(buildTriple: .tools) + } + + result.checkTarget("MMIOPlugin") { result in + result.check(buildTriple: .tools) + } + + result.checkTarget("MMIOMacro+PluginTests", destination: .tools) { result in + result.check(buildTriple: .tools) + result.check(dependencies: "MMIOPlugin", "MMIOMacros") + } + + result.checkTarget("MMIOMacro+PluginTests") { result in + result.check(buildTriple: .tools) + result.check(dependencies: "MMIOPlugin", "MMIOMacros") + } + } + } }