diff --git a/CHANGELOG.md b/CHANGELOG.md index 35fa173eaba..26faec224b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ Note: This is in reverse chronological order, so newer entries are added to the top. +* [#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. + Swift 6.0 ----------- diff --git a/Sources/Basics/CMakeLists.txt b/Sources/Basics/CMakeLists.txt index bb3948ef432..343bbedbc56 100644 --- a/Sources/Basics/CMakeLists.txt +++ b/Sources/Basics/CMakeLists.txt @@ -36,6 +36,7 @@ add_library(Basics FileSystem/VFSOverlay.swift Graph/AdjacencyMatrix.swift Graph/DirectedGraph.swift + Graph/GraphAlgorithms.swift Graph/UndirectedGraph.swift SourceControlURL.swift HTTPClient/HTTPClient.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 021ad4fa9fe..18fd0e50f6a 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 3255e1d854b..5c8b6f6a413 100644 --- a/Sources/Commands/PackageCommands/PluginCommand.swift +++ b/Sources/Commands/PackageCommands/PluginCommand.swift @@ -371,6 +371,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, @@ -382,10 +383,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 @@ -469,10 +477,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 bd65237bb02..4c33d937181 100644 --- a/Sources/Commands/PackageCommands/ShowDependencies.swift +++ b/Sources/Commands/PackageCommands/ShowDependencies.swift @@ -43,13 +43,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: @@ -61,7 +67,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 b20ef4c3ee7..e99aef7f964 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. @@ -44,17 +46,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{ @@ -75,33 +66,27 @@ 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 // Create the packages. var manifestToPackage: [Manifest: Package] = [:] @@ -164,18 +149,23 @@ extension ModulesGraph { ) let rootPackages = resolvedPackages.filter { root.manifests.values.contains($0.manifest) } - checkAllDependenciesAreUsed(rootPackages, observabilityScope: observabilityScope) + checkAllDependenciesAreUsed(packages: resolvedPackages, rootPackages, observabilityScope: observabilityScope) return try ModulesGraph( rootPackages: rootPackages, 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 @@ -189,7 +179,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 { @@ -250,7 +245,7 @@ private func createResolvedPackages( platformVersionProvider: PlatformVersionProvider, fileSystem: FileSystem, observabilityScope: ObservabilityScope -) throws -> [ResolvedPackage] { +) throws -> IdentifiableSet { // Create package builder objects from the input manifests. let packageBuilders: [ResolvedPackageBuilder] = nodes.compactMap{ node in @@ -644,7 +639,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( @@ -1045,11 +1065,11 @@ private final class ResolvedPackageBuilder: ResolvedBuilder { var targets = products.reduce(into: IdentifiableSet()) { $0.formUnion($1.targets) } try targets.formUnion(self.targets.map { try $0.construct() }) - return try ResolvedPackage( + return ResolvedPackage( underlying: self.package, defaultLocalization: self.defaultLocalization, supportedPlatforms: self.supportedPlatforms, - dependencies: self.dependencies.map{ try $0.construct() }, + dependencies: self.dependencies.map { $0.package.identity }, targets: targets, products: products, registryMetadata: self.registryMetadata, @@ -1057,55 +1077,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 private(set) var reachableTargets: IdentifiableSet @@ -139,17 +138,24 @@ public struct ModulesGraph { return self.rootPackages.contains(id: package.id) } - private var 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 var 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. @@ -162,6 +168,7 @@ public struct ModulesGraph { public init( rootPackages: [ResolvedPackage], rootDependencies: [ResolvedPackage] = [], + packages: IdentifiableSet, dependencies requiredDependencies: [PackageReference], binaryArtifacts: [PackageIdentity: [String: BinaryArtifact]] ) throws { @@ -169,22 +176,7 @@ public struct ModulesGraph { self.requiredDependencies = requiredDependencies self.inputPackages = rootPackages + rootDependencies self.binaryArtifacts = binaryArtifacts - self.packages = try topologicalSort(inputPackages, successors: { $0.dependencies }) - let identitiesToPackages = self.packages.spm_createDictionary { ($0.identity, $0) } - - // 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. - var modulesToPackages = self.packages.reduce(into: [:], { partial, package in - package.targets.forEach { partial[$0.id] = package } - }) - - // 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. - var productsToPackages = packages.reduce(into: [:], { partial, package in - package.products.forEach { partial[$0.id] = package } - }) + self.packages = packages var allTargets = IdentifiableSet() var allProducts = IdentifiableSet() @@ -207,12 +199,8 @@ public struct ModulesGraph { switch dependency { case .target(let targetDependency, _): allTargets.insert(targetDependency) - modulesToPackages[targetDependency.id] = - identitiesToPackages[targetDependency.packageIdentity] case .product(let productDependency, _): allProducts.insert(productDependency) - productsToPackages[productDependency.id] = - identitiesToPackages[productDependency.packageIdentity] } } } @@ -227,9 +215,6 @@ public struct ModulesGraph { } } - self.modulesToPackages = modulesToPackages - self.productsToPackages = productsToPackages - // Compute the reachable targets and products. let inputTargets = self.inputPackages.flatMap { $0.targets } let inputProducts = self.inputPackages.flatMap { $0.products } @@ -264,17 +249,6 @@ public struct ModulesGraph { product.buildTriple = buildTriple return product }) - - self.modulesToPackages = .init(self.modulesToPackages.map { - var target = $0 - target.buildTriple = buildTriple - return (target, $1) - }, uniquingKeysWith: { $1 }) - self.productsToPackages = .init(self.productsToPackages.map { - var product = $0 - product.buildTriple = buildTriple - return (product, $1) - }, uniquingKeysWith: { $1 }) } /// Computes a map from each executable target in any of the root packages to the corresponding test targets. diff --git a/Sources/PackageGraph/Resolution/ResolvedPackage.swift b/Sources/PackageGraph/Resolution/ResolvedPackage.swift index 78cd87c2345..32536f0dbb1 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: IdentifiableSet, products: [ResolvedProduct], registryMetadata: RegistryReleaseMetadata?, diff --git a/Sources/SPMBuildCore/Plugins/PluginContextSerializer.swift b/Sources/SPMBuildCore/Plugins/PluginContextSerializer.swift index 0ea946cb38e..340f02d1cf8 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 fc832b07dc4..3ab08cd47d5 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/SPMTestSupport/PackageGraphTester.swift b/Sources/SPMTestSupport/PackageGraphTester.swift index a45ee9d57ad..1ebeda753fe 100644 --- a/Sources/SPMTestSupport/PackageGraphTester.swift +++ b/Sources/SPMTestSupport/PackageGraphTester.swift @@ -147,7 +147,7 @@ package final class PackageGraphResult { } package func find(package: PackageIdentity) -> ResolvedPackage? { - return graph.packages.first(where: { $0.identity == package }) + return graph.package(for: package) } private func reachableBuildTargets(in environment: BuildEnvironment) throws -> IdentifiableSet { diff --git a/Sources/Workspace/Workspace+Manifests.swift b/Sources/Workspace/Workspace+Manifests.swift index 511dd191d50..8b48dea8df2 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 @@ -494,12 +495,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) @@ -527,36 +523,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, - 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 26bcd60543c..816fb2dcbe7 100644 --- a/Sources/Workspace/Workspace.swift +++ b/Sources/Workspace/Workspace.swift @@ -1150,7 +1150,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 2c5f88cd415..d14e7f30d9e 100644 --- a/Tests/CommandsTests/MermaidPackageSerializerTests.swift +++ b/Tests/CommandsTests/MermaidPackageSerializerTests.swift @@ -110,7 +110,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, @@ -169,7 +169,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 f67e148dac1..92b47f8695d 100644 --- a/Tests/CommandsTests/PackageCommandTests.swift +++ b/Tests/CommandsTests/PackageCommandTests.swift @@ -647,6 +647,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 c81842f822a..d96ff4ca8a7 100644 --- a/Tests/FunctionalTests/PluginTests.swift +++ b/Tests/FunctionalTests/PluginTests.swift @@ -548,6 +548,7 @@ final class PluginTests: XCTestCase { pkgConfigDirectories: [], sdkRootPath: nil, fileSystem: localFileSystem, + modulesGraph: packageGraph, observabilityScope: observability.topScope, callbackQueue: delegateQueue, delegate: delegate, @@ -825,6 +826,7 @@ final 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 16c86130bbc..946fd37d9b7 100644 --- a/Tests/PackageGraphTests/ModulesGraphTests.swift +++ b/Tests/PackageGraphTests/ModulesGraphTests.swift @@ -83,12 +83,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) } @@ -190,32 +190,77 @@ final class ModulesGraphTests: XCTestCase { } } - 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 783329c01f1..584ff82d7bd 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) } @@ -11123,7 +11123,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 ) } @@ -11207,7 +11207,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 ) } @@ -11282,7 +11286,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 ) } @@ -12015,7 +12019,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: [ @@ -12111,15 +12115,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: Root[1|2]Target -> *"), severity: .error ) }