remove resolution retry and removing package.resolved in resolution failure (#3853)

motivation: resolution retry + removing of package resolved in failure is difficult to justify

changes:
* remove the retry code from package resolution and "give up" after one attempt without mutating the package resolved file
* fix the resolver to also compare locations so that resolved file with same identity but different location would work correctly
* add and adjust tests

Author: tomerd

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

@@ -1394,14 +1394,14 @@ private final class ContainerProvider {
     /// Get the container for the given identifier, loading it if necessary.
     func getContainer(for package: PackageReference, completion: @escaping (Result<PubGrubPackageContainer, Error>) -> Void) {
         // Return the cached container, if available.
-        if let container = self.containersCache[package] {
+        if let container = self.containersCache[package], package.equalsIncludingLocation(container.package) {
             return completion(.success(container))
         }
 
         if let prefetchSync = self.prefetches[package] {
             // If this container is already being prefetched, wait for that to complete
             prefetchSync.notify(queue: .sharedConcurrent) {
-                if let container = self.containersCache[package] {
+                if let container = self.containersCache[package], package.equalsIncludingLocation(container.package) {
                     // should be in the cache once prefetch completed
                     return completion(.success(container))
                 } else {

Sources/Workspace/ManagedDependency.swift

@@ -80,7 +80,7 @@ extension Workspace {
         ///     - unmanagedPath: A custom absolute path instead of the subpath.
         public func edited(subpath: RelativePath, unmanagedPath: AbsolutePath?) throws -> ManagedDependency {
             guard case .sourceControlCheckout =  self.state else {
-                throw InternalError("invalid depenedency state: \(self.state)")
+                throw InternalError("invalid dependency state: \(self.state)")
             }
             return ManagedDependency(
                 packageRef: self.packageRef,

Sources/Workspace/Workspace.swift

@@ -2400,8 +2400,6 @@ extension Workspace {
         explicitProduct: String? = nil,
         forceResolution: Bool,
         constraints: [PackageContainerConstraint],
-        retryOnPackagePathMismatch: Bool = true,
-        resetPinsStoreOnFailure: Bool = true,
         observabilityScope: ObservabilityScope
     ) throws -> DependencyManifests {
         // Ensure the cache path exists and validate that edited dependencies.
@@ -2488,51 +2486,12 @@ extension Workspace {
 
         // Update the pinsStore.
         let updatedDependencyManifests = try self.loadDependencyManifests(root: graphRoot, observabilityScope: observabilityScope)
-
-        // If we still have required URLs, we probably cloned a wrong URL for
-        // some package dependency.
-        //
-        // This would usually happen when we're resolving from scratch and the
-        // resolved file has an outdated entry for a transitive dependency whose
-        // URL was changed. For e.g., the resolved file could refer to a dependency
-        // through a ssh url but its new reference is now changed to http.
+        // If we still have missing packages, something is fundamentally wrong with the resolution of the graph
         let stillMissingPackages = updatedDependencyManifests.computePackages().missing
-        if !stillMissingPackages.isEmpty {
-            if retryOnPackagePathMismatch {
-                // Retry resolution which will most likely resolve correctly now since
-                // we have the manifest files of all the dependencies.
-                return try self.resolve(
-                    root: root,
-                    explicitProduct: explicitProduct,
-                    forceResolution: forceResolution,
-                    constraints: constraints,
-                    retryOnPackagePathMismatch: false,
-                    resetPinsStoreOnFailure: resetPinsStoreOnFailure,
-                    observabilityScope: observabilityScope
-                )
-            } else if resetPinsStoreOnFailure, !pinsStore.pinsMap.isEmpty {
-                // If we weren't able to resolve properly even after a retry, it
-                // could mean that the dependency at fault has a different
-                // version of the manifest file which contains dependencies that
-                // have also changed their package references.
-                pinsStore.unpinAll()
-                try pinsStore.saveState(toolsVersion: rootManifestsMinimumToolsVersion)
-                // try again with pins reset
-                return try self.resolve(
-                    root: root,
-                    explicitProduct: explicitProduct,
-                    forceResolution: forceResolution,
-                    constraints: constraints,
-                    retryOnPackagePathMismatch: false,
-                    resetPinsStoreOnFailure: false,
-                    observabilityScope: observabilityScope
-                )
-            } else {
-                // give up
-                let missing = stillMissingPackages.map{ $0.description }
-                observabilityScope.emit(error: "exhausted attempts to resolve the dependencies graph, with '\(missing.joined(separator: "', '"))' unresolved.")
-                return updatedDependencyManifests
-            }
+        guard stillMissingPackages.isEmpty else {
+            let missing = stillMissingPackages.map{ $0.description }
+            observabilityScope.emit(error: "exhausted attempts to resolve the dependencies graph, with '\(missing.joined(separator: "', '"))' unresolved.")
+            return updatedDependencyManifests
         }
 
         self.pinAll(

Tests/WorkspaceTests/WorkspaceTests.swift

@@ -3237,7 +3237,10 @@ final class WorkspaceTests: XCTestCase {
         }
     }
 
-    func testDependencySwitchWithSameIdentity() throws {
+
+    // Test that switching between two same local packages placed at
+    // different locations works correctly.
+    func testDependencySwitchLocalWithSameIdentity() throws {
         let sandbox = AbsolutePath("/tmp/ws/")
         let fs = InMemoryFileSystem()
 
@@ -3279,9 +3282,6 @@ final class WorkspaceTests: XCTestCase {
             ]
         )
 
-        // Test that switching between two same local packages placed at
-        // different locations works correctly.
-
         var deps: [MockDependency] = [
             .fileSystem(path: "./Foo", products: .specific(["Foo"])),
         ]
@@ -3315,6 +3315,84 @@ final class WorkspaceTests: XCTestCase {
         }
     }
 
+    // Test that switching between two remote packages at
+    // different locations works correctly.
+    func testDependencySwitchRemoteWithSameIdentity() throws {
+        let sandbox = AbsolutePath("/tmp/ws/")
+        let fs = InMemoryFileSystem()
+
+        let workspace = try MockWorkspace(
+            sandbox: sandbox,
+            fs: fs,
+            roots: [
+                MockPackage(
+                    name: "Root",
+                    targets: [
+                        MockTarget(name: "Root", dependencies: []),
+                    ],
+                    products: [],
+                    dependencies: []
+                ),
+            ],
+            packages: [
+                MockPackage(
+                    name: "Foo",
+                    url: "https://scm.com/org/foo",
+                    targets: [
+                        MockTarget(name: "Foo"),
+                    ],
+                    products: [
+                        MockProduct(name: "Foo", targets: ["Foo"]),
+                    ],
+                    versions: ["1.0.0"]
+                ),
+                MockPackage(
+                    name: "Foo",
+                    url: "https://scm.com/other/foo",
+                    targets: [
+                        MockTarget(name: "OtherFoo"),
+                    ],
+                    products: [
+                        MockProduct(name: "OtherFoo", targets: ["OtherFoo"]),
+                    ],
+                    versions: ["1.1.0"]
+                ),
+            ]
+        )
+
+        var deps: [MockDependency] = [
+            .sourceControl(url: "https://scm.com/org/foo", requirement: .exact("1.0.0")),
+        ]
+        try workspace.checkPackageGraph(roots: ["Root"], deps: deps) { graph, diagnostics in
+            XCTAssertNoDiagnostics(diagnostics)
+            PackageGraphTester(graph) { result in
+                result.check(roots: "Root")
+                result.check(packages: "Foo", "Root")
+            }
+        }
+        workspace.checkManagedDependencies { result in
+            result.check(dependency: "foo", at: .checkout(.version("1.0.0")))
+        }
+        do {
+            let ws = try workspace.getOrCreateWorkspace()
+            XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "https://scm.com/org/foo")
+        }
+
+        deps = [
+            .sourceControl(url: "https://scm.com/other/foo", requirement: .exact("1.1.0"))
+        ]
+        try workspace.checkPackageGraph(roots: ["Root"], deps: deps) { _, diagnostics in
+            XCTAssertNoDiagnostics(diagnostics)
+        }
+        workspace.checkManagedDependencies { result in
+            result.check(dependency: "foo", at: .checkout(.version("1.1.0")))
+        }
+        do {
+            let ws = try workspace.getOrCreateWorkspace()
+            XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "https://scm.com/other/foo")
+        }
+    }
+
     func testResolvedFileUpdate() throws {
         let sandbox = AbsolutePath("/tmp/ws/")
         let fs = InMemoryFileSystem()
@@ -3525,6 +3603,9 @@ final class WorkspaceTests: XCTestCase {
         }
     }
 
+    // In this test, we get into a state where an entry in the resolved
+    // file for a transitive dependency whose URL is later changed to
+    // something else, while keeping the same package identity.
     func testTransitiveDependencySwitchWithSameIdentity() throws {
         let sandbox = AbsolutePath("/tmp/ws/")
         let fs = InMemoryFileSystem()
@@ -3536,44 +3617,59 @@ final class WorkspaceTests: XCTestCase {
                 MockPackage(
                     name: "Root",
                     targets: [
-                        MockTarget(name: "Root", dependencies: ["Bar"]),
+                        MockTarget(
+                            name: "Root",
+                            dependencies: [
+                                .product(name: "Bar", package: "bar")
+                            ]),
                     ],
                     products: [],
                     dependencies: [
-                        .sourceControl(path: "./Bar", requirement: .upToNextMajor(from: "1.0.0")),
+                        .sourceControl(url: "https://scm.com/org/bar", requirement: .upToNextMajor(from: "1.0.0")),
                     ]
                 ),
             ],
             packages: [
                 MockPackage(
                     name: "Bar",
+                    url: "https://scm.com/org/bar",
                     targets: [
-                        MockTarget(name: "Bar", dependencies: ["Foo"]),
+                        MockTarget(
+                            name: "Bar",
+                            dependencies: [
+                                .product(name: "Foo", package: "foo")
+                            ]),
                     ],
                     products: [
                         MockProduct(name: "Bar", targets: ["Bar"]),
                     ],
                     dependencies: [
-                        .sourceControl(path: "./Foo", requirement: .upToNextMajor(from: "1.0.0")),
+                        .sourceControl(url: "https://scm.com/org/foo", requirement: .upToNextMajor(from: "1.0.0")),
                     ],
                     versions: ["1.0.0"]
                 ),
                 MockPackage(
                     name: "Bar",
+                    url: "https://scm.com/org/bar",
                     targets: [
-                        MockTarget(name: "Bar", dependencies: ["Nested/Foo"]),
+                        MockTarget(
+                            name: "Bar",
+                            dependencies: [
+                                .product(name: "OtherFoo", package: "foo")
+                            ]),
                     ],
                     products: [
                         MockProduct(name: "Bar", targets: ["Bar"]),
                     ],
                     dependencies: [
-                        .sourceControl(path: "Nested/Foo", requirement: .upToNextMajor(from: "1.0.0")),
+                        .sourceControl(url: "https://scm.com/other/foo", requirement: .upToNextMajor(from: "1.0.0")),
                     ],
                     versions: ["1.1.0"],
                     toolsVersion: .v5
                 ),
                 MockPackage(
                     name: "Foo",
+                    url: "https://scm.com/org/foo",
                     targets: [
                         MockTarget(name: "Foo"),
                     ],
@@ -3584,40 +3680,27 @@ final class WorkspaceTests: XCTestCase {
                 ),
                 MockPackage(
                     name: "Foo",
-                    path: "Nested/Foo",
+                    url: "https://scm.com/other/foo",
                     targets: [
-                        MockTarget(name: "Foo"),
+                        MockTarget(name: "OtherFoo"),
                     ],
                     products: [
-                        MockProduct(name: "Nested/Foo", targets: ["Foo"]),
+                        MockProduct(name: "OtherFoo", targets: ["OtherFoo"]),
                     ],
                     versions: ["1.0.0"]
                 ),
             ]
         )
 
-        // In this test, we get into a state where add an entry in the resolved
-        // file for a transitive dependency whose URL is later changed to
-        // something else, while keeping the same package identity.
-        //
-        // This is normally detected during pins validation before the
-        // dependency resolution process even begins but if we're starting with
-        // a clean slate, we don't even know about the correct urls of the
-        // transitive dependencies. We will end up fetching the wrong
-        // dependency as we prefetch the pins. If we get into this case, it
-        // should kick off another dependency resolution operation which will
-        // have enough information to remove the invalid pins of transitive
-        // dependencies.
-
         var deps: [MockDependency] = [
-            .sourceControl(path: "./Bar", requirement: .exact("1.0.0"), products: .specific(["Bar"])),
+            .sourceControl(url: "https://scm.com/org/bar", requirement: .exact("1.0.0")),
         ]
         try workspace.checkPackageGraph(roots: ["Root"], deps: deps) { graph, diagnostics in
+            XCTAssertNoDiagnostics(diagnostics)
             PackageGraphTester(graph) { result in
                 result.check(roots: "Root")
                 result.check(packages: "Bar", "Foo", "Root")
             }
-            XCTAssertNoDiagnostics(diagnostics)
         }
         workspace.checkManagedDependencies { result in
             result.check(dependency: "foo", at: .checkout(.version("1.0.0")))
@@ -3626,22 +3709,22 @@ final class WorkspaceTests: XCTestCase {
 
         do {
             let ws = try workspace.getOrCreateWorkspace()
-            XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "/tmp/ws/pkgs/Foo")
+            XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "https://scm.com/org/foo")
         }
 
         workspace.checkReset { diagnostics in
             XCTAssertNoDiagnostics(diagnostics)
         }
 
         deps = [
-            .sourceControl(path: "./Bar", requirement: .exact("1.1.0"), products: .specific(["Bar"])),
+            .sourceControl(url: "https://scm.com/org/bar", requirement: .exact("1.1.0")),
         ]
         try workspace.checkPackageGraph(roots: ["Root"], deps: deps) { graph, diagnostics in
+            XCTAssertNoDiagnostics(diagnostics)
             PackageGraphTester(graph) { result in
                 result.check(roots: "Root")
                 result.check(packages: "Bar", "Foo", "Root")
             }
-            XCTAssertNoDiagnostics(diagnostics)
         }
         workspace.checkManagedDependencies { result in
             result.check(dependency: "foo", at: .checkout(.version("1.0.0")))
@@ -3650,7 +3733,7 @@ final class WorkspaceTests: XCTestCase {
 
         do {
             let ws = try workspace.getOrCreateWorkspace()
-            XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "/tmp/ws/pkgs/Nested/Foo")
+            XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "https://scm.com/other/foo")
         }
     }