Skip to content

[Build/PackageGraph] Switch BuildSubset.{product, target} and `Modu… #7650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions Sources/Build/BuildOperation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Sources/Commands/Utilities/PluginDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)”")
}

Expand Down
44 changes: 24 additions & 20 deletions Sources/PackageGraph/ModulesGraph.swift
Original file line number Diff line number Diff line change
Expand Up @@ -166,55 +166,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
Expand Down
10 changes: 6 additions & 4 deletions Sources/SPMBuildCore/BuildSystem/BuildSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions Sources/SPMTestSupport/PackageGraphTester.swift
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public final class PackageGraphResult {

public func checkTarget(
_ name: String,
destination: BuildTriple = .destination,
destination: BuildTriple? = .none,
file: StaticString = #file,
line: UInt = #line,
body: (ResolvedTargetResult) -> Void
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
}
}

result.checkProduct("MMIOMacros", destination: .destination) { result in
result.checkProduct("MMIOMacros") { result in
result.check(buildTriple: .tools)
}

Expand Down Expand Up @@ -152,11 +152,11 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
}
}

result.checkTarget("MMIOMacros", destination: .destination) { result in
result.checkTarget("MMIOMacros") { result in
result.check(buildTriple: .tools)
}

result.checkTarget("MMIOMacrosTests", destination: .destination) { result in
result.checkTarget("MMIOMacrosTests") { result in
result.check(buildTriple: .tools)
}

Expand Down Expand Up @@ -190,15 +190,15 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
result.check(buildTriple: .tools)
}

result.checkProduct("MMIOPlugin", destination: .destination) { result in
result.checkProduct("MMIOPlugin") { result in
result.check(buildTriple: .tools)
}

result.checkTarget("MMIOPlugin", destination: .tools) { result in
result.check(buildTriple: .tools)
}

result.checkTarget("MMIOPlugin", destination: .destination) { result in
result.checkTarget("MMIOPlugin") { result in
result.check(buildTriple: .tools)
}

Expand All @@ -207,7 +207,7 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
result.check(dependencies: "MMIOPlugin", "MMIOMacros")
}

result.checkTarget("MMIOMacro+PluginTests", destination: .destination) { result in
result.checkTarget("MMIOMacro+PluginTests") { result in
result.check(buildTriple: .tools)
result.check(dependencies: "MMIOPlugin", "MMIOMacros")
}
Expand Down