Skip to content

Commit 160018b

Browse files
xedinxedin
authored
[Build/PackageGraph] Sink fallback logic for macro/plugin/test products and packages identification into ModulesGraph (#7646)
This is a temporary fix until we can figure out a proper way to handle situations were all we get is a name of a product or a target. ### Motivation: Callers or `ModulesGraph.{product, target}(for:destination:)` cannot always know the right `destination` to use at the moment because i.e. for test products and targets its contextual. We need a proper fix for this at the level or BuildSystem but for now sinking the logic down into `ModulesGraph` is the safest option. ### Modifications: - `ModulesGraph.{product, target}(for:destination:)` can handle fallback if product/target turn out to be a macro, a plugin or a test. ### Result: `--target` and `--product` options should work correctly regardless of underlying product/target kind. Resolves: rdar://129400066
1 parent 3b9dd49 commit 160018b

File tree

7 files changed

+182
-47
lines changed

7 files changed

+182
-47
lines changed

‎Sources/Build/BuildOperation.swift

Lines changed: 15 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
516516
}
517517

518518
/// Compute the llbuild target name using the given subset.
519-
private func computeLLBuildTargetName(for subset: BuildSubset) throws -> String {
519+
func computeLLBuildTargetName(for subset: BuildSubset) throws -> String {
520520
switch subset {
521521
case .allExcludingTests:
522522
return LLBuildManifestBuilder.TargetKind.main.targetName
@@ -526,36 +526,22 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
526526
// FIXME: This is super unfortunate that we might need to load the package graph.
527527
let graph = try getPackageGraph()
528528

529-
var product = graph.product(
529+
let product = graph.product(
530530
for: productName,
531531
destination: destination == .host ? .tools : .destination
532532
)
533533

534-
var buildParameters = if destination == .host {
535-
self.toolsBuildParameters
536-
} else {
537-
self.productsBuildParameters
538-
}
539-
540-
// It's possible to request a build of a macro or a plugin via `swift build`
541-
// which won't have the right destination set because it's impossible to indicate it.
542-
//
543-
// Same happens with `--test-product` - if one of the test targets directly references
544-
// a macro then all if its targets and the product itself become `host`.
545-
if product == nil && destination == .target {
546-
if let toolsProduct = graph.product(for: productName, destination: .tools),
547-
toolsProduct.type == .macro || toolsProduct.type == .plugin || toolsProduct.type == .test
548-
{
549-
product = toolsProduct
550-
buildParameters = self.toolsBuildParameters
551-
}
552-
}
553-
554534
guard let product else {
555535
observabilityScope.emit(error: "no product named '\(productName)'")
556536
throw Diagnostics.fatalError
557537
}
558538

539+
let buildParameters = if product.buildTriple == .tools {
540+
self.toolsBuildParameters
541+
} else {
542+
self.productsBuildParameters
543+
}
544+
559545
// If the product is automatic, we build the main target because automatic products
560546
// do not produce a binary right now.
561547
if product.type == .library(.automatic) {
@@ -570,33 +556,22 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
570556
// FIXME: This is super unfortunate that we might need to load the package graph.
571557
let graph = try getPackageGraph()
572558

573-
var target = graph.target(
559+
let target = graph.target(
574560
for: targetName,
575561
destination: destination == .host ? .tools : .destination
576562
)
577563

578-
var buildParameters = if destination == .host {
579-
self.toolsBuildParameters
580-
} else {
581-
self.productsBuildParameters
582-
}
583-
584-
// It's possible to request a build of a macro or a plugin via `swift build`
585-
// which won't have the right destination because it's impossible to indicate it.
586-
if target == nil && destination == .target {
587-
if let toolsTarget = graph.target(for: targetName, destination: .tools),
588-
toolsTarget.type == .macro || toolsTarget.type == .plugin
589-
{
590-
target = toolsTarget
591-
buildParameters = self.toolsBuildParameters
592-
}
593-
}
594-
595564
guard let target else {
596565
observabilityScope.emit(error: "no target named '\(targetName)'")
597566
throw Diagnostics.fatalError
598567
}
599568

569+
let buildParameters = if target.buildTriple == .tools {
570+
self.toolsBuildParameters
571+
} else {
572+
self.productsBuildParameters
573+
}
574+
600575
return target.getLLBuildTargetName(buildParameters: buildParameters)
601576
}
602577
}

‎Sources/Commands/Utilities/PluginDelegate.swift

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -389,8 +389,19 @@ final class PluginDelegate: PluginInvocationDelegate {
389389
throw StringError("could not find a target named “\(targetName)”")
390390
}
391391

392+
// FIXME: This is currently necessary because `target(for:destination:)` can
393+
// produce a module that is targeting host when `targetName`` corresponds to
394+
// a macro, plugin, or a test. Ideally we'd ask a build system for a`BuildSubset`
395+
// and get the destination from there but there are other places that need
396+
// refactoring in that way as well.
397+
let buildParameters = if target.buildTriple == .tools {
398+
try swiftCommandState.toolsBuildParameters
399+
} else {
400+
try swiftCommandState.productsBuildParameters
401+
}
402+
392403
// Build the target, if needed.
393-
try buildSystem.build(subset: .target(target.name))
404+
try buildSystem.build(subset: .target(target.name, for: buildParameters.destination))
394405

395406
// Configure the symbol graph extractor.
396407
var symbolGraphExtractor = try SymbolGraphExtract(
@@ -419,7 +430,7 @@ final class PluginDelegate: PluginInvocationDelegate {
419430
guard let package = packageGraph.package(for: target) else {
420431
throw StringError("could not determine the package for target “\(target.name)”")
421432
}
422-
let outputDir = try buildSystem.buildPlan.toolsBuildParameters.dataPath.appending(
433+
let outputDir = try buildParameters.dataPath.appending(
423434
components: "extracted-symbols",
424435
package.identity.description,
425436
target.name
@@ -430,7 +441,7 @@ final class PluginDelegate: PluginInvocationDelegate {
430441
let result = try symbolGraphExtractor.extractSymbolGraph(
431442
module: target,
432443
buildPlan: try buildSystem.buildPlan,
433-
buildParameters: buildSystem.buildPlan.destinationBuildParameters,
444+
buildParameters: buildParameters,
434445
outputRedirection: .collect,
435446
outputDirectory: outputDir,
436447
verboseOutput: self.swiftCommandState.logLevel <= .info

‎Sources/PackageGraph/ModulesGraph.swift

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,11 +167,57 @@ public struct ModulesGraph {
167167
}
168168

169169
public func product(for name: String, destination: BuildTriple) -> ResolvedProduct? {
170-
self.allProducts.first { $0.name == name && $0.buildTriple == destination }
170+
func findProduct(name: String, destination: BuildTriple) -> ResolvedProduct? {
171+
self.allProducts.first { $0.name == name && $0.buildTriple == destination }
172+
}
173+
174+
if let product = findProduct(name: name, destination: destination) {
175+
return product
176+
}
177+
178+
// FIXME: This is a temporary workaround and needs to be handled by the callers.
179+
180+
// It's possible to request a build of a macro, a plugin, or a test via `swift build`
181+
// which won't have the right destination set because it's impossible to indicate it.
182+
//
183+
// Same happens with `--test-product` - if one of the test targets directly references
184+
// a macro then all if its targets and the product itself become `host`.
185+
if destination == .destination {
186+
if let toolsProduct = findProduct(name: name, destination: .tools),
187+
toolsProduct.type == .macro || toolsProduct.type == .plugin || toolsProduct.type == .test
188+
{
189+
return toolsProduct
190+
}
191+
}
192+
193+
return nil
171194
}
172195

173196
public func target(for name: String, destination: BuildTriple) -> ResolvedModule? {
174-
self.allTargets.first { $0.name == name && $0.buildTriple == destination }
197+
func findModule(name: String, destination: BuildTriple) -> ResolvedModule? {
198+
self.allTargets.first { $0.name == name && $0.buildTriple == destination }
199+
}
200+
201+
if let module = findModule(name: name, destination: destination) {
202+
return module
203+
}
204+
205+
// FIXME: This is a temporary workaround and needs to be handled by the callers.
206+
207+
// It's possible to request a build of a macro, a plugin or a test via `swift build`
208+
// which won't have the right destination set because it's impossible to indicate it.
209+
//
210+
// Same happens with `--test-product` - if one of the test targets directly references
211+
// a macro then all if its targets and the product itself become `host`.
212+
if destination == .destination {
213+
if let toolsModule = findModule(name: name, destination: .tools),
214+
toolsModule.type == .macro || toolsModule.type == .plugin || toolsModule.type == .test
215+
{
216+
return toolsModule
217+
}
218+
}
219+
220+
return nil
175221
}
176222

177223
/// All root and root dependency packages provided as input to the graph.

‎Sources/SPMTestSupport/MockPackageGraphs.swift

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,9 +135,11 @@ public func macrosPackageGraph() throws -> MockPackageGraph {
135135
@_spi(SwiftPMInternal)
136136
public func macrosTestsPackageGraph() throws -> MockPackageGraph {
137137
let fs = InMemoryFileSystem(emptyFiles:
138+
"/swift-mmio/Plugins/MMIOPlugin/source.swift",
138139
"/swift-mmio/Sources/MMIO/source.swift",
139140
"/swift-mmio/Sources/MMIOMacros/source.swift",
140141
"/swift-mmio/Sources/MMIOMacrosTests/source.swift",
142+
"/swift-mmio/Sources/MMIOMacro+PluginTests/source.swift",
141143
"/swift-syntax/Sources/SwiftSyntax/source.swift",
142144
"/swift-syntax/Sources/SwiftSyntaxMacrosTestSupport/source.swift",
143145
"/swift-syntax/Sources/SwiftSyntaxMacros/source.swift",
@@ -164,6 +166,11 @@ public func macrosTestsPackageGraph() throws -> MockPackageGraph {
164166
name: "MMIO",
165167
type: .library(.automatic),
166168
targets: ["MMIO"]
169+
),
170+
ProductDescription(
171+
name: "MMIOPlugin",
172+
type: .plugin,
173+
targets: ["MMIOPlugin"]
167174
)
168175
],
169176
targets: [
@@ -179,13 +186,26 @@ public func macrosTestsPackageGraph() throws -> MockPackageGraph {
179186
],
180187
type: .macro
181188
),
189+
TargetDescription(
190+
name: "MMIOPlugin",
191+
type: .plugin,
192+
pluginCapability: .buildTool
193+
),
182194
TargetDescription(
183195
name: "MMIOMacrosTests",
184196
dependencies: [
185197
.target(name: "MMIOMacros"),
186198
.product(name: "SwiftSyntaxMacrosTestSupport", package: "swift-syntax")
187199
],
188200
type: .test
201+
),
202+
TargetDescription(
203+
name: "MMIOMacro+PluginTests",
204+
dependencies: [
205+
.target(name: "MMIOPlugin"),
206+
.target(name: "MMIOMacros")
207+
],
208+
type: .test
189209
)
190210
]
191211
),

‎Tests/BuildTests/BuildOperationTests.swift

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import LLBuildManifest
1818
@_spi(DontAdoptOutsideOfSwiftPMExposedForBenchmarksAndTestsOnly)
1919
import PackageGraph
2020
import SPMBuildCore
21+
@_spi(SwiftPMInternal)
2122
import SPMTestSupport
2223
import XCTest
2324

@@ -179,4 +180,41 @@ final class BuildOperationTests: XCTestCase {
179180
}
180181
}
181182
}
183+
184+
func testHostProductsAndTargetsWithoutExplicitDestination() throws {
185+
let mock = try macrosTestsPackageGraph()
186+
187+
let op = mockBuildOperation(
188+
productsBuildParameters: mockBuildParameters(destination: .target),
189+
toolsBuildParameters: mockBuildParameters(destination: .host),
190+
packageGraphLoader: { mock.graph },
191+
scratchDirectory: AbsolutePath("/.build/\(hostTriple)"),
192+
fs: mock.fileSystem,
193+
observabilityScope: mock.observabilityScope
194+
)
195+
196+
XCTAssertEqual(
197+
"MMIOMacros-\(hostTriple)-debug-tool.exe",
198+
try op.computeLLBuildTargetName(for: .product("MMIOMacros"))
199+
)
200+
201+
for target in ["MMIOMacros", "MMIOPlugin", "MMIOMacrosTests", "MMIOMacro+PluginTests"] {
202+
XCTAssertEqual(
203+
"\(target)-\(hostTriple)-debug-tool.module",
204+
try op.computeLLBuildTargetName(for: .target(target))
205+
)
206+
}
207+
208+
let dependencies = try BuildSubset.target("MMIOMacro+PluginTests").recursiveDependencies(
209+
for: mock.graph,
210+
observabilityScope: mock.observabilityScope
211+
)
212+
213+
XCTAssertNotNil(dependencies)
214+
XCTAssertTrue(dependencies!.count > 0)
215+
216+
for dependency in dependencies! {
217+
XCTAssertEqual(dependency.buildTriple, .tools)
218+
}
219+
}
182220
}

‎Tests/BuildTests/CrossCompilationBuildPlanTests.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ final class CrossCompilationBuildPlanTests: XCTestCase {
302302
)
303303
let result = try BuildPlanResult(plan: plan)
304304
result.checkProductsCount(2)
305-
result.checkTargetsCount(15)
305+
result.checkTargetsCount(16)
306306

307307
XCTAssertTrue(try result.allTargets(named: "SwiftSyntax")
308308
.map { try $0.swiftTarget() }

‎Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,10 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
8383
}
8484
}
8585

86+
result.checkProduct("MMIOMacros", destination: .destination) { result in
87+
result.check(buildTriple: .tools)
88+
}
89+
8690
result.checkTargets("SwiftSyntax") { results in
8791
XCTAssertEqual(results.count, 2)
8892

@@ -100,6 +104,7 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
100104
result.check(
101105
targets: "MMIO",
102106
"MMIOMacros",
107+
"MMIOPlugin",
103108
"SwiftCompilerPlugin",
104109
"SwiftCompilerPlugin",
105110
"SwiftCompilerPluginMessageHandling",
@@ -111,14 +116,15 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
111116
"SwiftSyntaxMacrosTestSupport",
112117
"SwiftSyntaxMacrosTestSupport"
113118
)
114-
result.check(testModules: "MMIOMacrosTests")
119+
result.check(testModules: "MMIOMacrosTests", "MMIOMacro+PluginTests")
115120
result.checkTarget("MMIO") { result in
116121
result.check(buildTriple: .destination)
117122
result.check(dependencies: "MMIOMacros")
118123
}
119124
result.checkTargets("MMIOMacros") { results in
120125
XCTAssertEqual(results.count, 1)
121126
}
127+
122128
result.checkTarget("MMIOMacrosTests", destination: .tools) { result in
123129
result.check(buildTriple: .tools)
124130
result.checkDependency("MMIOMacros") { result in
@@ -146,6 +152,14 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
146152
}
147153
}
148154

155+
result.checkTarget("MMIOMacros", destination: .destination) { result in
156+
result.check(buildTriple: .tools)
157+
}
158+
159+
result.checkTarget("MMIOMacrosTests", destination: .destination) { result in
160+
result.check(buildTriple: .tools)
161+
}
162+
149163
result.checkTargets("SwiftSyntax") { results in
150164
XCTAssertEqual(results.count, 2)
151165

@@ -168,4 +182,35 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
168182
}
169183
}
170184
}
185+
186+
func testPlugins() throws {
187+
let graph = try macrosTestsPackageGraph().graph
188+
PackageGraphTester(graph) { result in
189+
result.checkProduct("MMIOPlugin", destination: .tools) { result in
190+
result.check(buildTriple: .tools)
191+
}
192+
193+
result.checkProduct("MMIOPlugin", destination: .destination) { result in
194+
result.check(buildTriple: .tools)
195+
}
196+
197+
result.checkTarget("MMIOPlugin", destination: .tools) { result in
198+
result.check(buildTriple: .tools)
199+
}
200+
201+
result.checkTarget("MMIOPlugin", destination: .destination) { result in
202+
result.check(buildTriple: .tools)
203+
}
204+
205+
result.checkTarget("MMIOMacro+PluginTests", destination: .tools) { result in
206+
result.check(buildTriple: .tools)
207+
result.check(dependencies: "MMIOPlugin", "MMIOMacros")
208+
}
209+
210+
result.checkTarget("MMIOMacro+PluginTests", destination: .destination) { result in
211+
result.check(buildTriple: .tools)
212+
result.check(dependencies: "MMIOPlugin", "MMIOMacros")
213+
}
214+
}
215+
}
171216
}

0 commit comments

Comments
 (0)