Skip to content

Commit 3098b2d

Browse files
xedinxedin
committed
[PackageGraph] Allow package-level cyclic dependency only for >= 6.0 manifests
Follow-up to #7530 Otherwise it might be suprising for package authors to discover that their packages cannot be used with older tools because they inadvertently introduced a cyclic dependency in a new version.
1 parent c965d5a commit 3098b2d

File tree

5 files changed

+168
-22
lines changed

5 files changed

+168
-22
lines changed

‎CHANGELOG.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ Note: This is in reverse chronological order, so newer entries are added to the
22

33
* [#7530]
44

5-
Makes it possible for packages to depend on each other if such dependency doesn't form any target-level cycles. For example,
6-
package `A` can depend on `B` and `B` on `A` unless targets in `B` depend on products of `A` that depend on some of the same
5+
Starting from tools-version 6.0 makes it possible for packages to depend on each other if such dependency doesn't form any target-level cycles.
6+
For example, package `A` can depend on `B` and `B` on `A` unless targets in `B` depend on products of `A` that depend on some of the same
77
targets from `B` and vice versa.
88

99
Swift 6.0

‎Sources/PackageGraph/ModulesGraph+Loading.swift

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,16 @@ extension ModulesGraph {
6363
)
6464
}
6565
}
66-
let inputManifests = rootManifestNodes + rootDependencyNodes
66+
let inputManifests = (rootManifestNodes + rootDependencyNodes).map {
67+
KeyedPair($0, key: $0.id)
68+
}
6769

6870
// Collect the manifests for which we are going to build packages.
6971
var allNodes = [GraphLoadingNode]()
7072

