[5.6] make package.resolved more deterministic in edge cases (#3967)

Sources/PackageCollections/PackageCollections.swift

@@ -571,7 +571,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
                         let matches: [PackageCollectionsModel.Package]
                         if let location = location {
                             // A package identity can be associated with multiple repository URLs
-                            matches = packagesCollections.packages.filter { CanonicalPackageIdentity($0.location) == CanonicalPackageIdentity(location) }
+                            matches = packagesCollections.packages.filter { CanonicalPackageLocation($0.location) == CanonicalPackageLocation(location) }
                         }
                         else {
                             matches = packagesCollections.packages

Sources/PackageGraph/PackageGraph+Loading.swift

@@ -236,21 +236,7 @@ private func createResolvedPackages(
 
         // Establish the manifest-declared package dependencies.
         package.manifest.dependenciesRequired(for: packageBuilder.productFilter).forEach { dependency in
-            // FIXME: change this validation logic to use identity instead of location
-            let dependencyLocation: String
-            switch dependency {
-            case .fileSystem(let settings):
-                dependencyLocation = settings.path.pathString
-            case .sourceControl(let settings):
-                switch settings.location {
-                case .local(let path):
-                    dependencyLocation = path.pathString
-                case .remote(let url):
-                    dependencyLocation = url.absoluteString
-                }
-            case .registry(let settings):
-                dependencyLocation = settings.identity.description
-            }
+            let dependencyPackageRef = dependency.createPackageRef()
 
             // Otherwise, look it up by its identity.
             if let resolvedPackage = packagesByIdentity[dependency.identity] {
@@ -260,7 +246,7 @@ private func createResolvedPackages(
                 guard dependencies[resolvedPackage.package.identity] == nil else {
                     let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
                         package: package.identity.description,
-                        dependencyLocation: dependencyLocation,
+                        dependencyLocation: dependencyPackageRef.locationString,
                         otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
                         identity: dependency.identity)
                     return packageObservabilityScope.emit(error)
@@ -269,10 +255,10 @@ private func createResolvedPackages(
                 // check if the resolved package location is the same as the dependency one
                 // if not, this means that the dependencies share the same identity
                 // which only allowed when overriding
-                if !LocationComparator.areEqual(resolvedPackage.package.manifest.packageLocation, dependencyLocation) && !resolvedPackage.allowedToOverride {
+                if resolvedPackage.package.manifest.canonicalPackageLocation != dependencyPackageRef.canonicalLocation && !resolvedPackage.allowedToOverride {
                     let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
                         package: package.identity.description,
-                        dependencyLocation: dependencyLocation,
+                        dependencyLocation: dependencyPackageRef.locationString,
                         otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
                         identity: dependency.identity)
                     // 9/2021 this is currently emitting a warning only to support
@@ -284,14 +270,18 @@ private func createResolvedPackages(
                     } else {
                         return packageObservabilityScope.emit(error)
                     }
+                } else if resolvedPackage.package.manifest.canonicalPackageLocation == dependencyPackageRef.canonicalLocation &&
+                            resolvedPackage.package.manifest.packageLocation != dependencyPackageRef.locationString  &&
+                            !resolvedPackage.allowedToOverride {
+                    packageObservabilityScope.emit(info: "dependency on '\(resolvedPackage.package.identity)' is represented by similar locations ('\(resolvedPackage.package.manifest.packageLocation)' and '\(dependencyPackageRef.locationString)') which are treated as the same canonical location '\(dependencyPackageRef.canonicalLocation)'.")
                 }
 
                 // checks if two dependencies have the same explicit name which can cause target based dependency package lookup issue
                 if let explicitDependencyName = dependency.explicitNameForTargetDependencyResolutionOnly {
                     if let previouslyResolvedPackage = dependenciesByNameForTargetDependencyResolution[explicitDependencyName] {
                         let error = PackageGraphError.dependencyAlreadySatisfiedByName(
                             package: package.identity.description,
-                            dependencyLocation: dependencyLocation,
+                            dependencyLocation: dependencyPackageRef.locationString,
                             otherDependencyURL: previouslyResolvedPackage.package.manifest.packageLocation,
                             name: explicitDependencyName)
                         return packageObservabilityScope.emit(error)
@@ -302,7 +292,7 @@ private func createResolvedPackages(
                 if let previouslyResolvedPackage = dependenciesByNameForTargetDependencyResolution[dependency.identity.description] {
                     let error = PackageGraphError.dependencyAlreadySatisfiedByName(
                         package: package.identity.description,
-                        dependencyLocation: dependencyLocation,
+                        dependencyLocation: dependencyPackageRef.locationString,
                         otherDependencyURL: previouslyResolvedPackage.package.manifest.packageLocation,
                         name: dependency.identity.description)
                     return packageObservabilityScope.emit(error)
@@ -674,22 +664,3 @@ fileprivate func findCycle(
     // Couldn't find any cycle in the graph.
     return nil
 }
-
-
-// TODO: model package location better / encapsulate into a new type (PackageLocation) so that such comparison is reusable
-// additionally move and rename CanonicalPackageIdentity to become a detail function of the PackageLocation abstraction, as it is not used otherwise
-struct LocationComparator {
-    static func areEqual(_ lhs: String, _ rhs: String) -> Bool {
-        if lhs == rhs {
-            return true
-        }
-
-        let canonicalLHS = CanonicalPackageIdentity(lhs)
-        let canonicalRHS = CanonicalPackageIdentity(rhs)
-        if canonicalLHS == canonicalRHS {
-            return true
-        }
-
-        return false
-    }
-}

Sources/PackageGraph/PinsStore.swift

@@ -101,6 +101,13 @@ public final class PinsStore {
         self._pins[pin.packageRef.identity] = pin
     }
 
+    /// Remove a pin.
+    ///
+    /// This will replace any previous pin with same package name.
+    public func remove(_ pin: Pin) {
+        self._pins[pin.packageRef.identity] = nil
+    }
+
     /// Unpin all of the currently pinned dependencies.
     ///
     /// This method does not automatically write to state file.

Sources/PackageModel/Manifest.swift

@@ -48,6 +48,11 @@ public final class Manifest {
     /// The repository URL the manifest was loaded from.
     public let packageLocation: String
 
+    /// The canonical repository URL the manifest was loaded from.
+    public var canonicalPackageLocation: CanonicalPackageLocation {
+        CanonicalPackageLocation(self.packageLocation)
+    }
+
     // FIXME: deprecated 2/2021, remove once clients migrate
     @available(*, deprecated, message: "use packageLocation instead")
     public var url: String {

Sources/PackageModel/Manifest/PackageDependencyDescription.swift

@@ -72,13 +72,13 @@ public enum PackageDependency: Equatable {
     public var nameForTargetDependencyResolutionOnly: String {
         switch self {
         case .fileSystem(let settings):
-            return settings.nameForTargetDependencyResolutionOnly ?? LegacyPackageIdentity.computeDefaultName(fromPath: settings.path)
+            return settings.nameForTargetDependencyResolutionOnly ?? PackageIdentityParser.computeDefaultName(fromPath: settings.path)
         case .sourceControl(let settings):
             switch settings.location {
             case .local(let path):
-                return settings.nameForTargetDependencyResolutionOnly ?? LegacyPackageIdentity.computeDefaultName(fromPath: path)
+                return settings.nameForTargetDependencyResolutionOnly ?? PackageIdentityParser.computeDefaultName(fromPath: path)
             case .remote(let url):
-                return settings.nameForTargetDependencyResolutionOnly ?? LegacyPackageIdentity.computeDefaultName(fromURL: url)
+                return settings.nameForTargetDependencyResolutionOnly ?? PackageIdentityParser.computeDefaultName(fromURL: url)
             }
         case .registry:
             return self.identity.description

Sources/PackageModel/PackageIdentity.swift

@@ -12,24 +12,8 @@ import Foundation
 import TSCBasic
 import TSCUtility
 
-/// When set to `false`,
-/// `PackageIdentity` uses the canonical location of package dependencies as its identity.
-/// Otherwise, only the last path component is used to identify package dependencies.
-public var _useLegacyIdentities: Bool = true {
-    willSet {
-        PackageIdentity.provider = newValue ? LegacyPackageIdentity.self : CanonicalPackageIdentity.self
-    }
-}
-
-internal protocol PackageIdentityProvider: CustomStringConvertible {
-    init(_ string: String)
-}
-
 /// The canonical identifier for a package, based on its source location.
 public struct PackageIdentity: CustomStringConvertible {
-    /// The underlying type used to create package identities.
-    internal static var provider: PackageIdentityProvider.Type = LegacyPackageIdentity.self
-
     /// A textual representation of this instance.
     public let description: String
 
@@ -49,13 +33,13 @@ public struct PackageIdentity: CustomStringConvertible {
     /// - Parameter urlString: The package's URL.
     // FIXME: deprecate this
     public init(urlString: String) {
-        self.description = Self.provider.init(urlString).description
+        self.description = PackageIdentityParser(urlString).description
     }
 
     /// Creates a package identity from a file path.
     /// - Parameter path: An absolute path to the package.
     public init(path: AbsolutePath) {
-        self.description = Self.provider.init(path.pathString).description
+        self.description = PackageIdentityParser(path.pathString).description
     }
 
     /// Creates a plain package identity for a root package
@@ -282,7 +266,7 @@ extension PackageIdentity {
 
 // MARK: -
 
-struct LegacyPackageIdentity: PackageIdentityProvider, Equatable {
+struct PackageIdentityParser {
     /// A textual representation of this instance.
     public let description: String
 
@@ -329,7 +313,7 @@ struct LegacyPackageIdentity: PackageIdentityProvider, Equatable {
     }
 }
 
-/// A canonicalized package identity.
+/// A canonicalized package location.
 ///
 /// A package may declare external packages as dependencies in its manifest.
 /// Each external package is uniquely identified by the location of its source code.
@@ -400,7 +384,7 @@ struct LegacyPackageIdentity: PackageIdentityProvider, Equatable {
 ///   ```
 ///   file:///Users/mona/LinkedList → /Users/mona/LinkedList
 ///   ```
-public struct CanonicalPackageIdentity: PackageIdentityProvider, Equatable {
+public struct CanonicalPackageLocation: Equatable, CustomStringConvertible {
     /// A textual representation of this instance.
     public let description: String
 

Sources/PackageModel/PackageReference.swift

@@ -34,7 +34,7 @@ public struct PackageReference {
         /// A package from  a registry.
         case registry(PackageIdentity)
 
-        // FIXME: we should not need this
+        // FIXME: we should not need this once we migrate off URLs
         //@available(*, deprecated)
         public var locationString: String {
             switch self {
@@ -52,6 +52,12 @@ public struct PackageReference {
             }
         }
 
+        // FIXME: we should not need this once we migrate off URLs
+        //@available(*, deprecated)
+        public var canonicalLocation: CanonicalPackageLocation {
+            return CanonicalPackageLocation(self.locationString)
+        }
+
         public var description: String {
             switch self {
             case .root(let path):
@@ -92,12 +98,18 @@ public struct PackageReference {
     /// The location of the package.
     ///
     /// This could be a remote repository, local repository or local package.
-    // FIXME: we should not need this
+    // FIXME: we should not need this once we migrate off URLs
     //@available(*, deprecated)
     public var locationString: String {
         self.kind.locationString
     }
 
+    // FIXME: we should not need this once we migrate off URLs
+    //@available(*, deprecated)
+    public var canonicalLocation: CanonicalPackageLocation {
+        self.kind.canonicalLocation
+    }
+
     /// The kind of package: root, local, or remote.
     public let kind: Kind
 
@@ -107,13 +119,13 @@ public struct PackageReference {
         self.kind = kind
         switch kind {
         case .root(let path):
-            self.deprecatedName = name ?? LegacyPackageIdentity.computeDefaultName(fromPath: path)
+            self.deprecatedName = name ?? PackageIdentityParser.computeDefaultName(fromPath: path)
         case .fileSystem(let path):
-            self.deprecatedName = name ?? LegacyPackageIdentity.computeDefaultName(fromPath: path)
+            self.deprecatedName = name ?? PackageIdentityParser.computeDefaultName(fromPath: path)
         case .localSourceControl(let path):
-            self.deprecatedName = name ?? LegacyPackageIdentity.computeDefaultName(fromPath: path)
+            self.deprecatedName = name ?? PackageIdentityParser.computeDefaultName(fromPath: path)
         case .remoteSourceControl(let url):
-            self.deprecatedName = name ?? LegacyPackageIdentity.computeDefaultName(fromURL: url)
+            self.deprecatedName = name ?? PackageIdentityParser.computeDefaultName(fromURL: url)
         case .registry(let identity):
             // FIXME: this is a placeholder
             self.deprecatedName = name ?? identity.description
@@ -154,7 +166,7 @@ extension PackageReference: Equatable {
 
     // TODO: consider rolling into Equatable
     public func equalsIncludingLocation(_ other: PackageReference) -> Bool {
-        return self.identity == other.identity && self.kind.locationString == other.kind.locationString
+        return self.identity == other.identity && self.canonicalLocation == other.canonicalLocation
     }
 }
 

Sources/SPMTestSupport/Observability.swift

@@ -90,7 +90,17 @@ public func testDiagnostics(
     line: UInt = #line,
     handler: (DiagnosticsTestResult) throws -> Void
 ) {
-    let diagnostics = problemsOnly ? diagnostics.filter({ $0.severity >= .warning }) : diagnostics
+    testDiagnostics(diagnostics, minSeverity: problemsOnly ? .warning : .debug, file: file, line: line, handler: handler)
+}
+
+public func testDiagnostics(
+    _ diagnostics: [Basics.Diagnostic],
+    minSeverity: Basics.Diagnostic.Severity,
+    file: StaticString = #file,
+    line: UInt = #line,
+    handler: (DiagnosticsTestResult) throws -> Void
+) {
+    let diagnostics = diagnostics.filter{ $0.severity >= minSeverity }
     let testResult = DiagnosticsTestResult(diagnostics)
 
     do {