refine location comparison when pre-loading package state from local state (#7002)

Author: tomerd

Sources/PackageGraph/PubGrub/ContainerProvider.swift

@@ -62,14 +62,14 @@ final class ContainerProvider {
         completion: @escaping (Result<PubGrubPackageContainer, Error>) -> Void
     ) {
         // Return the cached container, if available.
-        if let container = self.containersCache[package], package.equalsIncludingLocation(container.package) {
+        if let container = self.containersCache[comparingLocation: 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], package.equalsIncludingLocation(container.package) {
+                if let container = self.containersCache[comparingLocation: package] {
                     // should be in the cache once prefetch completed
                     return completion(.success(container))
                 } else {
@@ -125,3 +125,12 @@ final class ContainerProvider {
         }
     }
 }
+
+extension ThreadSafeKeyValueStore where Key == PackageReference, Value == PubGrubPackageContainer {
+    subscript(comparingLocation package: PackageReference) -> PubGrubPackageContainer? {
+        if let container = self[package], container.package.equalsIncludingLocation(package) {
+            return container
+        }
+        return .none
+    }
+}

Sources/PackageModel/PackageIdentity.swift

@@ -424,7 +424,7 @@ public struct CanonicalPackageLocation: Equatable, CustomStringConvertible {
 }
 
 /// Similar to `CanonicalPackageLocation` but differentiates based on the scheme.
-public struct CanonicalPackageURL: CustomStringConvertible {
+public struct CanonicalPackageURL: Equatable, CustomStringConvertible {
     public let description: String
     public let scheme: String?
 

Sources/PackageModel/PackageReference.swift

@@ -161,7 +161,24 @@ extension PackageReference: Equatable {
 
     // TODO: consider rolling into Equatable
     public func equalsIncludingLocation(_ other: PackageReference) -> Bool {
-        return self.identity == other.identity && self.canonicalLocation == other.canonicalLocation
+        if self.identity != other.identity {
+            return false
+        }
+        if self.canonicalLocation != other.canonicalLocation {
+            return false
+        }
+        switch (self.kind, other.kind) {
+        case (.remoteSourceControl(let lurl), .remoteSourceControl(let rurl)):
+            return lurl.canonicalURL == rurl.canonicalURL
+        default:
+            return true
+        }
+    }
+}
+
+extension SourceControlURL {
+    var canonicalURL: CanonicalPackageURL {
+        CanonicalPackageURL(self.absoluteString)
     }
 }
 

Sources/Workspace/ManagedDependency.swift

@@ -196,7 +196,7 @@ extension Workspace {
 
         // When loading manifests in Workspace, there are cases where we must also compare the location
         // as it may attempt to load manifests for dependencies that have the same identity but from a different location
-        // (e.g. dependency is changed to  a fork with the same identity)
+        // (e.g. dependency is changed to a fork with the same identity)
         public subscript(comparingLocation package: PackageReference) -> ManagedDependency? {
             if let dependency = self.dependencies[package.identity], dependency.packageRef.equalsIncludingLocation(package) {
                 return dependency

Sources/Workspace/Workspace+Manifests.swift

@@ -198,9 +198,11 @@ extension Workspace {
                 }
             }
 
+            let topLevelDependencies = root.packages.flatMap { $1.manifest.dependencies.map { $0.packageRef } }
+
             var requiredIdentities: OrderedCollections.OrderedSet<PackageReference> = []
             _ = transitiveClosure(inputNodes) { node in
-                node.manifest.dependenciesRequired(for: node.productFilter).compactMap { dependency in
+                return node.manifest.dependenciesRequired(for: node.productFilter).compactMap{ dependency in
                     let package = dependency.packageRef
                     let (inserted, index) = requiredIdentities.append(package)
                     if !inserted {
@@ -209,17 +211,26 @@ extension Workspace {
                         if existing.canonicalLocation == package.canonicalLocation {
                             // same literal location is fine
                             if existing.locationString != package.locationString {
-                                let preferred = [existing, package].sorted(by: {
-                                    $0.locationString > $1.locationString
-                                }).first! // safe
-                                observabilityScope.emit(debug: """
-                                similar variants of package '\(package.identity)' \
-                                found at '\(package.locationString)' and '\(existing.locationString)'. \
-                                using preferred variant '\(preferred.locationString)'
-                                """)
-                                if preferred.locationString != existing.locationString {
-                                    requiredIdentities.remove(existing)
-                                    requiredIdentities.insert(preferred, at: index)
+                                // we prefer the top level dependencies
+                                if topLevelDependencies.contains(where: { $0.locationString ==  existing.locationString }) {
+                                    observabilityScope.emit(debug: """
+                                    similar variants of package '\(package.identity)' \
+                                    found at '\(package.locationString)' and '\(existing.locationString)'. \
+                                    using preferred root variant '\(existing.locationString)'
+                                    """)
+                                } else {
+                                    let preferred = [existing, package].sorted(by: {
+                                        $0.locationString > $1.locationString
+                                    }).first! // safe
+                                    observabilityScope.emit(debug: """
+                                    similar variants of package '\(package.identity)' \
+                                    found at '\(package.locationString)' and '\(existing.locationString)'. \
+                                    using preferred variant '\(preferred.locationString)'
+                                    """)
+                                    if preferred.locationString != existing.locationString {
+                                        requiredIdentities.remove(existing)
+                                        requiredIdentities.insert(preferred, at: index)
+                                    }
                                 }
                             }
                         } else {

Sources/Workspace/Workspace.swift

@@ -1723,8 +1723,8 @@ extension Workspace {
                 needsUpdate = true
             } else {
                 for dependency in dependenciesToPin {
-                    if let pin = storedPinStore.pins.first(where: { $0.value.packageRef.equalsIncludingLocation(dependency.packageRef) }) {
-                        if pin.value.state != PinsStore.Pin(dependency)?.state {
+                    if let pin = storedPinStore.pins[comparingLocation: dependency.packageRef] {
+                        if pin.state != PinsStore.Pin(dependency)?.state {
                             needsUpdate = true
                             break
                         }
@@ -2531,9 +2531,8 @@ extension Workspace {
             // a required dependency that is already loaded (managed) should be represented in the pins store.
             // also comparing location as it may have changed at this point
             if requiredDependencies.contains(where: { $0.equalsIncludingLocation(dependency.packageRef) }) {
-                let pin = pinsStore.pins[dependency.packageRef.identity]
                 // if pin not found, or location is different (it may have changed at this point) pin it
-                if !(pin?.packageRef.equalsIncludingLocation(dependency.packageRef) ?? false) {
+                if pinsStore.pins[comparingLocation: dependency.packageRef] == .none {
                     pinsStore.pin(dependency)
                 }
             } else if let pin = pinsStore.pins[dependency.packageRef.identity]  {
@@ -3099,7 +3098,7 @@ extension Workspace {
 
                 // only update if necessary
                 if !workingCopy.exists(revision: revision) {
-                    // The fetch operation may update contents of the checkout, 
+                    // The fetch operation may update contents of the checkout,
                     // so we need to do mutable-immutable dance.
                     try self.fileSystem.chmod(.userWritable, path: checkoutPath, options: [.recursive, .onlyFiles])
                     try workingCopy.fetch()
@@ -3951,3 +3950,12 @@ extension Workspace.ManagedDependencies {
         })
     }
 }
+
+extension PinsStore.Pins {
+    subscript(comparingLocation package: PackageReference) -> PinsStore.Pin? {
+        if let pin = self[package.identity], pin.packageRef.equalsIncludingLocation(package) {
+            return pin
+        }
+        return .none
+    }
+}