Skip to content

Commit a0d1600

Browse files
authored
Fix duplicate modulemap errors with macro and plugin deps (swiftlang#8472)
We were including flags to hook up modulemaps and include files to C library dependencies in macros and plugin tools through to the modules that depend on those. This adds the capability to prune the depth first searches through the dependencies to ensure these are skipped when crossing macro and plugin boundaries.
1 parent cd5d9dd commit a0d1600

File tree

7 files changed

+150
-8
lines changed

7 files changed

+150
-8
lines changed

Sources/Basics/Graph/GraphAlgorithms.swift

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,3 +131,50 @@ public func depthFirstSearch<T: Hashable>(
131131
}
132132
}
133133
}
134+
135+
/// Implements a pre-order depth-first search that traverses the whole graph and
136+
/// doesn't distinguish between unique and duplicate nodes. The visitor can abort
137+
/// a path as needed to prune the tree.
138+
/// The method expects the graph to be acyclic but doesn't check that.
139+
///
140+
/// - Parameters:
141+
/// - nodes: The list of input nodes to sort.
142+
/// - successors: A closure for fetching the successors of a particular node.
143+
/// - onNext: A callback to indicate the node currently being processed
144+
/// including its parent (if any) and its depth. Returns whether to
145+
/// continue down the current path.
146+
///
147+
/// - Complexity: O(v + e) where (v, e) are the number of vertices and edges
148+
/// reachable from the input nodes via the relation.
149+
public enum DepthFirstContinue {
150+
case `continue`
151+
case abort
152+
}
153+
154+
public func depthFirstSearch<T: Hashable>(
155+
_ nodes: [T],
156+
successors: (T) throws -> [T],
157+
visitNext: (T, _ parent: T?) throws -> DepthFirstContinue
158+
) rethrows {
159+
var stack = OrderedSet<TraversalNode<T>>()
160+
161+
for node in nodes {
162+
precondition(stack.isEmpty)
163+
stack.append(TraversalNode(parent: nil, curr: node))
164+
165+
while !stack.isEmpty {
166+
let node = stack.removeLast()
167+
168+
if try visitNext(node.curr, node.parent) == .continue {
169+
for succ in try successors(node.curr) {
170+
stack.append(
171+
TraversalNode(
172+
parent: node.curr,
173+
curr: succ
174+
)
175+
)
176+
}
177+
}
178+
}
179+
}
180+
}

Sources/Build/BuildDescription/ModuleBuildDescription.swift

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,8 +187,30 @@ extension ModuleBuildDescription {
187187
var dependencies: [Dependency] = []
188188
plan.traverseDependencies(of: self) { product, _, description in
189189
dependencies.append(.product(product, description))
190+
return .continue
190191
} onModule: { module, _, description in
191192
dependencies.append(.module(module, description))
193+
return .continue
194+
}
195+
return dependencies
196+
}
197+
198+
package func recursiveLinkDependencies(using plan: BuildPlan) -> [Dependency] {
199+
var dependencies: [Dependency] = []
200+
plan.traverseDependencies(of: self) { product, _, description in
201+
guard product.type != .macro && product.type != .plugin else {
202+
return .abort
203+
}
204+
205+
dependencies.append(.product(product, description))
206+
return .continue
207+
} onModule: { module, _, description in
208+
guard module.type != .macro && module.type != .plugin else {
209+
return .abort
210+
}
211+
212+
dependencies.append(.module(module, description))
213+
return .continue
192214
}
193215
return dependencies
194216
}

Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,6 +1042,12 @@ extension SwiftModuleBuildDescription {
10421042
ModuleBuildDescription.swift(self).dependencies(using: plan)
10431043
}
10441044

