Merge pull request #1760 from aciidb0mb3r/stable-dependency-manifests

[Workspace] Use topological sort when creating DependencyManifests

Author: aciidgh

Sources/Workspace/Workspace.swift

@@ -914,7 +914,7 @@ extension Workspace {
         var loadedManifests = [String: Manifest]()
 
         // Compute the transitive closure of available dependencies.
-        let dependencies = transitiveClosure(inputManifests.map({ KeyedPair($0, key: $0.name) })) { node in
+        let allManifests = try! topologicalSort(inputManifests.map({ KeyedPair($0, key: $0.name) })) { node in
             return node.item.dependencies.compactMap({ dependency in
                 let ref = dependency.createPackageRef()
                 let manifest = loadedManifests[ref.identity] ?? loadManifest(for: ref, diagnostics: diagnostics)
@@ -923,10 +923,9 @@ extension Workspace {
             })
         }
 
-        // It is possible that some root dependency is also present as a regular dependency, so we
-        // form a unique set of all dependency manifests.
-        let allManifests = Set(rootDependencyManifests.map({ KeyedPair($0, key: $0.name) }) + dependencies).map({ $0.item })
-        let deps: [(Manifest, ManagedDependency)] = allManifests.map({
+        let allDependencyManifests = allManifests.map({ $0.item }).filter({ !root.manifests.contains($0) })
+
+        let deps: [(Manifest, ManagedDependency)] = allDependencyManifests.map({
             // FIXME: We should use package name directly once this radar is fixed:
             // <rdar://problem/33693433> Ensure that identity and package name
             // are the same once we have an API to specify identity in the

Tests/WorkspaceTests/WorkspaceTests.swift

@@ -991,6 +991,72 @@ final class WorkspaceTests: XCTestCase {
         }
     }
 
+    func testDependencyManifestsOrder() throws {
+        let sandbox = AbsolutePath("/tmp/ws/")
+        let fs = InMemoryFileSystem()
+
+        let workspace = try TestWorkspace(
+            sandbox: sandbox,
+            fs: fs,
+            roots: [
+                TestPackage(
+                    name: "Root1",
+                    targets: [
+                        TestTarget(name: "Root1", dependencies: ["Foo", "Bar", "Baz", "Bam"]),
+                    ],
+                    products: [],
+                    dependencies: [
+                        TestDependency(name: "Foo", requirement: .upToNextMajor(from: "1.0.0")),
+                        TestDependency(name: "Bar", requirement: .upToNextMajor(from: "1.0.0")),
+                        TestDependency(name: "Baz", requirement: .upToNextMajor(from: "1.0.0")),
+                        TestDependency(name: "Bam", requirement: .upToNextMajor(from: "1.0.0")),
+                    ]
+                ),
+            ],
+            packages: [
+                TestPackage(
+                    name: "Foo",
+                    targets: [
+                        TestTarget(name: "Foo", dependencies: ["Bar", "Baz"]),
+                    ],
+                    products: [
+                        TestProduct(name: "Foo", targets: ["Foo"]),
+                    ],
+                    dependencies: [
+                        TestDependency(name: "Bar", requirement: .upToNextMajor(from: "1.0.0")),
+                        TestDependency(name: "Baz", requirement: .upToNextMajor(from: "1.0.0")),
+                    ],
+                    versions: ["1.0.0"]
+                ),
+                .genericPackage1(named: "Bar"),
+                TestPackage(
+                    name: "Baz",
+                    targets: [
+                        TestTarget(name: "Baz", dependencies: ["Bam"]),
+                    ],
+                    products: [
+                        TestProduct(name: "Baz", targets: ["Baz"]),
+                    ],
+                    dependencies: [
+                        TestDependency(name: "Bam", requirement: .upToNextMajor(from: "1.0.0")),
+                    ],
+                    versions: ["1.0.0"]
+                ),
+                .genericPackage1(named: "Bam"),
+            ]
+        )
+
+        workspace.checkPackageGraph(roots: ["Root1"]) { (graph, diagnostics) in
+            XCTAssertNoDiagnostics(diagnostics)
+        }
+
+        workspace.loadDependencyManifests(roots: ["Root1"]) { (manifests, diagnostics) in
+			// Ensure that the order of the manifests is stable.
+			XCTAssertEqual(manifests.allManifests().map({$0.name}), ["Foo", "Baz", "Bam", "Bar"])
+            XCTAssertNoDiagnostics(diagnostics)
+        }
+    }
+
     func testBranchAndRevision() throws {
         let sandbox = AbsolutePath("/tmp/ws/")
         let fs = InMemoryFileSystem()

Tests/WorkspaceTests/XCTestManifests.swift

@@ -48,6 +48,7 @@ extension WorkspaceTests {
         ("testCleanAndReset", testCleanAndReset),
         ("testDeletedCheckoutDirectory", testDeletedCheckoutDirectory),
         ("testDependencyManifestLoading", testDependencyManifestLoading),
+        ("testDependencyManifestsOrder", testDependencyManifestsOrder),
         ("testDependencyResolutionWithEdit", testDependencyResolutionWithEdit),
         ("testEditDependency", testEditDependency),
         ("testGraphData", testGraphData),