refactor managed artifacts (#3699)

motivation: ManagedArtifacts are based on location instead of on identity, refactor them to be based on identity as part of the transition to identity based logic

changes:
* transition ManagedArtifacts underlying hashmap to be based on identity
* update call sites to use package identity instead of location when reading/writing managed artifacts
* adjust tests where needed

Author: tomerd

Sources/SPMTestSupport/MockWorkspace.swift

@@ -379,7 +379,7 @@ public final class MockWorkspace {
     public func set(
         pins: [PackageReference: CheckoutState] = [:],
         managedDependencies: [ManagedDependency] = [],
-        managedArtifacts: [ManagedArtifact] = []
+        managedArtifacts: [Workspace.ManagedArtifact] = []
     ) throws {
         let workspace = try self.getOrCreateWorkspace()
         let pinsStore = try workspace.pinsStore.load()
@@ -463,36 +463,44 @@ public final class MockWorkspace {
     }
 
     public struct ManagedArtifactResult {
-        public let managedArtifacts: ManagedArtifacts
+        public let managedArtifacts: Workspace.ManagedArtifacts
 
-        public init(_ managedArtifacts: ManagedArtifacts) {
+        public init(_ managedArtifacts: Workspace.ManagedArtifacts) {
             self.managedArtifacts = managedArtifacts
         }
 
+        public func checkNotPresent(packageName: String, targetName: String, file: StaticString = #file, line: UInt = #line) {
+            self.checkNotPresent(packageIdentity: .plain(packageName), targetName: targetName, file : file, line: line)
+        }
+
         public func checkNotPresent(
-            packageName: String,
+            packageIdentity: PackageIdentity,
             targetName: String,
             file: StaticString = #file,
             line: UInt = #line
         ) {
-            let artifact = self.managedArtifacts[packageName: packageName, targetName: targetName]
-            XCTAssert(artifact == nil, "Unexpectedly found \(packageName).\(targetName) in managed artifacts", file: file, line: line)
+            let artifact = self.managedArtifacts[packageIdentity: packageIdentity, targetName: targetName]
+            XCTAssert(artifact == nil, "Unexpectedly found \(packageIdentity).\(targetName) in managed artifacts", file: file, line: line)
         }
 
         public func checkEmpty(file: StaticString = #file, line: UInt = #line) {
             XCTAssertEqual(self.managedArtifacts.count, 0, file: file, line: line)
         }
 
+        public func check(packageName: String, targetName: String, source: Workspace.ManagedArtifact.Source, path: AbsolutePath, file: StaticString = #file, line: UInt = #line) {
+            self.check(packageIdentity: .plain(packageName), targetName: targetName, source: source, path: path, file: file, line: line)
+        }
+
         public func check(
-            packageName: String,
+            packageIdentity: PackageIdentity,
             targetName: String,
-            source: ManagedArtifact.Source,
+            source: Workspace.ManagedArtifact.Source,
             path: AbsolutePath,
             file: StaticString = #file,
             line: UInt = #line
         ) {
-            guard let artifact = managedArtifacts[packageName: packageName, targetName: targetName] else {
-                XCTFail("\(packageName).\(targetName) does not exists", file: file, line: line)
+            guard let artifact = managedArtifacts[packageIdentity: packageIdentity, targetName: targetName] else {
+                XCTFail("\(packageIdentity).\(targetName) does not exists", file: file, line: line)
                 return
             }
             XCTAssertEqual(artifact.path, path)

Sources/Workspace/ManagedArtifact.swift

@@ -14,83 +14,83 @@ import SourceControl
 import TSCBasic
 import TSCUtility
 
-/// A downloaded artifact managed by the workspace.
-public struct ManagedArtifact {
-    /// The package reference.
-    public let packageRef: PackageReference
-
-    /// The name of the binary target the artifact corresponds to.
-    public let targetName: String
-
-    /// The source of the artifact (local or remote).
-    public let source: Source
-
-    /// The path of the artifact on disk
-    public let path: AbsolutePath
-
-    public init(
-        packageRef: PackageReference,
-        targetName: String,
-        source: Source,
-        path: AbsolutePath
-    ) {
-        self.packageRef = packageRef
-        self.targetName = targetName
-        self.source = source
-        self.path = path
-    }
+extension Workspace {
+    /// A downloaded artifact managed by the workspace.
+    public struct ManagedArtifact {
+        /// The package reference.
+        public let packageRef: PackageReference
+
+        /// The name of the binary target the artifact corresponds to.
+        public let targetName: String
+
+        /// The source of the artifact (local or remote).
+        public let source: Source
+
+        /// The path of the artifact on disk
+        public let path: AbsolutePath
+
+        public init(
+            packageRef: PackageReference,
+            targetName: String,
+            source: Source,
+            path: AbsolutePath
+        ) {
+            self.packageRef = packageRef
+            self.targetName = targetName
+            self.source = source
+            self.path = path
+        }
 
-    /// Create an artifact downloaded from a remote url.
-    public static func remote(
-        packageRef: PackageReference,
-        targetName: String,
-        url: String,
-        checksum: String,
-        path: AbsolutePath
-    ) -> ManagedArtifact {
-        return ManagedArtifact(
-            packageRef: packageRef,
-            targetName: targetName,
-            source: .remote(url: url, checksum: checksum),
-            path: path
-        )
-    }
+        /// Create an artifact downloaded from a remote url.
+        public static func remote(
+            packageRef: PackageReference,
+            targetName: String,
+            url: String,
+            checksum: String,
+            path: AbsolutePath
+        ) -> ManagedArtifact {
+            return ManagedArtifact(
+                packageRef: packageRef,
+                targetName: targetName,
+                source: .remote(url: url, checksum: checksum),
+                path: path
+            )
+        }
 
-    /// Create an artifact present locally on the filesystem.
-    public static func local(
-        packageRef: PackageReference,
-        targetName: String,
-        path: AbsolutePath
-    ) -> ManagedArtifact {
-        return ManagedArtifact(
-            packageRef: packageRef,
-            targetName: targetName,
-            source: .local,
-            path: path
-        )
-    }
+        /// Create an artifact present locally on the filesystem.
+        public static func local(
+            packageRef: PackageReference,
+            targetName: String,
+            path: AbsolutePath
+        ) -> ManagedArtifact {
+            return ManagedArtifact(
+                packageRef: packageRef,
+                targetName: targetName,
+                source: .local,
+                path: path
+            )
+        }
 
-    /// Represents the source of the artifact.
-    public enum Source: Equatable {
+        /// Represents the source of the artifact.
+        public enum Source: Equatable {
 
-        /// Represents a remote artifact, with the url it was downloaded from, its checksum, and its path relative to
-        /// the workspace artifacts path.
-        case remote(url: String, checksum: String)
+            /// Represents a remote artifact, with the url it was downloaded from, its checksum, and its path relative to
+            /// the workspace artifacts path.
+            case remote(url: String, checksum: String)
 
-        /// Represents a locally available artifact, with its path relative to its package.
-        case local
+            /// Represents a locally available artifact, with its path relative to its package.
+            case local
+        }
     }
 }
 
-// MARK: - CustomStringConvertible
-
-extension ManagedArtifact: CustomStringConvertible {
+extension Workspace.ManagedArtifact: CustomStringConvertible {
     public var description: String {
         return "<ManagedArtifact: \(self.packageRef.name).\(self.targetName) \(self.source) \(self.path)>"
     }
 }
 
-extension ManagedArtifact.Source: CustomStringConvertible {
+extension Workspace.ManagedArtifact.Source: CustomStringConvertible {
     public var description: String {
         switch self {
         case .local:
@@ -101,63 +101,59 @@ extension ManagedArtifact.Source: CustomStringConvertible {
     }
 }
 
-// MARK: -
-
-/// A collection of managed artifacts which have been downloaded.
-public final class ManagedArtifacts {
+// MARK: - ManagedArtifacts
 
-    /// A mapping from package url, to target name, to ManagedArtifact.
-    private var artifactMap: [String: [String: ManagedArtifact]]
-
-    internal var artifacts: AnyCollection<ManagedArtifact> {
-        AnyCollection(artifactMap.values.lazy.flatMap({ $0.values }))
-    }
+extension Workspace {
+    /// A collection of managed artifacts which have been downloaded.
+    public final class ManagedArtifacts {
+        /// A mapping from package identity, to target name, to ManagedArtifact.
+        private var artifactMap: [PackageIdentity: [String: ManagedArtifact]]
 
-    init(artifactMap: [String: [String: ManagedArtifact]] = [:]) {
-        self.artifactMap = artifactMap
-    }
+        internal var artifacts: AnyCollection<ManagedArtifact> {
+            AnyCollection(self.artifactMap.values.lazy.flatMap{ $0.values })
+        }
 
-    public subscript(packageURL packageURL: String, targetName targetName: String) -> ManagedArtifact? {
-        artifactMap[packageURL]?[targetName]
-    }
+        init(_ artifacts: [ManagedArtifact] = []) {
+            let artifactsByPackagePath = Dictionary(grouping: artifacts, by: { $0.packageRef.identity })
+            self.artifactMap = artifactsByPackagePath.mapValues{ artifacts in
+                Dictionary(uniqueKeysWithValues: artifacts.map{ ($0.targetName, $0) })
+            }
+        }
 
-    public subscript(packageName packageName: String, targetName targetName: String) -> ManagedArtifact? {
-        artifacts.first(where: { $0.packageRef.name == packageName && $0.targetName == targetName })
-    }
+        public subscript(packageIdentity packageIdentity: PackageIdentity, targetName targetName: String) -> ManagedArtifact? {
+            self.artifactMap[packageIdentity]?[targetName]
+        }
 
-    public func add(_ artifact: ManagedArtifact) {
-        artifactMap[artifact.packageRef.location, default: [:]][artifact.targetName] = artifact
-    }
+        public func add(_ artifact: ManagedArtifact) {
+            self.artifactMap[artifact.packageRef.identity, default: [:]][artifact.targetName] = artifact
+        }
 
-    public func remove(packageURL: String, targetName: String) {
-        artifactMap[packageURL]?[targetName] = nil
+        public func remove(packageIdentity: PackageIdentity, targetName: String) {
+            self.artifactMap[packageIdentity]?[targetName] = nil
+        }
     }
 }
 
-// MARK: - Collection
-
-extension ManagedArtifacts: Collection {
+extension Workspace.ManagedArtifacts: Collection {
     public var startIndex: AnyIndex {
-        artifacts.startIndex
+        self.artifacts.startIndex
     }
 
     public var endIndex: AnyIndex {
-        artifacts.endIndex
+        self.artifacts.endIndex
     }
 
-    public subscript(index: AnyIndex) -> ManagedArtifact {
-        artifacts[index]
+    public subscript(index: AnyIndex) -> Workspace.ManagedArtifact {
+        self.artifacts[index]
     }
 
     public func index(after index: AnyIndex) -> AnyIndex {
-        artifacts.index(after: index)
+        self.artifacts.index(after: index)
     }
 }
 
-// MARK: - CustomStringConvertible
-
-extension ManagedArtifacts: CustomStringConvertible {
+extension Workspace.ManagedArtifacts: CustomStringConvertible {
     public var description: String {
-        "<ManagedArtifacts: \(Array(artifacts))>"
+        "<ManagedArtifacts: \(Array(self.artifacts))>"
     }
 }

Sources/Workspace/Workspace.swift

@@ -1672,8 +1672,8 @@ extension Workspace {
         }
 
         for artifact in manifestArtifacts.local {
-            let existingArtifact = state.artifacts[
-                packageURL: artifact.packageRef.location,
+            let existingArtifact = self.state.artifacts[
+                packageIdentity: artifact.packageRef.identity,
                 targetName: artifact.targetName
             ]
 
@@ -1686,8 +1686,8 @@ extension Workspace {
         }
 
         for artifact in manifestArtifacts.remote {
-            let existingArtifact = state.artifacts[
-                packageURL: artifact.packageRef.location,
+            let existingArtifact = self.state.artifacts[
+                packageIdentity: artifact.packageRef.identity,
                 targetName: artifact.targetName
             ]
 
@@ -1712,7 +1712,7 @@ extension Workspace {
         // Remove the artifacts and directories which are not needed anymore.
         diagnostics.wrap {
             for artifact in artifactsToRemove {
-                state.artifacts.remove(packageURL: artifact.packageRef.location, targetName: artifact.targetName)
+                state.artifacts.remove(packageIdentity: artifact.packageRef.identity, targetName: artifact.targetName)
 
                 if case .remote = artifact.source {
                     try fileSystem.removeFileTree(artifact.path)
@@ -2878,7 +2878,7 @@ private struct ArchiveIndexFile: Decodable {
     }
 }
 
-private extension ManagedArtifact {
+private extension Workspace.ManagedArtifact {
     var originURL: String? {
         switch self.source {
         case .remote(let url, _):