From 550650baa325c478eebeece0dcfe395776a08bca Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 8 May 2024 11:53:04 -0700 Subject: [PATCH] [Package/ModuleGraph] Allow cyclic package dependencies if they don't introduce a cycle in a build graph (#7530) It should be possible for packages to depend on each other if such dependence doesn't introduce cycles in the build graph. - Introduced a new DFS method to walk over graphs that breaks cycles. - Replaces use of `findCycle` + `topologicalSort` with `DFS` while building manifest and package graphs. This allows cycles in dependencies to be modeled correctly. - Removes some of the redundant data transformations from modules graph. - Modifies `ResolvedPackage` to carry identities of its dependencies instead of resolved dependencies themselves. This helps to simplify logic in `createResolvedPackages`. - Adds detection of target cycles across packages. Makes it possible for package A to depend on package B and B to depend on A if their targets don't form a cycle. (cherry picked from commit 8b129093e02bb30c2dd9d3af5187435aba25f53e) --- CHANGELOG.md | 6 + Sources/Basics/CMakeLists.txt | 1 + Sources/Basics/Graph/GraphAlgorithms.swift | 57 +++++++ .../PackageCommands/CompletionCommand.swift | 1 + .../PackageCommands/PluginCommand.swift | 16 +- .../PackageCommands/ShowDependencies.swift | 10 +- .../Utilities/DependenciesSerializer.swift | 22 +-- .../PackageGraph/ModulesGraph+Loading.swift | 158 +++++++----------- Sources/PackageGraph/ModulesGraph.swift | 39 ++--- .../Resolution/ResolvedPackage.swift | 4 +- .../Plugins/PluginContextSerializer.swift | 3 +- .../Plugins/PluginInvocation.swift | 5 + Sources/Workspace/Workspace+Manifests.swift | 53 +++--- Sources/Workspace/Workspace+Signing.swift | 4 +- Sources/Workspace/Workspace.swift | 2 +- .../MermaidPackageSerializerTests.swift | 4 +- Tests/CommandsTests/PackageCommandTests.swift | 1 + Tests/FunctionalTests/PluginTests.swift | 2 + .../PackageGraphTests/ModulesGraphTests.swift | 65 +++++-- Tests/WorkspaceTests/WorkspaceTests.swift | 24 ++- 20 files changed, 277 insertions(+), 200 deletions(-) create mode 100644 Sources/Basics/Graph/GraphAlgorithms.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 954af3d106d..0d321199221 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,12 @@ Note: This is in reverse chronological order, so newer entries are added to the 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 + targets from `B` and vice versa. + * [#7507] `swift experimental-sdk` command is deprecated with `swift sdk` command replacing it. `--experimental-swift-sdk` and diff --git a/Sources/Basics/CMakeLists.txt b/Sources/Basics/CMakeLists.txt index bed9dcebfd2..caf29d8d1c5 100644 --- a/Sources/Basics/CMakeLists.txt +++ b/Sources/Basics/CMakeLists.txt @@ -34,6 +34,7 @@ add_library(Basics FileSystem/TemporaryFile.swift FileSystem/TSCAdapters.swift FileSystem/VFSOverlay.swift + Graph/GraphAlgorithms.swift SourceControlURL.swift HTTPClient/HTTPClient.swift HTTPClient/HTTPClientConfiguration.swift diff --git a/Sources/Basics/Graph/GraphAlgorithms.swift b/Sources/Basics/Graph/GraphAlgorithms.swift new file mode 100644 index 00000000000..6f7d98970a8 --- /dev/null +++ b/Sources/Basics/Graph/GraphAlgorithms.swift @@ -0,0 +1,57 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift open source project +// +// Copyright (c) 2015-2024 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See http://swift.org/LICENSE.txt for license information +// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import struct OrderedCollections.OrderedSet + +/// Implements a pre-order depth-first search. +/// +/// The cycles are handled by skipping cycle points but it should be possible to +/// to extend this in the future to provide a callback for every cycle. +/// +/// - Parameters: +/// - nodes: The list of input nodes to sort. +/// - successors: A closure for fetching the successors of a particular node. +/// - onUnique: A callback to indicate the the given node is being processed for the first time. +/// - onDuplicate: A callback to indicate that the node was already processed at least once. +/// +/// - Complexity: O(v + e) where (v, e) are the number of vertices and edges +/// reachable from the input nodes via the relation. +public func depthFirstSearch( + _ nodes: [T], + successors: (T) throws -> [T], + onUnique: (T) -> Void, + onDuplicate: (T, T) -> Void +) rethrows { + var stack = OrderedSet() + var visited = Set() + + for node in nodes { + precondition(stack.isEmpty) + stack.append(node) + + while !stack.isEmpty { + let curr = stack.removeLast() + + let visitResult = visited.insert(curr) + if visitResult.inserted { + onUnique(curr) + } else { + onDuplicate(visitResult.memberAfterInsert, curr) + continue + } + + for succ in try successors(curr) { + stack.append(succ) + } + } + } +} diff --git a/Sources/Commands/PackageCommands/CompletionCommand.swift b/Sources/Commands/PackageCommands/CompletionCommand.swift index 89c4da084ef..0c86b77fd82 100644 --- a/Sources/Commands/PackageCommands/CompletionCommand.swift +++ b/Sources/Commands/PackageCommands/CompletionCommand.swift @@ -68,6 +68,7 @@ extension SwiftPackageCommand { // command's result output goes on stdout // ie "swift package list-dependencies" should output to stdout ShowDependencies.dumpDependenciesOf( + graph: graph, rootPackage: graph.rootPackages[graph.rootPackages.startIndex], mode: .flatlist, on: TSCBasic.stdoutStream diff --git a/Sources/Commands/PackageCommands/PluginCommand.swift b/Sources/Commands/PackageCommands/PluginCommand.swift index 2fac7c1498d..6f7c95bce4c 100644 --- a/Sources/Commands/PackageCommands/PluginCommand.swift +++ b/Sources/Commands/PackageCommands/PluginCommand.swift @@ -360,6 +360,7 @@ struct PluginCommand: SwiftCommand { pkgConfigDirectories: swiftCommandState.options.locations.pkgConfigDirectories, sdkRootPath: buildParameters.toolchain.sdkRootPath, fileSystem: swiftCommandState.fileSystem, + modulesGraph: packageGraph, observabilityScope: swiftCommandState.observabilityScope, callbackQueue: delegateQueue, delegate: pluginDelegate, @@ -371,10 +372,17 @@ struct PluginCommand: SwiftCommand { static func availableCommandPlugins(in graph: ModulesGraph, limitedTo packageIdentity: String?) -> [PluginTarget] { // All targets from plugin products of direct dependencies are "available". - let directDependencyPackages = graph.rootPackages.flatMap { $0.dependencies }.filter { $0.matching(identity: packageIdentity) } + let directDependencyPackages = graph.rootPackages.flatMap { + $0.dependencies + }.filter { + $0.matching(identity: packageIdentity) + }.compactMap { + graph.package(for: $0) + } + let directDependencyPluginTargets = directDependencyPackages.flatMap { $0.products.filter { $0.type == .plugin } }.flatMap { $0.targets } // As well as any plugin targets in root packages. - let rootPackageTargets = graph.rootPackages.filter { $0.matching(identity: packageIdentity) }.flatMap { $0.targets } + let rootPackageTargets = graph.rootPackages.filter { $0.identity.matching(identity: packageIdentity) }.flatMap { $0.targets } return (directDependencyPluginTargets + rootPackageTargets).compactMap { $0.underlying as? PluginTarget }.filter { switch $0.capability { case .buildTool: return false @@ -458,10 +466,10 @@ extension SandboxNetworkPermission { } } -extension ResolvedPackage { +extension PackageIdentity { fileprivate func matching(identity: String?) -> Bool { if let identity { - return self.identity == .plain(identity) + return self == .plain(identity) } else { return true } diff --git a/Sources/Commands/PackageCommands/ShowDependencies.swift b/Sources/Commands/PackageCommands/ShowDependencies.swift index 1572c6e0f63..405e95e0eaa 100644 --- a/Sources/Commands/PackageCommands/ShowDependencies.swift +++ b/Sources/Commands/PackageCommands/ShowDependencies.swift @@ -41,13 +41,19 @@ extension SwiftPackageCommand { // ie "swift package show-dependencies" should output to stdout let stream: OutputByteStream = try outputPath.map { try LocalFileOutputByteStream($0) } ?? TSCBasic.stdoutStream Self.dumpDependenciesOf( + graph: graph, rootPackage: graph.rootPackages[graph.rootPackages.startIndex], mode: format, on: stream ) } - static func dumpDependenciesOf(rootPackage: ResolvedPackage, mode: ShowDependenciesMode, on stream: OutputByteStream) { + static func dumpDependenciesOf( + graph: ModulesGraph, + rootPackage: ResolvedPackage, + mode: ShowDependenciesMode, + on stream: OutputByteStream + ) { let dumper: DependenciesDumper switch mode { case .text: @@ -59,7 +65,7 @@ extension SwiftPackageCommand { case .flatlist: dumper = FlatListDumper() } - dumper.dump(dependenciesOf: rootPackage, on: stream) + dumper.dump(graph: graph, dependenciesOf: rootPackage, on: stream) stream.flush() } diff --git a/Sources/Commands/Utilities/DependenciesSerializer.swift b/Sources/Commands/Utilities/DependenciesSerializer.swift index 3a036bec15f..25190f011c4 100644 --- a/Sources/Commands/Utilities/DependenciesSerializer.swift +++ b/Sources/Commands/Utilities/DependenciesSerializer.swift @@ -17,11 +17,11 @@ import enum TSCBasic.JSON import protocol TSCBasic.OutputByteStream protocol DependenciesDumper { - func dump(dependenciesOf: ResolvedPackage, on: OutputByteStream) + func dump(graph: ModulesGraph, dependenciesOf: ResolvedPackage, on: OutputByteStream) } final class PlainTextDumper: DependenciesDumper { - func dump(dependenciesOf rootpkg: ResolvedPackage, on stream: OutputByteStream) { + func dump(graph: ModulesGraph, dependenciesOf rootpkg: ResolvedPackage, on stream: OutputByteStream) { func recursiveWalk(packages: [ResolvedPackage], prefix: String = "") { var hanger = prefix + "├── " @@ -39,14 +39,14 @@ final class PlainTextDumper: DependenciesDumper { var childPrefix = hanger let startIndex = childPrefix.index(childPrefix.endIndex, offsetBy: -4) childPrefix.replaceSubrange(startIndex.. = [] func printNode(_ package: ResolvedPackage) { let url = package.manifest.packageLocation @@ -87,7 +87,7 @@ final class DotDumper: DependenciesDumper { var dependenciesAlreadyPrinted: Set = [] func recursiveWalk(rootpkg: ResolvedPackage) { printNode(rootpkg) - for dependency in rootpkg.dependencies { + for dependency in graph.directDependencies(for: rootpkg) { let rootURL = rootpkg.manifest.packageLocation let dependencyURL = dependency.manifest.packageLocation let urlPair = DependencyURLs(root: rootURL, dependency: dependencyURL) @@ -120,7 +120,7 @@ final class DotDumper: DependenciesDumper { } final class JSONDumper: DependenciesDumper { - func dump(dependenciesOf rootpkg: ResolvedPackage, on stream: OutputByteStream) { + func dump(graph: ModulesGraph, dependenciesOf rootpkg: ResolvedPackage, on stream: OutputByteStream) { func convert(_ package: ResolvedPackage) -> JSON { return .orderedDictionary([ "identity": .string(package.identity.description), @@ -128,7 +128,7 @@ final class JSONDumper: DependenciesDumper { "url": .string(package.manifest.packageLocation), "version": .string(package.manifest.version?.description ?? "unspecified"), "path": .string(package.path.pathString), - "dependencies": .array(package.dependencies.map(convert)), + "dependencies": .array(package.dependencies.compactMap { graph.packages[$0] }.map(convert)), ]) } diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index 716ff676695..f12604e4dad 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -15,7 +15,9 @@ import OrderedCollections import PackageLoading import PackageModel +import struct TSCBasic.KeyedPair import func TSCBasic.bestMatch +import func TSCBasic.findCycle extension ModulesGraph { /// Load the package graph for the given package path. @@ -45,17 +47,6 @@ extension ModulesGraph { root.manifests.forEach { manifestMap[$0.key] = ($0.value, fileSystem) } - func nodeSuccessorsProvider(node: GraphLoadingNode) -> [GraphLoadingNode] { - node.requiredDependencies.compactMap { dependency in - manifestMap[dependency.identity].map { (manifest, fileSystem) in - GraphLoadingNode( - identity: dependency.identity, - manifest: manifest, - productFilter: dependency.productFilter - ) - } - } - } // Construct the root root dependencies set. let rootDependencies = Set(root.dependencies.compactMap{ @@ -76,33 +67,29 @@ extension ModulesGraph { let inputManifests = rootManifestNodes + rootDependencyNodes // Collect the manifests for which we are going to build packages. - var allNodes: [GraphLoadingNode] + var allNodes = [GraphLoadingNode]() - // Detect cycles in manifest dependencies. - if let cycle = findCycle(inputManifests, successors: nodeSuccessorsProvider) { - observabilityScope.emit(PackageGraphError.cycleDetected(cycle)) - // Break the cycle so we can build a partial package graph. - allNodes = inputManifests.filter({ $0.manifest != cycle.cycle[0] }) - } else { - // Sort all manifests topologically. - allNodes = try topologicalSort(inputManifests, successors: nodeSuccessorsProvider) - } - - var flattenedManifests: [PackageIdentity: GraphLoadingNode] = [:] - for node in allNodes { - if let existing = flattenedManifests[node.identity] { - let merged = GraphLoadingNode( - identity: node.identity, - manifest: node.manifest, - productFilter: existing.productFilter.union(node.productFilter) - ) - flattenedManifests[node.identity] = merged - } else { - flattenedManifests[node.identity] = node + // 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 + KeyedPair( + GraphLoadingNode( + identity: dependency.identity, + manifest: manifest, + productFilter: dependency.productFilter + ), + key: dependency.identity + ) + } } + } onUnique: { + allNodes.append($0.item) + } onDuplicate: { _,_ in + // no de-duplication is required. } // sort by identity - allNodes = flattenedManifests.keys.sorted().map { flattenedManifests[$0]! } // force unwrap fine since we are iterating on keys + allNodes = allNodes.sorted { $0.id < $1.id } // Create the packages. var manifestToPackage: [Manifest: Package] = [:] @@ -165,19 +152,24 @@ extension ModulesGraph { observabilityScope: observabilityScope ) - let rootPackages = resolvedPackages.filter{ root.manifests.values.contains($0.manifest) } - checkAllDependenciesAreUsed(rootPackages, observabilityScope: observabilityScope) + let rootPackages = resolvedPackages.filter { root.manifests.values.contains($0.manifest) } + checkAllDependenciesAreUsed(packages: resolvedPackages, rootPackages, observabilityScope: observabilityScope) return try ModulesGraph( rootPackages: rootPackages, - rootDependencies: resolvedPackages.filter{ rootDependencies.contains($0.manifest) }, + rootDependencies: resolvedPackages.filter { rootDependencies.contains($0.manifest) }, + packages: resolvedPackages, dependencies: requiredDependencies, binaryArtifacts: binaryArtifacts ) } } -private func checkAllDependenciesAreUsed(_ rootPackages: [ResolvedPackage], observabilityScope: ObservabilityScope) { +private func checkAllDependenciesAreUsed( + packages: IdentifiableSet, + _ rootPackages: [ResolvedPackage], + observabilityScope: ObservabilityScope +) { for package in rootPackages { // List all dependency products dependent on by the package targets. let productDependencies = IdentifiableSet(package.targets.flatMap({ target in @@ -191,7 +183,12 @@ private func checkAllDependenciesAreUsed(_ rootPackages: [ResolvedPackage], obse }) })) - for dependency in package.dependencies { + for dependencyId in package.dependencies { + guard let dependency = packages[dependencyId] else { + observabilityScope.emit(.error("Unknown package: \(dependencyId)")) + return + } + // We continue if the dependency contains executable products to make sure we don't // warn on a valid use-case for a lone dependency: swift run dependency executables. guard !dependency.products.contains(where: { $0.type == .executable }) else { @@ -248,7 +245,7 @@ private func createResolvedPackages( availableLibraries: [LibraryMetadata], fileSystem: FileSystem, observabilityScope: ObservabilityScope -) throws -> [ResolvedPackage] { +) throws -> IdentifiableSet { // Create package builder objects from the input manifests. let packageBuilders: [ResolvedPackageBuilder] = nodes.compactMap{ node in @@ -645,7 +642,32 @@ private func createResolvedPackages( } } - return try packageBuilders.map{ try $0.construct() } + do { + let targetBuilders = packageBuilders.flatMap { + $0.targets.map { + KeyedPair($0, key: $0.target) + } + } + if let cycle = findCycle(targetBuilders, successors: { + $0.item.dependencies.flatMap { + switch $0 { + case .product(let productBuilder, conditions: _): + return productBuilder.targets.map { KeyedPair($0, key: $0.target) } + case .target: + return [] // local targets were checked by PackageBuilder. + } + } + }) { + observabilityScope.emit( + ModuleError.cycleDetected( + (cycle.path.map(\.key.name), cycle.cycle.map(\.key.name)) + ) + ) + return IdentifiableSet() + } + } + + return IdentifiableSet(try packageBuilders.map { try $0.construct() }) } private func emitDuplicateProductDiagnostic( @@ -1046,7 +1068,7 @@ private final class ResolvedPackageBuilder: ResolvedBuilder { underlying: self.package, defaultLocalization: self.defaultLocalization, supportedPlatforms: self.supportedPlatforms, - dependencies: try self.dependencies.map{ try $0.construct() }, + dependencies: self.dependencies.map { $0.package.identity }, targets: try self.targets.map{ try $0.construct() }, products: try self.products.map{ try $0.construct() }, registryMetadata: self.registryMetadata, @@ -1054,55 +1076,3 @@ private final class ResolvedPackageBuilder: ResolvedBuilder { ) } } - -/// Finds the first cycle encountered in a graph. -/// -/// This is different from the one in tools support core, in that it handles equality separately from node traversal. Nodes traverse product filters, but only the manifests must be equal for there to be a cycle. -fileprivate func findCycle( - _ nodes: [GraphLoadingNode], - successors: (GraphLoadingNode) throws -> [GraphLoadingNode] -) rethrows -> (path: [Manifest], cycle: [Manifest])? { - // Ordered set to hold the current traversed path. - var path = OrderedCollections.OrderedSet() - - var fullyVisitedManifests = Set() - - // Function to visit nodes recursively. - // FIXME: Convert to stack. - func visit( - _ node: GraphLoadingNode, - _ successors: (GraphLoadingNode) throws -> [GraphLoadingNode] - ) rethrows -> (path: [Manifest], cycle: [Manifest])? { - // Once all successors have been visited, this node cannot participate - // in a cycle. - if fullyVisitedManifests.contains(node.manifest) { - return nil - } - - // If this node is already in the current path then we have found a cycle. - if !path.append(node.manifest).inserted { - let index = path.firstIndex(of: node.manifest)! // forced unwrap safe - return (Array(path[path.startIndex.. - /// The complete list of contained packages, in topological order starting - /// with the root packages. - public let packages: [ResolvedPackage] + /// The complete set of contained packages. + public let packages: IdentifiableSet /// The list of all targets reachable from root targets. public let reachableTargets: IdentifiableSet @@ -116,17 +115,24 @@ public struct ModulesGraph { return self.rootPackages.contains(id: package.id) } - private let modulesToPackages: [ResolvedModule.ID: ResolvedPackage] + /// Returns the package based on the given identity, or nil if the package isn't in the graph. + public func package(for identity: PackageIdentity) -> ResolvedPackage? { + packages[identity] + } + /// Returns the package that contains the module, or nil if the module isn't in the graph. public func package(for module: ResolvedModule) -> ResolvedPackage? { - return self.modulesToPackages[module.id] + self.package(for: module.packageIdentity) } - - private let productsToPackages: [ResolvedProduct.ID: ResolvedPackage] /// Returns the package that contains the product, or nil if the product isn't in the graph. public func package(for product: ResolvedProduct) -> ResolvedPackage? { - return self.productsToPackages[product.id] + self.package(for: product.packageIdentity) + } + + /// Returns all of the packages that the given package depends on directly. + public func directDependencies(for package: ResolvedPackage) -> [ResolvedPackage] { + package.dependencies.compactMap { self.package(for: $0) } } /// All root and root dependency packages provided as input to the graph. @@ -139,6 +145,7 @@ public struct ModulesGraph { public init( rootPackages: [ResolvedPackage], rootDependencies: [ResolvedPackage] = [], + packages: IdentifiableSet, dependencies requiredDependencies: [PackageReference], binaryArtifacts: [PackageIdentity: [String: BinaryArtifact]] ) throws { @@ -146,14 +153,7 @@ public struct ModulesGraph { self.requiredDependencies = requiredDependencies self.inputPackages = rootPackages + rootDependencies self.binaryArtifacts = binaryArtifacts - self.packages = try topologicalSort(inputPackages, successors: { $0.dependencies }) - - // Create a mapping from targets to the packages that define them. Here - // we include all targets, including tests in non-root packages, since - // this is intended for lookup and not traversal. - self.modulesToPackages = packages.reduce(into: [:], { partial, package in - package.targets.forEach{ partial[$0.id] = package } - }) + self.packages = packages let allTargets = IdentifiableSet(packages.flatMap({ package -> [ResolvedModule] in if rootPackages.contains(id: package.id) { @@ -165,13 +165,6 @@ public struct ModulesGraph { } })) - // Create a mapping from products to the packages that define them. Here - // we include all products, including tests in non-root packages, since - // this is intended for lookup and not traversal. - self.productsToPackages = packages.reduce(into: [:], { partial, package in - package.products.forEach { partial[$0.id] = package } - }) - let allProducts = IdentifiableSet(packages.flatMap({ package -> [ResolvedProduct] in if rootPackages.contains(id: package.id) { return package.products diff --git a/Sources/PackageGraph/Resolution/ResolvedPackage.swift b/Sources/PackageGraph/Resolution/ResolvedPackage.swift index f1e8f93fe26..2c3af59896c 100644 --- a/Sources/PackageGraph/Resolution/ResolvedPackage.swift +++ b/Sources/PackageGraph/Resolution/ResolvedPackage.swift @@ -40,7 +40,7 @@ public struct ResolvedPackage { public let products: [ResolvedProduct] /// The dependencies of the package. - public let dependencies: [ResolvedPackage] + public let dependencies: [PackageIdentity] /// The default localization for resources. public let defaultLocalization: String? @@ -57,7 +57,7 @@ public struct ResolvedPackage { underlying: Package, defaultLocalization: String?, supportedPlatforms: [SupportedPlatform], - dependencies: [ResolvedPackage], + dependencies: [PackageIdentity], targets: [ResolvedModule], products: [ResolvedProduct], registryMetadata: RegistryReleaseMetadata?, diff --git a/Sources/SPMBuildCore/Plugins/PluginContextSerializer.swift b/Sources/SPMBuildCore/Plugins/PluginContextSerializer.swift index 4352b35ffac..2c8562f6f4a 100644 --- a/Sources/SPMBuildCore/Plugins/PluginContextSerializer.swift +++ b/Sources/SPMBuildCore/Plugins/PluginContextSerializer.swift @@ -22,6 +22,7 @@ typealias WireInput = HostToPluginMessage.InputContext /// the input information to a plugin. internal struct PluginContextSerializer { let fileSystem: FileSystem + let modulesGraph: ModulesGraph let buildEnvironment: BuildEnvironment let pkgConfigDirectories: [AbsolutePath] let sdkRootPath: AbsolutePath? @@ -244,7 +245,7 @@ internal struct PluginContextSerializer { } // Serialize the dependencies. It is important to do this before the `let id = package.count` below so the correct wire ID gets assigned. - let dependencies = try package.dependencies.map { + let dependencies = try modulesGraph.directDependencies(for: package).map { WireInput.Package.Dependency(packageId: try serialize(package: $0)) } diff --git a/Sources/SPMBuildCore/Plugins/PluginInvocation.swift b/Sources/SPMBuildCore/Plugins/PluginInvocation.swift index 9fb03674540..235b89a116c 100644 --- a/Sources/SPMBuildCore/Plugins/PluginInvocation.swift +++ b/Sources/SPMBuildCore/Plugins/PluginInvocation.swift @@ -43,6 +43,7 @@ extension PluginTarget { pkgConfigDirectories: [AbsolutePath], sdkRootPath: AbsolutePath?, fileSystem: FileSystem, + modulesGraph: ModulesGraph, observabilityScope: ObservabilityScope, callbackQueue: DispatchQueue, delegate: PluginInvocationDelegate @@ -62,6 +63,7 @@ extension PluginTarget { pkgConfigDirectories: pkgConfigDirectories, sdkRootPath: sdkRootPath, fileSystem: fileSystem, + modulesGraph: modulesGraph, observabilityScope: observabilityScope, callbackQueue: callbackQueue, delegate: delegate, @@ -107,6 +109,7 @@ extension PluginTarget { pkgConfigDirectories: [AbsolutePath], sdkRootPath: AbsolutePath?, fileSystem: FileSystem, + modulesGraph: ModulesGraph, observabilityScope: ObservabilityScope, callbackQueue: DispatchQueue, delegate: PluginInvocationDelegate, @@ -125,6 +128,7 @@ extension PluginTarget { do { var serializer = PluginContextSerializer( fileSystem: fileSystem, + modulesGraph: modulesGraph, buildEnvironment: buildEnvironment, pkgConfigDirectories: pkgConfigDirectories, sdkRootPath: sdkRootPath @@ -571,6 +575,7 @@ extension ModulesGraph { pkgConfigDirectories: pkgConfigDirectories, sdkRootPath: buildParameters.toolchain.sdkRootPath, fileSystem: fileSystem, + modulesGraph: self, observabilityScope: observabilityScope, callbackQueue: delegateQueue, delegate: delegate, diff --git a/Sources/Workspace/Workspace+Manifests.swift b/Sources/Workspace/Workspace+Manifests.swift index 64646669616..5d4513adb32 100644 --- a/Sources/Workspace/Workspace+Manifests.swift +++ b/Sources/Workspace/Workspace+Manifests.swift @@ -16,6 +16,7 @@ import struct Basics.InternalError import class Basics.ObservabilityScope import struct Basics.SwiftVersion import func Basics.temp_await +import func Basics.depthFirstSearch import class Basics.ThreadSafeKeyValueStore import class Dispatch.DispatchGroup import struct Dispatch.DispatchTime @@ -510,12 +511,7 @@ extension Workspace { // Continue to load the rest of the manifest for this graph // Creates a map of loaded manifests. We do this to avoid reloading the shared nodes. var loadedManifests = firstLevelManifests - // Compute the transitive closure of available dependencies. - let topologicalSortInput = topLevelManifests.map { identity, manifest in KeyedPair( - manifest, - key: Key(identity: identity, productFilter: .everything) - ) } - let topologicalSortSuccessors: (KeyedPair) throws -> [KeyedPair] = { pair in + let successorManifests: (KeyedPair) throws -> [KeyedPair] = { pair in // optimization: preload manifest we know about in parallel let dependenciesRequired = pair.item.dependenciesRequired(for: pair.key.productFilter) let dependenciesToLoad = dependenciesRequired.map(\.packageRef) @@ -543,37 +539,26 @@ extension Workspace { } } - // Look for any cycle in the dependencies. - if let cycle = try findCycle(topologicalSortInput, successors: topologicalSortSuccessors) { - observabilityScope.emit( - error: "cyclic dependency declaration found: " + - (cycle.path + cycle.cycle).map(\.key.identity.description).joined(separator: " -> ") + - " -> " + cycle.cycle[0].key.identity.description - ) - // return partial results - return DependencyManifests( - root: root, - dependencies: [], - workspace: self, - availableLibraries: availableLibraries, - observabilityScope: observabilityScope - ) - } - let allManifestsWithPossibleDuplicates = try topologicalSort( - topologicalSortInput, - successors: topologicalSortSuccessors - ) - - // merge the productFilter of the same package (by identity) - var deduplication = [PackageIdentity: Int]() var allManifests = [(identity: PackageIdentity, manifest: Manifest, productFilter: ProductFilter)]() - for pair in allManifestsWithPossibleDuplicates { - if let index = deduplication[pair.key.identity] { - let productFilter = allManifests[index].productFilter.merge(pair.key.productFilter) - allManifests[index] = (pair.key.identity, pair.item, productFilter) - } else { + do { + let manifestGraphRoots = topLevelManifests.map { identity, manifest in + KeyedPair( + manifest, + key: Key(identity: identity, productFilter: .everything) + ) + } + + var deduplication = [PackageIdentity: Int]() + try depthFirstSearch( + manifestGraphRoots, + successors: successorManifests + ) { pair in deduplication[pair.key.identity] = allManifests.count allManifests.append((pair.key.identity, pair.item, pair.key.productFilter)) + } onDuplicate: { old, new in + let index = deduplication[old.key.identity]! + let productFilter = allManifests[index].productFilter.merge(new.key.productFilter) + allManifests[index] = (new.key.identity, new.item, productFilter) } } diff --git a/Sources/Workspace/Workspace+Signing.swift b/Sources/Workspace/Workspace+Signing.swift index dd13d0606b4..1db01de2dd5 100644 --- a/Sources/Workspace/Workspace+Signing.swift +++ b/Sources/Workspace/Workspace+Signing.swift @@ -46,7 +46,7 @@ extension Workspace { expectedSigningEntities: [PackageIdentity: RegistryReleaseMetadata.SigningEntity] ) throws { try expectedSigningEntities.forEach { identity, expectedSigningEntity in - if let package = packageGraph.packages.first(where: { $0.identity == identity }) { + if let package = packageGraph.package(for: identity) { guard let actualSigningEntity = package.registryMetadata?.signature?.signedBy else { throw SigningError.unsigned(package: identity, expected: expectedSigningEntity) } @@ -68,7 +68,7 @@ extension Workspace { expected: expectedSigningEntity ) } - guard let package = packageGraph.packages.first(where: { $0.identity == mirroredIdentity }) else { + guard let package = packageGraph.package(for: mirroredIdentity) else { // Unsure if this case is reachable in practice. throw SigningError.expectedIdentityNotFound(package: identity) } diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift index e6016e3d554..75a205d769b 100644 --- a/Sources/Workspace/Workspace.swift +++ b/Sources/Workspace/Workspace.swift @@ -1158,7 +1158,7 @@ extension Workspace { observabilityScope: ObservabilityScope, completion: @escaping (Result) -> Void ) { - guard let previousPackage = packageGraph.packages.first(where: { $0.identity == identity }) else { + guard let previousPackage = packageGraph.package(for: identity) else { return completion(.failure(StringError("could not find package with identity \(identity)"))) } diff --git a/Tests/CommandsTests/MermaidPackageSerializerTests.swift b/Tests/CommandsTests/MermaidPackageSerializerTests.swift index 7427267895f..2fd68f8e4c2 100644 --- a/Tests/CommandsTests/MermaidPackageSerializerTests.swift +++ b/Tests/CommandsTests/MermaidPackageSerializerTests.swift @@ -107,7 +107,7 @@ final class MermaidPackageSerializerTests: XCTestCase { XCTAssertNoDiagnostics(observability.diagnostics) XCTAssertEqual(graph.packages.count, 2) - let package = try XCTUnwrap(graph.packages.first) + let package = try XCTUnwrap(graph.package(for: .plain("A"))) let serializer = MermaidPackageSerializer(package: package.underlying) XCTAssertEqual( serializer.renderedMarkdown, @@ -166,7 +166,7 @@ final class MermaidPackageSerializerTests: XCTestCase { XCTAssertNoDiagnostics(observability.diagnostics) XCTAssertEqual(graph.packages.count, 2) - let package = try XCTUnwrap(graph.packages.first) + let package = try XCTUnwrap(graph.package(for: .plain("A"))) let serializer = MermaidPackageSerializer(package: package.underlying) XCTAssertEqual( serializer.renderedMarkdown, diff --git a/Tests/CommandsTests/PackageCommandTests.swift b/Tests/CommandsTests/PackageCommandTests.swift index b5c8c3e2c7f..f6d288a28fd 100644 --- a/Tests/CommandsTests/PackageCommandTests.swift +++ b/Tests/CommandsTests/PackageCommandTests.swift @@ -644,6 +644,7 @@ final class PackageCommandTests: CommandsTestCase { let output = BufferedOutputByteStream() SwiftPackageCommand.ShowDependencies.dumpDependenciesOf( + graph: graph, rootPackage: graph.rootPackages[graph.rootPackages.startIndex], mode: .dot, on: output diff --git a/Tests/FunctionalTests/PluginTests.swift b/Tests/FunctionalTests/PluginTests.swift index 77ea7072ae2..7abb495a067 100644 --- a/Tests/FunctionalTests/PluginTests.swift +++ b/Tests/FunctionalTests/PluginTests.swift @@ -550,6 +550,7 @@ class PluginTests: XCTestCase { pkgConfigDirectories: [], sdkRootPath: nil, fileSystem: localFileSystem, + modulesGraph: packageGraph, observabilityScope: observability.topScope, callbackQueue: delegateQueue, delegate: delegate, @@ -829,6 +830,7 @@ class PluginTests: XCTestCase { pkgConfigDirectories: [], sdkRootPath: try UserToolchain.default.sdkRootPath, fileSystem: localFileSystem, + modulesGraph: packageGraph, observabilityScope: observability.topScope, callbackQueue: delegateQueue, delegate: delegate diff --git a/Tests/PackageGraphTests/ModulesGraphTests.swift b/Tests/PackageGraphTests/ModulesGraphTests.swift index d03795ba91b..4a5b729adad 100644 --- a/Tests/PackageGraphTests/ModulesGraphTests.swift +++ b/Tests/PackageGraphTests/ModulesGraphTests.swift @@ -80,12 +80,12 @@ final class ModulesGraphTests: XCTestCase { result.checkTarget("Baz") { result in result.check(dependencies: "Bar") } } - let fooPackage = try XCTUnwrap(g.packages.first{ $0.identity == .plain("Foo") }) + let fooPackage = try XCTUnwrap(g.package(for: .plain("Foo"))) let fooTarget = try XCTUnwrap(g.allTargets.first{ $0.name == "Foo" }) let fooDepTarget = try XCTUnwrap(g.allTargets.first{ $0.name == "FooDep" }) XCTAssertEqual(g.package(for: fooTarget)?.id, fooPackage.id) XCTAssertEqual(g.package(for: fooDepTarget)?.id, fooPackage.id) - let barPackage = try XCTUnwrap(g.packages.first{ $0.identity == .plain("Bar") }) + let barPackage = try XCTUnwrap(g.package(for: .plain("Bar"))) let barTarget = try XCTUnwrap(g.allTargets.first{ $0.name == "Bar" }) XCTAssertEqual(g.package(for: barTarget)?.id, barPackage.id) } @@ -183,36 +183,81 @@ 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 declaration found: Bar -> Baz -> Bar", severity: .error) } } - func testCycle2() throws { + func testLocalTargetCycle() throws { let fs = InMemoryFileSystem(emptyFiles: "/Foo/Sources/Foo/source.swift", - "/Bar/Sources/Bar/source.swift", - "/Baz/Sources/Baz/source.swift" + "/Foo/Sources/Bar/source.swift" ) let observability = ObservabilitySystem.makeForTesting() _ = try loadModulesGraph( + fileSystem: fs, + manifests: [ + Manifest.createRootManifest( + displayName: "Foo", + path: "/Foo", + targets: [ + TargetDescription(name: "Foo", dependencies: ["Bar"]), + TargetDescription(name: "Bar", dependencies: ["Foo"]) + ]), + ], + observabilityScope: observability.topScope + ) + + testDiagnostics(observability.diagnostics) { result in + result.check(diagnostic: "cyclic dependency declaration found: Bar -> Foo -> Bar", severity: .error) + } + } + + func testDependencyCycleWithoutTargetCycle() 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 graph = try loadModulesGraph( fileSystem: fs, manifests: [ Manifest.createRootManifest( displayName: "Foo", path: "/Foo", dependencies: [ - .localSourceControl(path: "/Foo", requirement: .upToNextMajor(from: "1.0.0")) + .localSourceControl(path: "/Bar", requirement: .upToNextMajor(from: "1.0.0")) + ], + products: [ + ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]) ], targets: [ - TargetDescription(name: "Foo"), + 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 declaration found: Foo -> Foo", severity: .error) + XCTAssertNoDiagnostics(observability.diagnostics) + PackageGraphTester(graph) { result in + result.check(packages: "Foo", "Bar") + result.check(targets: "Bar", "Baz", "Foo") } } diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index 01776baab5b..6e4bb85df05 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -1971,7 +1971,7 @@ final class WorkspaceTests: XCTestCase { // Ensure that the order of the manifests is stable. XCTAssertEqual( manifests.allDependencyManifests.map(\.value.manifest.displayName), - ["Foo", "Baz", "Bam", "Bar"] + ["Bam", "Baz", "Bar", "Foo"] ) XCTAssertNoDiagnostics(diagnostics) } @@ -11083,7 +11083,7 @@ final class WorkspaceTests: XCTestCase { // FIXME: rdar://72940946 // we need to improve this situation or diagnostics when working on identity result.check( - diagnostic: "cyclic dependency declaration found: Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage", + diagnostic: "'bar' dependency on '/tmp/ws/pkgs/other/utility' conflicts with dependency on '/tmp/ws/pkgs/foo/utility' which has the same identity 'utility'", severity: .error ) } @@ -11167,7 +11167,11 @@ final class WorkspaceTests: XCTestCase { // FIXME: rdar://72940946 // we need to improve this situation or diagnostics when working on identity result.check( - diagnostic: "cyclic dependency declaration found: Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage", + 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'.", severity: .error ) } @@ -11242,7 +11246,7 @@ final class WorkspaceTests: XCTestCase { // FIXME: rdar://72940946 // we need to improve this situation or diagnostics when working on identity result.check( - diagnostic: "cyclic dependency declaration found: Root -> BarPackage -> Root", + diagnostic: "product 'FooProduct' required by package 'bar' target 'BarTarget' not found in package 'FooPackage'.", severity: .error ) } @@ -11975,7 +11979,7 @@ final class WorkspaceTests: XCTestCase { targets: [ .init(name: "Root1Target", dependencies: [ .product(name: "FooProduct", package: "foo"), - .product(name: "Root2Target", package: "Root2") + .product(name: "Root2Product", package: "Root2") ]), ], products: [ @@ -12071,15 +12075,7 @@ final class WorkspaceTests: XCTestCase { try workspace.checkPackageGraph(roots: ["Root1", "Root2"]) { _, diagnostics in testDiagnostics(diagnostics) { result in result.check( - diagnostic: .regex("cyclic dependency declaration found: root[1|2] -> *"), - severity: .error - ) - result.check( - diagnostic: """ - exhausted attempts to resolve the dependencies graph, with the following dependencies unresolved: - * 'bar' from http://scm.com/org/bar - * 'foo' from http://scm.com/org/foo - """, + diagnostic: .regex("cyclic dependency declaration found: BarTarget -> BazTarget -> FooTarget -> BarTarget"), severity: .error ) }