* rename CanonicalPackageIdentity as CanonicalPackageLocation and use it to compare locations where required

* rename LegacyPackageIdentity as PackageIdentityParser and simplify how identity parsing is wired (removed unusued complexity)

Author: tomerd

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.packageKind.canonicalLocation != 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
@@ -291,7 +277,7 @@ private func createResolvedPackages(
                     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 +288,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 +660,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/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 {
     /// 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.caseInsensitiveCompare(other.kind.locationString) == .orderedSame
+        return self.identity == other.identity && self.canonicalLocation == other.canonicalLocation
     }
 }
 

Sources/Workspace/Workspace.swift

@@ -1740,13 +1740,13 @@ extension Workspace {
             let dependenciesToLoad = dependenciesRequired.map{ $0.createPackageRef() }.filter { !loadedManifests.keys.contains($0.identity) }
             let dependenciesManifests = try temp_await { self.loadManagedManifests(for: dependenciesToLoad, observabilityScope: observabilityScope, completion: $0) }
             dependenciesManifests.forEach { loadedManifests[$0.key] = $0.value }
-            return pair.item.dependenciesRequired(for: pair.key.productFilter).compactMap{ dependency in
+            return pair.item.dependenciesRequired(for: pair.key.productFilter).compactMap { dependency in
                 loadedManifests[dependency.identity].flatMap {
                     // we also compare the location as this function may attempt to load
                     // dependencies that have the same identity but from a different location
                     // which is an error case we diagnose an report about in the GraphLoading part which
                     // is prepared to handle the case where not all manifest are available
-                    $0.packageLocation.caseInsensitiveCompare(dependency.locationString) == .orderedSame ?
+                    $0.packageKind.canonicalLocation == dependency.createPackageRef().canonicalLocation ?
                     KeyedPair($0, key: Key(identity: dependency.identity, productFilter: dependency.productFilter)) : nil
                 }
             }
@@ -3372,9 +3372,8 @@ extension Workspace {
         try workingCopy.checkout(revision: checkoutState.revision)
         try? fileSystem.chmod(.userUnWritable, path: checkoutPath, options: [.recursive, .onlyFiles])
 
-        print("********************* adding \(package.locationString) to managed deps")
-
-        // Write the state record.
+        // Record the new state.
+        observabilityScope.emit(debug: "adding '\(package.identity)' (\(package.locationString)) to managed dependencies")
         self.state.dependencies.add(
             try .sourceControlCheckout(
                 packageRef: package,
@@ -3532,8 +3531,8 @@ extension Workspace {
              )
          }
 
-         // TODO: make download read-only?
-
+         // Record the new state.
+         observabilityScope.emit(debug: "adding '\(package.identity)' (\(package.locationString)) to managed dependencies")
          self.state.dependencies.add(
             try .registryDownload(
                 packageRef: package,