Skip to content

[PackageGraph] Allow package-level cyclic dependency only for >= 6.0 … #7579

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.

Sign up for GitHub

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
May 22, 2024
Merged

[PackageGraph] Allow package-level cyclic dependency only for >= 6.0 … #7579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ Note: This is in reverse chronological order, so newer entries are added to the

* [#7530]

Makes it possible for packages to depend on each other if such dependency doesn't form any target-level cycles. 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
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.
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
targets from `B` and vice versa.

Swift 6.0
Expand Down
37 changes: 31 additions & 6 deletions Sources/PackageGraph/ModulesGraph+Loading.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,16 @@ extension ModulesGraph {
)
}
}
let inputManifests = rootManifestNodes + rootDependencyNodes
let inputManifests = (rootManifestNodes + rootDependencyNodes).map {
KeyedPair($0, key: $0.id)
}

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

// Cycles in dependencies don't matter as long as there are no target cycles between packages.
depthFirstSearch(inputManifests.map { KeyedPair($0, key: $0.id) }) {
$0.item.requiredDependencies.compactMap { dependency in
manifestMap[dependency.identity].map { (manifest, fileSystem) in
let nodeSuccessorProvider = { (node: KeyedPair<GraphLoadingNode, PackageIdentity>) in
node.item.requiredDependencies.compactMap { dependency in
manifestMap[dependency.identity].map { manifest, _ in
KeyedPair(
GraphLoadingNode(
identity: dependency.identity,
Expand All @@ -82,7 +83,31 @@ extension ModulesGraph {
)
}
}
} onUnique: {
}

// Package dependency cycles feature is gated on tools version 6.0.
if !root.manifests.allSatisfy({ $1.toolsVersion >= .v6_0 }) {
if let cycle = findCycle(inputManifests, successors: nodeSuccessorProvider) {
let path = (cycle.path + cycle.cycle).map(\.item.manifest)
observabilityScope.emit(PackageGraphError.dependencyCycleDetected(
path: path, cycle: cycle.cycle[0].item.manifest
))

return try ModulesGraph(
rootPackages: [],
rootDependencies: [],
packages: IdentifiableSet(),
dependencies: requiredDependencies,
binaryArtifacts: binaryArtifacts
)
}
}

// Cycles in dependencies don't matter as long as there are no target cycles between packages.
depthFirstSearch(
inputManifests,
successors: nodeSuccessorProvider
) {
allNodes.append($0.item)
} onDuplicate: { _,_ in
// no de-duplication is required.
Expand Down
10 changes: 5 additions & 5 deletions Sources/PackageGraph/ModulesGraph.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ enum PackageGraphError: Swift.Error {
case noModules(Package)

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

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

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

case .productDependencyNotFound(let package, let targetName, let dependencyProductName, let dependencyPackageName, let dependencyProductInDecl, let similarProductName, let packageContainingSimilarProduct):
if dependencyProductInDecl {
Expand Down
129 changes: 127 additions & 2 deletions Tests/PackageGraphTests/ModulesGraphTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,10 @@ final class ModulesGraphTests: XCTestCase {
)

testDiagnostics(observability.diagnostics) { result in
result.check(diagnostic: "cyclic dependency declaration found: Foo -> Bar -> Baz -> Bar", severity: .error)
result.check(
diagnostic: "cyclic dependency between packages Foo -> Bar -> Baz -> Bar requires tools-version 6.0 or later",
severity: .error
)
}
}

Expand All @@ -212,11 +215,132 @@ final class ModulesGraphTests: XCTestCase {
)

testDiagnostics(observability.diagnostics) { result in
result.check(diagnostic: "cyclic dependency declaration found: Bar -> Foo -> Bar", severity: .error)
result.check(
diagnostic: "cyclic dependency declaration found: Bar -> Foo -> Bar",
severity: .error
)
}
}

func testDependencyCycleWithoutTargetCycleV5() throws {
let fs = InMemoryFileSystem(emptyFiles:
"/Foo/Sources/Foo/source.swift",
"/Bar/Sources/Bar/source.swift",
"/Bar/Sources/Baz/source.swift"
)

let observability = ObservabilitySystem.makeForTesting()
let _ = try loadModulesGraph(
fileSystem: fs,
manifests: [
Manifest.createRootManifest(
displayName: "Foo",
path: "/Foo",
toolsVersion: .v5_10,
dependencies: [
.localSourceControl(path: "/Bar", requirement: .upToNextMajor(from: "1.0.0"))
],
products: [
ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"])
],
targets: [
TargetDescription(name: "Foo", dependencies: ["Bar"]),
]),
Manifest.createFileSystemManifest(
displayName: "Bar",
path: "/Bar",
dependencies: [
.localSourceControl(path: "/Foo", requirement: .upToNextMajor(from: "1.0.0"))
],
products: [
ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar"]),
ProductDescription(name: "Baz", type: .library(.automatic), targets: ["Baz"])
],
targets: [
TargetDescription(name: "Bar"),
TargetDescription(name: "Baz", dependencies: ["Foo"]),
])
],
observabilityScope: observability.topScope
)

testDiagnostics(observability.diagnostics) { result in
result.check(
diagnostic: "cyclic dependency between packages Foo -> Bar -> Foo requires tools-version 6.0 or later",
severity: .error
)
}
}

func testDependencyCycleWithoutTargetCycle() throws {
let fs = InMemoryFileSystem(emptyFiles:
"/A/Sources/A/source.swift",
"/B/Sources/B/source.swift",
"/C/Sources/C/source.swift"
)

func testDependencyCycleDetection(rootToolsVersion: ToolsVersion) throws -> [Diagnostic] {
let observability = ObservabilitySystem.makeForTesting()
let _ = try loadModulesGraph(
fileSystem: fs,
manifests: [
Manifest.createRootManifest(
displayName: "A",
path: "/A",
toolsVersion: rootToolsVersion,
dependencies: [
.localSourceControl(path: "/B", requirement: .upToNextMajor(from: "1.0.0"))
],
products: [
ProductDescription(name: "A", type: .library(.automatic), targets: ["A"])
],
targets: [
TargetDescription(name: "A", dependencies: ["B"]),
]
),
Manifest.createFileSystemManifest(
displayName: "B",
path: "/B",
dependencies: [
.localSourceControl(path: "/C", requirement: .upToNextMajor(from: "1.0.0"))
],
products: [
ProductDescription(name: "B", type: .library(.automatic), targets: ["B"]),
],
targets: [
TargetDescription(name: "B"),
]
),
Manifest.createFileSystemManifest(
displayName: "C",
path: "/C",
dependencies: [
.localSourceControl(path: "/A", requirement: .upToNextMajor(from: "1.0.0"))
],
products: [
ProductDescription(name: "C", type: .library(.automatic), targets: ["C"]),
],
targets: [
TargetDescription(name: "C"),
]
)
],
observabilityScope: observability.topScope
)
return observability.diagnostics
}

try testDiagnostics(testDependencyCycleDetection(rootToolsVersion: .v5)) { result in
result.check(
diagnostic: "cyclic dependency between packages A -> B -> C -> A requires tools-version 6.0 or later",
severity: .error
)
}

try XCTAssertNoDiagnostics(testDependencyCycleDetection(rootToolsVersion: .v6_0))
}

func testDependencyCycleWithoutTargetCycleV6() throws {
let fs = InMemoryFileSystem(emptyFiles:
"/Foo/Sources/Foo/source.swift",
"/Bar/Sources/Bar/source.swift",
Expand All @@ -230,6 +354,7 @@ final class ModulesGraphTests: XCTestCase {
Manifest.createRootManifest(
displayName: "Foo",
path: "/Foo",
toolsVersion: .v6_0,
dependencies: [
.localSourceControl(path: "/Bar", requirement: .upToNextMajor(from: "1.0.0"))
],
Expand Down
10 changes: 3 additions & 7 deletions Tests/WorkspaceTests/WorkspaceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11061,7 +11061,7 @@ final class WorkspaceTests: XCTestCase {
requirement: .upToNextMajor(from: "1.0.0")
),
],
toolsVersion: .v5
toolsVersion: .v6_0
),
],
packages: [
Expand Down Expand Up @@ -11209,11 +11209,7 @@ final class WorkspaceTests: XCTestCase {
// FIXME: rdar://72940946
// we need to improve this situation or diagnostics when working on identity
result.check(
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.",
severity: .warning
)
result.check(
diagnostic: "product 'OtherUtilityProduct' required by package 'bar' target 'BarTarget' not found in package 'OtherUtilityPackage'.",
diagnostic: "cyclic dependency between packages Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage requires tools-version 6.0 or later",
severity: .error
)
}
Expand Down Expand Up @@ -11244,7 +11240,7 @@ final class WorkspaceTests: XCTestCase {
requirement: .upToNextMajor(from: "1.0.0")
),
],
toolsVersion: .v5
toolsVersion: .v6_0
),
],
packages: [
Expand Down