From 3098b2d6988623801a83eae297885690ebd21b76 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Fri, 17 May 2024 20:28:36 -0700 Subject: [PATCH] [PackageGraph] Allow package-level cyclic dependency only for >= 6.0 manifests Follow-up to https://github.com/apple/swift-package-manager/pull/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. --- CHANGELOG.md | 4 +- .../PackageGraph/ModulesGraph+Loading.swift | 37 ++++- Sources/PackageGraph/ModulesGraph.swift | 10 +- .../PackageGraphTests/ModulesGraphTests.swift | 129 +++++++++++++++++- Tests/WorkspaceTests/WorkspaceTests.swift | 10 +- 5 files changed, 168 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 460f42544e4..335c5381d29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index e99aef7f964..085e118be2e 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -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) in + node.item.requiredDependencies.compactMap { dependency in + manifestMap[dependency.identity].map { manifest, _ in KeyedPair( GraphLoadingNode( identity: dependency.identity, @@ -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. diff --git a/Sources/PackageGraph/ModulesGraph.swift b/Sources/PackageGraph/ModulesGraph.swift index ae877b52d98..d65e767e128 100644 --- a/Sources/PackageGraph/ModulesGraph.swift +++ b/Sources/PackageGraph/ModulesGraph.swift @@ -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( @@ -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 { diff --git a/Tests/PackageGraphTests/ModulesGraphTests.swift b/Tests/PackageGraphTests/ModulesGraphTests.swift index e321eba8cf1..d86c5ef2bca 100644 --- a/Tests/PackageGraphTests/ModulesGraphTests.swift +++ b/Tests/PackageGraphTests/ModulesGraphTests.swift @@ -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 + ) } } @@ -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", @@ -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")) ], diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index 66142b65b4b..bbbcd8943af 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -11061,7 +11061,7 @@ final class WorkspaceTests: XCTestCase { requirement: .upToNextMajor(from: "1.0.0") ), ], - toolsVersion: .v5 + toolsVersion: .v6_0 ), ], packages: [ @@ -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 ) } @@ -11244,7 +11240,7 @@ final class WorkspaceTests: XCTestCase { requirement: .upToNextMajor(from: "1.0.0") ), ], - toolsVersion: .v5 + toolsVersion: .v6_0 ), ], packages: [