Skip to content

Commit c9186a5

Browse files
xedinxedin
authored
[PackageGraph] Allow package-level cyclic dependency only for >= 6.0 … (#7579)
…manifests ### Motivation: 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. ### Modifications: `ModulesGraph.load` has been adjusted to run `findCycle` some of the root manifests support tools version that is older than 6.0. New diagnostic was added to help guide package authors to update their versions if they have cyclic package-level dependencies. ### Result: Attempts to introduce cyclic package-level dependencies to manifests that support tools-versions less than 6.0 would result in a failure.
1 parent c965d5a commit c9186a5

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)