1045+
package func recursiveLinkDependencies(
1046+
using plan: BuildPlan
1047+
) -> [ModuleBuildDescription.Dependency] {
1048+
ModuleBuildDescription.swift(self).recursiveLinkDependencies(using: plan)
1049+
}
1050+
10451051
package func recursiveDependencies(
10461052
using plan: BuildPlan
10471053
) -> [ModuleBuildDescription.Dependency] {

Sources/Build/BuildPlan/BuildPlan+Swift.swift

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import class PackageModel.SystemLibraryModule
1919
extension BuildPlan {
2020
func plan(swiftTarget: SwiftModuleBuildDescription) throws {
2121
// We need to iterate recursive dependencies because Swift compiler needs to see all the targets a target
22-
// depends on.
22+
// builds against
2323
for case .module(let dependency, let description) in swiftTarget.recursiveDependencies(using: self) {
2424
switch dependency.underlying {
2525
case let underlyingTarget as ClangModule where underlyingTarget.type == .library:
@@ -53,5 +53,4 @@ extension BuildPlan {
5353
}
5454
}
5555
}
56-
5756
}

Sources/Build/BuildPlan/BuildPlan.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1154,8 +1154,8 @@ extension BuildPlan {
11541154

11551155
package func traverseDependencies(
11561156
of description: ModuleBuildDescription,
1157-
onProduct: (ResolvedProduct, BuildParameters.Destination, ProductBuildDescription?) -> Void,
1158-
onModule: (ResolvedModule, BuildParameters.Destination, ModuleBuildDescription?) -> Void
1157+
onProduct: (ResolvedProduct, BuildParameters.Destination, ProductBuildDescription?) -> DepthFirstContinue,
1158+
onModule: (ResolvedModule, BuildParameters.Destination, ModuleBuildDescription?) -> DepthFirstContinue
11591159
) {
11601160
var visited = Set<TraversalNode>()
11611161
func successors(
@@ -1196,16 +1196,16 @@ extension BuildPlan {
11961196
case .package:
11971197
[]
11981198
}
1199-
} onNext: { module, _ in
1199+
} visitNext: { module, _ in
12001200
switch module {
12011201
case .package:
1202-
break
1202+
return .continue
12031203

12041204
case .product(let product, let destination):
1205-
onProduct(product, destination, self.description(for: product, context: destination))
1205+
return onProduct(product, destination, self.description(for: product, context: destination))
12061206

12071207
case .module(let module, let destination):
1208-
onModule(module, destination, self.description(for: module, context: destination))
1208+
return onModule(module, destination, self.description(for: module, context: destination))
12091209
}
12101210
}
12111211
}

Tests/BuildTests/BuildPlanTests.swift

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6909,6 +6909,72 @@ class BuildPlanTestCase: BuildSystemProviderTestCase {
69096909
XCTAssertMatch(contents, .regex(#"args: \[.*"-I","/testpackagedep/SomeArtifact.xcframework/macos/Headers".*,"/testpackage/Sources/CLib/lib.c".*]"#))
69106910
XCTAssertMatch(contents, .regex(#"args: \[.*"-module-name","SwiftLib",.*"-I","/testpackagedep/SomeArtifact.xcframework/macos/Headers".*]"#))
69116911
}
6912+
6913+
func testMacroPluginDependencyLeakage() async throws {
6914+
// Make sure the include paths from macro and plugin executables don't leak into dependents
6915+
let observability = ObservabilitySystem.makeForTesting()
6916+
let fs = InMemoryFileSystem(emptyFiles: [
6917+
"/LeakTest/Sources/CLib/include/Clib.h",
6918+
"/LeakTest/Sources/CLib/Clib.c",
6919+
"/LeakTest/Sources/MyMacro/MyMacro.swift",
6920+
"/LeakTest/Sources/MyPluginTool/MyPluginTool.swift",
6921+
"/LeakTest/Plugins/MyPlugin/MyPlugin.swift",
6922+
"/LeakTest/Sources/MyLib/MyLib.swift",
6923+
"/LeakLib/Sources/CLib2/include/Clib.h",
6924+
"/LeakLib/Sources/CLib2/Clib.c",
6925+
"/LeakLib/Sources/MyMacro2/MyMacro.swift",
6926+
"/LeakLib/Sources/MyPluginTool2/MyPluginTool.swift",
6927+
"/LeakLib/Plugins/MyPlugin2/MyPlugin.swift",
6928+
"/LeakLib/Sources/MyLib2/MyLib.swift"
6929+
])
6930+
6931+
let graph = try loadModulesGraph(fileSystem: fs, manifests: [
6932+
Manifest.createFileSystemManifest(
6933+
displayName: "LeakLib",
6934+
path: "/LeakLib",
6935+
products: [
6936+
ProductDescription(name: "MyLib2", type: .library(.automatic), targets: ["MyLib2"]),
6937+
],
6938+
targets: [
6939+
TargetDescription(name: "CLib2"),
6940+
TargetDescription(name: "MyMacro2", dependencies: ["CLib2"], type: .macro),
6941+
TargetDescription(name: "MyPluginTool2", dependencies: ["CLib2"], type: .executable),
6942+
TargetDescription(name: "MyPlugin2", dependencies: ["MyPluginTool2"], type: .plugin, pluginCapability: .buildTool),
6943+
TargetDescription(name: "MyLib2", dependencies: ["CLib2", "MyMacro2"], pluginUsages: [.plugin(name: "MyPlugin2", package: nil)]),
6944+
]
6945+
),
6946+
Manifest.createRootManifest(
6947+
displayName: "LeakTest",
6948+
path: "/LeakTest",
6949+
dependencies: [
6950+
.fileSystem(path: "/LeakLib")
6951+
],
6952+
targets: [
6953+
TargetDescription(name: "CLib"),
6954+
TargetDescription(name: "MyMacro", dependencies: ["CLib"], type: .macro),
6955+
TargetDescription(name: "MyPluginTool", dependencies: ["CLib"], type: .executable),
6956+
TargetDescription(name: "MyPlugin", dependencies: ["MyPluginTool"], type: .plugin, pluginCapability: .buildTool),
6957+
TargetDescription(
6958+
name: "MyLib",
6959+
dependencies: ["CLib", "MyMacro", .product(name: "MyLib2", package: "LeakLib")],
6960+
pluginUsages: [.plugin(name: "MyPlugin", package: nil)]
6961+
),
6962+
]
6963+
)
6964+
], observabilityScope: observability.topScope)
6965+
XCTAssertNoDiagnostics(observability.diagnostics)
6966+
6967+
let plan = try await mockBuildPlan(
6968+
graph: graph,
6969+
fileSystem: fs,
6970+
observabilityScope: observability.topScope
6971+
)
6972+
XCTAssertNoDiagnostics(observability.diagnostics)
6973+
6974+
let myLib = try XCTUnwrap(plan.targets.first(where: { $0.module.name == "MyLib" })).swift()
6975+
print(myLib.additionalFlags)
6976+
XCTAssertFalse(myLib.additionalFlags.contains(where: { $0.contains("-tool/include")}), "flags shouldn't contain tools items")
6977+
}
69126978
}
69136979

69146980
class BuildPlanNativeTests: BuildPlanTestCase {

Tests/BuildTests/BuildPlanTraversalTests.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,8 +146,10 @@ final class BuildPlanTraversalTests: XCTestCase {
146146
XCTAssertEqual(product.name, "SwiftSyntax")
147147
XCTAssertEqual(destination, .host)
148148
XCTAssertNil(description)
149+
return .continue
149150
} onModule: { module, destination, description in
150151
moduleDependencies.append((module, destination, description))
152+
return .continue
151153
}
152154

153155
XCTAssertEqual(moduleDependencies.count, 2)

0 commit comments

Comments
 (0)