Skip to content

Commit

Permalink
[PackageGraph] Allow package-level cyclic dependency only for >= 6.0 …
Browse files Browse the repository at this point in the history 
…manifests

Follow-up to swiftlang#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.

(cherry picked from commit 3098b2d)
  • Loading branch information
@xedin
xedin committed May 31, 2024
1 parent aaeecec commit 41f3a4f
Show file tree
Hide file tree
Showing 5 changed files with 168 additions and 22 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ Swift 6.0

* [#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.

* [#7507]
Expand Down
37 changes: 31 additions & 6 deletions Sources/PackageGraph/ModulesGraph+Loading.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,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 @@ -83,7 +84,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
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,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(package: String, targetName: String, dependencyProductName: String, dependencyPackageName: String?, dependencyProductInDecl: Bool, similarProductName: String?, packageContainingSimilarProduct: String?)
Expand Down Expand Up @@ -226,10 +226,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
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,10 @@ final class ModulesGraphTests: XCTestCase {
)

testDiagnostics(observability.diagnostics) { result in
result.check(diagnostic: "cyclic dependency declaration found: 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 @@ -209,11 +212,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 @@ -227,6 +351,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
Original file line number Diff line number Diff line change
Expand Up @@ -11019,7 +11019,7 @@ final class WorkspaceTests: XCTestCase {
requirement: .upToNextMajor(from: "1.0.0")
),
],
toolsVersion: .v5
toolsVersion: .v6_0
),
],
packages: [
Expand Down Expand Up @@ -11167,11 +11167,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 @@ -11202,7 +11198,7 @@ final class WorkspaceTests: XCTestCase {
requirement: .upToNextMajor(from: "1.0.0")
),
],
toolsVersion: .v5
toolsVersion: .v6_0
),
],
packages: [
Expand Down

0 comments on commit 41f3a4f

Please sign in to comment.