diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d321199221..823b4e423de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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] diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index f12604e4dad..a324b283e95 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -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) in + node.item.requiredDependencies.compactMap { dependency in + manifestMap[dependency.identity].map { manifest, _ in KeyedPair( GraphLoadingNode( identity: dependency.identity, @@ -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. diff --git a/Sources/PackageGraph/ModulesGraph.swift b/Sources/PackageGraph/ModulesGraph.swift index 9488171aabe..2af4f421e54 100644 --- a/Sources/PackageGraph/ModulesGraph.swift +++ b/Sources/PackageGraph/ModulesGraph.swift @@ -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?) @@ -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 { diff --git a/Tests/PackageGraphTests/ModulesGraphTests.swift b/Tests/PackageGraphTests/ModulesGraphTests.swift index 4a5b729adad..98e76341893 100644 --- a/Tests/PackageGraphTests/ModulesGraphTests.swift +++ b/Tests/PackageGraphTests/ModulesGraphTests.swift @@ -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 + ) } } @@ -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", @@ -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")) ], diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index 6e4bb85df05..57925ac86d9 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -11019,7 +11019,7 @@ final class WorkspaceTests: XCTestCase { requirement: .upToNextMajor(from: "1.0.0") ), ], - toolsVersion: .v5 + toolsVersion: .v6_0 ), ], packages: [ @@ -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 ) } @@ -11202,7 +11198,7 @@ final class WorkspaceTests: XCTestCase { requirement: .upToNextMajor(from: "1.0.0") ), ], - toolsVersion: .v5 + toolsVersion: .v6_0 ), ], packages: [