71-
// Cycles in dependencies don't matter as long as there are no target cycles between packages.
72-
depthFirstSearch(inputManifests.map { KeyedPair($0, key: $0.id) }) {
73-
$0.item.requiredDependencies.compactMap { dependency in
74-
manifestMap[dependency.identity].map { (manifest, fileSystem) in
73+
let nodeSuccessorProvider = { (node: KeyedPair<GraphLoadingNode, PackageIdentity>) in
74+
node.item.requiredDependencies.compactMap { dependency in
75+
manifestMap[dependency.identity].map { manifest, _ in
7576
KeyedPair(
7677
GraphLoadingNode(
7778
identity: dependency.identity,
@@ -82,7 +83,31 @@ extension ModulesGraph {
8283
)
8384
}
8485
}
85-
} onUnique: {
86+
}
87+
88+
// Package dependency cycles feature is gated on tools version 6.0.
89+
if !root.manifests.allSatisfy({ $1.toolsVersion >= .v6_0 }) {
90+
if let cycle = findCycle(inputManifests, successors: nodeSuccessorProvider) {
91+
let path = (cycle.path + cycle.cycle).map(\.item.manifest)
92+
observabilityScope.emit(PackageGraphError.dependencyCycleDetected(
93+
path: path, cycle: cycle.cycle[0].item.manifest
94+
))
95+
96+
return try ModulesGraph(
97+
rootPackages: [],
98+
rootDependencies: [],
99+
packages: IdentifiableSet(),
100+
dependencies: requiredDependencies,
101+
binaryArtifacts: binaryArtifacts
102+
)
103+
}
104+
}
105+
106+
// Cycles in dependencies don't matter as long as there are no target cycles between packages.
107+
depthFirstSearch(
108+
inputManifests,
109+
successors: nodeSuccessorProvider
110+
) {
86111
allNodes.append($0.item)
87112
} onDuplicate: { _,_ in
88113
// no de-duplication is required.

‎Sources/PackageGraph/ModulesGraph.swift

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ enum PackageGraphError: Swift.Error {
2222
case noModules(Package)
2323

2424
/// The package dependency declaration has cycle in it.
25-
case cycleDetected((path: [Manifest], cycle: [Manifest]))
25+
case dependencyCycleDetected(path: [Manifest], cycle: Manifest)
2626

2727
/// The product dependency not found.
2828
case productDependencyNotFound(
@@ -299,10 +299,10 @@ extension PackageGraphError: CustomStringConvertible {
299299
case .noModules(let package):
300300
return "package '\(package)' contains no products"
301301

302-
case .cycleDetected(let cycle):
303-
return "cyclic dependency declaration found: " +
304-
(cycle.path + cycle.cycle).map({ $0.displayName }).joined(separator: " -> ") +
305-
" -> " + cycle.cycle[0].displayName
302+
case .dependencyCycleDetected(let path, let package):
303+
return "cyclic dependency between packages " +
304+
(path.map({ $0.displayName }).joined(separator: " -> ")) +
305+
" -> \(package.displayName) requires tools-version 6.0 or later"
306306

307307
case .productDependencyNotFound(let package, let targetName, let dependencyProductName, let dependencyPackageName, let dependencyProductInDecl, let similarProductName, let packageContainingSimilarProduct):
308308
if dependencyProductInDecl {

‎Tests/PackageGraphTests/ModulesGraphTests.swift

Lines changed: 127 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,10 @@ final class ModulesGraphTests: XCTestCase {
186186
)
187187

188188
testDiagnostics(observability.diagnostics) { result in
189-
result.check(diagnostic: "cyclic dependency declaration found: Foo -> Bar -> Baz -> Bar", severity: .error)
189+
result.check(
190+
diagnostic: "cyclic dependency between packages Foo -> Bar -> Baz -> Bar requires tools-version 6.0 or later",
191+
severity: .error
192+
)
190193
}
191194
}
192195

@@ -212,11 +215,132 @@ final class ModulesGraphTests: XCTestCase {
212215
)
213216

214217
testDiagnostics(observability.diagnostics) { result in
215-
result.check(diagnostic: "cyclic dependency declaration found: Bar -> Foo -> Bar", severity: .error)
218+
result.check(
219+
diagnostic: "cyclic dependency declaration found: Bar -> Foo -> Bar",
220+
severity: .error
221+
)
222+
}
223+
}
224+
225+
func testDependencyCycleWithoutTargetCycleV5() throws {
226+
let fs = InMemoryFileSystem(emptyFiles:
227+
"/Foo/Sources/Foo/source.swift",
228+
"/Bar/Sources/Bar/source.swift",
229+
"/Bar/Sources/Baz/source.swift"
230+
)
231+
232+
let observability = ObservabilitySystem.makeForTesting()
233+
let _ = try loadModulesGraph(
234+
fileSystem: fs,
235+
manifests: [
236+
Manifest.createRootManifest(
237+
displayName: "Foo",
238+
path: "/Foo",
239+
toolsVersion: .v5_10,
240+
dependencies: [
241+
.localSourceControl(path: "/Bar", requirement: .upToNextMajor(from: "1.0.0"))
242+
],
243+
products: [
244+
ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"])
245+
],
246+
targets: [
247+
TargetDescription(name: "Foo", dependencies: ["Bar"]),
248+
]),
249+
Manifest.createFileSystemManifest(
250+
displayName: "Bar",
251+
path: "/Bar",
252+
dependencies: [
253+
.localSourceControl(path: "/Foo", requirement: .upToNextMajor(from: "1.0.0"))
254+
],
255+
products: [
256+
ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]),
257+
ProductDescription(name: "Baz", type: .library(.automatic), targets: ["Baz"])
258+
],
259+
targets: [
260+
TargetDescription(name: "Bar"),
261+
TargetDescription(name: "Baz", dependencies: ["Foo"]),
262+
])
263+
],
264+
observabilityScope: observability.topScope
265+
)
266+
267+
testDiagnostics(observability.diagnostics) { result in
268+
result.check(
269+
diagnostic: "cyclic dependency between packages Foo -> Bar -> Foo requires tools-version 6.0 or later",
270+
severity: .error
271+
)
216272
}
217273
}
218274

219275
func testDependencyCycleWithoutTargetCycle() throws {
276+
let fs = InMemoryFileSystem(emptyFiles:
277+
"/A/Sources/A/source.swift",
278+
"/B/Sources/B/source.swift",
279+
"/C/Sources/C/source.swift"
280+
)
281+
282+
func testDependencyCycleDetection(rootToolsVersion: ToolsVersion) throws -> [Diagnostic] {
283+
let observability = ObservabilitySystem.makeForTesting()
284+
let _ = try loadModulesGraph(
285+
fileSystem: fs,
286+
manifests: [
287+
Manifest.createRootManifest(
288+
displayName: "A",
289+
path: "/A",
290+
toolsVersion: rootToolsVersion,
291+
dependencies: [
292+
.localSourceControl(path: "/B", requirement: .upToNextMajor(from: "1.0.0"))
293+
],
294+
products: [
295+
ProductDescription(name: "A", type: .library(.automatic), targets: ["A"])
296+
],
297+
targets: [
298+
TargetDescription(name: "A", dependencies: ["B"]),
299+
]
300+
),
301+
Manifest.createFileSystemManifest(
302+
displayName: "B",
303+
path: "/B",
304+
dependencies: [
305+
.localSourceControl(path: "/C", requirement: .upToNextMajor(from: "1.0.0"))
306+
],
307+
products: [
308+
ProductDescription(name: "B", type: .library(.automatic), targets: ["B"]),
309+
],
310+
targets: [
311+
TargetDescription(name: "B"),
312+
]
313+
),
314+
Manifest.createFileSystemManifest(
315+
displayName: "C",
316+
path: "/C",
317+
dependencies: [
318+
.localSourceControl(path: "/A", requirement: .upToNextMajor(from: "1.0.0"))
319+
],
320+
products: [
321+
ProductDescription(name: "C", type: .library(.automatic), targets: ["C"]),
322+
],
323+
targets: [
324+
TargetDescription(name: "C"),
325+
]
326+
)
327+
],
328+
observabilityScope: observability.topScope
329+
)
330+
return observability.diagnostics
331+
}
332+
333+
try testDiagnostics(testDependencyCycleDetection(rootToolsVersion: .v5)) { result in
334+
result.check(
335+
diagnostic: "cyclic dependency between packages A -> B -> C -> A requires tools-version 6.0 or later",
336+
severity: .error
337+
)
338+
}
339+
340+
try XCTAssertNoDiagnostics(testDependencyCycleDetection(rootToolsVersion: .v6_0))
341+
}
342+
343+
func testDependencyCycleWithoutTargetCycleV6() throws {
220344
let fs = InMemoryFileSystem(emptyFiles:
221345
"/Foo/Sources/Foo/source.swift",
222346
"/Bar/Sources/Bar/source.swift",
@@ -230,6 +354,7 @@ final class ModulesGraphTests: XCTestCase {
230354
Manifest.createRootManifest(
231355
displayName: "Foo",
232356
path: "/Foo",
357+
toolsVersion: .v6_0,
233358
dependencies: [
234359
.localSourceControl(path: "/Bar", requirement: .upToNextMajor(from: "1.0.0"))
235360
],

‎Tests/WorkspaceTests/WorkspaceTests.swift

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11061,7 +11061,7 @@ final class WorkspaceTests: XCTestCase {
1106111061
requirement: .upToNextMajor(from: "1.0.0")
1106211062
),
1106311063
],
11064-
toolsVersion: .v5
11064+
toolsVersion: .v6_0
1106511065
),
1106611066
],
1106711067
packages: [
@@ -11209,11 +11209,7 @@ final class WorkspaceTests: XCTestCase {
1120911209
// FIXME: rdar://72940946
1121011210
// we need to improve this situation or diagnostics when working on identity
1121111211
result.check(
11212-
diagnostic: "'bar' dependency on '/tmp/ws/pkgs/other/utility' conflicts with dependency on '/tmp/ws/pkgs/foo/utility' which has the same identity 'utility'. this will be escalated to an error in future versions of SwiftPM.",
11213-
severity: .warning
11214-
)
11215-
result.check(
11216-
diagnostic: "product 'OtherUtilityProduct' required by package 'bar' target 'BarTarget' not found in package 'OtherUtilityPackage'.",
11212+
diagnostic: "cyclic dependency between packages Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage requires tools-version 6.0 or later",
1121711213
severity: .error
1121811214
)
1121911215
}
@@ -11244,7 +11240,7 @@ final class WorkspaceTests: XCTestCase {
1124411240
requirement: .upToNextMajor(from: "1.0.0")
1124511241
),
1124611242
],
11247-
toolsVersion: .v5
11243+
toolsVersion: .v6_0
1124811244
),
1124911245
],
1125011246
packages: [

0 commit comments

Comments
 (0)