Skip to content

Commit

Permalink
Fix issues with excluding macros/plugins from dependency computation (#…
Browse files Browse the repository at this point in the history
…6723)

While fixing this, I also noticed the original issue also exists for executable targets, so this gets fixed here as well.

There's one unfortunate nuance here for test targets since using a macro/plugin is indistinguishable from needing to link it because it is being tested. We err on the side of caution here and will always link.

(sidenote: theoretically, plugins *do* distinguish between linkage and use in the package manifest, but this distinction is not carried forward into the actual model)

Partially fixes https://github.com/apple/swift/issues/67371 since the underlying project also does not declare a dependency on the macro that is being tested.
  • Loading branch information
neonichu committed Jul 19, 2023
1 parent 068fa49 commit b31c19a
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 8 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// swift-tools-version: 5.6
// swift-tools-version: 5.9
import PackageDescription

let package = Package(
Expand Down
38 changes: 31 additions & 7 deletions Sources/Build/BuildPlan.swift
Original file line number Diff line number Diff line change
Expand Up @@ -695,13 +695,28 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
libraryBinaryPaths: Set<AbsolutePath>,
availableTools: [String: AbsolutePath]
) {
/* Prior to tools-version 5.9, we used to errorneously recursively traverse plugin dependencies and statically include their
/* Prior to tools-version 5.9, we used to errorneously recursively traverse executable/plugin dependencies and statically include their
targets. For compatibility reasons, we preserve that behavior for older tools-versions. */
let shouldExcludePlugins: Bool
let shouldExcludeExecutablesAndPlugins: Bool
if let toolsVersion = self.graph.package(for: product)?.manifest.toolsVersion {
shouldExcludePlugins = toolsVersion >= .v5_9
shouldExcludeExecutablesAndPlugins = toolsVersion >= .v5_9
} else {
shouldExcludePlugins = false
shouldExcludeExecutablesAndPlugins = false
}

// For test targets, we need to consider the first level of transitive dependencies since the first level is always test targets.
let topLevelDependencies: [PackageModel.Target]
if product.type == .test {
topLevelDependencies = product.targets.flatMap { $0.underlyingTarget.dependencies }.compactMap {
switch $0 {
case .product:
return nil
case .target(let target, _):
return target
}
}
} else {
topLevelDependencies = []
}

// Sort the product targets in topological order.
Expand All @@ -710,10 +725,19 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
switch dependency {
// Include all the dependencies of a target.
case .target(let target, _):
if target.type == .macro {
let isTopLevel = topLevelDependencies.contains(target.underlyingTarget) || product.targets.contains(target)
let topLevelIsExecutable = isTopLevel && product.type == .executable
let topLevelIsMacro = isTopLevel && product.type == .macro
let topLevelIsPlugin = isTopLevel && product.type == .plugin
let topLevelIsTest = isTopLevel && product.type == .test

if !topLevelIsMacro && !topLevelIsTest && target.type == .macro {
return []
}
if shouldExcludeExecutablesAndPlugins, !topLevelIsPlugin && !topLevelIsTest && target.type == .plugin {
return []
}
if shouldExcludePlugins, target.type == .plugin {
if shouldExcludeExecutablesAndPlugins, !topLevelIsExecutable && topLevelIsTest && target.type == .executable {
return []
}
return target.dependencies.filter { $0.satisfies(self.buildEnvironment) }
Expand All @@ -730,7 +754,7 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
case .library(.automatic), .library(.static):
return productDependencies
case .plugin:
return shouldExcludePlugins ? [] : productDependencies
return shouldExcludeExecutablesAndPlugins ? [] : productDependencies
case .library(.dynamic), .test, .executable, .snippet, .macro:
return []
}
Expand Down

0 comments on commit b31c19a

Please sign in to comment.