refactor workspace initializers (#3664)

motivation: nicer API to work with

changes:
* deprecate previous initializer
* introduce new initializer with minimal signatures, and clearly defined argument names and API docs
* make DependencyMirrors thread safe
* adjust call sites

Author: tomerd

Sources/Commands/APIDigester.swift

@@ -33,12 +33,6 @@ struct APIDigesterBaselineDumper {
     /// The input build parameters.
     let inputBuildParameters: BuildParameters
 
-    /// The manifest loader.
-    let manifestLoader: ManifestLoaderProtocol
-
-    /// The repository manager.
-    let repositoryManager: RepositoryManager
-
     /// The API digester tool.
     let apiDigesterTool: SwiftAPIDigester
 
@@ -49,16 +43,12 @@ struct APIDigesterBaselineDumper {
         baselineRevision: Revision,
         packageRoot: AbsolutePath,
         buildParameters: BuildParameters,
-        manifestLoader: ManifestLoaderProtocol,
-        repositoryManager: RepositoryManager,
         apiDigesterTool: SwiftAPIDigester,
         diags: DiagnosticsEngine
     ) {
         self.baselineRevision = baselineRevision
         self.packageRoot = packageRoot
         self.inputBuildParameters = buildParameters
-        self.manifestLoader = manifestLoader
-        self.repositoryManager = repositoryManager
         self.apiDigesterTool = apiDigesterTool
         self.diags = diags
     }
@@ -93,8 +83,9 @@ struct APIDigesterBaselineDumper {
         }
 
         // Clone the current package in a sandbox and checkout the baseline revision.
+        let repositoryProvider = GitRepositoryProvider()
         let specifier = RepositorySpecifier(url: baselinePackageRoot.pathString)
-        let workingCopy = try repositoryManager.provider.createWorkingCopy(
+        let workingCopy = try repositoryProvider.createWorkingCopy(
             repository: specifier,
             sourcePath: packageRoot,
             at: baselinePackageRoot,
@@ -105,9 +96,7 @@ struct APIDigesterBaselineDumper {
 
         // Create the workspace for this package.
         let workspace = try Workspace(
-            forRootPackage: baselinePackageRoot,
-            manifestLoader: manifestLoader,
-            repositoryManager: repositoryManager
+            forRootPackage: baselinePackageRoot
         )
 
         let graph = try workspace.loadPackageGraph(
@@ -123,7 +112,7 @@ struct APIDigesterBaselineDumper {
 
         // Update the data path input build parameters so it's built in the sandbox.
         var buildParameters = inputBuildParameters
-        buildParameters.dataPath = workspace.dataPath
+        buildParameters.dataPath = workspace.location.workingDirectory
 
         // Build the baseline module.
         let buildOp = BuildOperation(

Sources/Commands/SwiftPackageTool.swift

@@ -364,13 +364,10 @@ extension SwiftPackageTool {
             try buildOp.build()
 
             // Dump JSON for the baseline package.
-            let workspace = try swiftTool.getActiveWorkspace()
             let baselineDumper = try APIDigesterBaselineDumper(
                 baselineRevision: baselineRevision,
                 packageRoot: swiftTool.getPackageRoot(),
                 buildParameters: buildOp.buildParameters,
-                manifestLoader: workspace.manifestLoader,
-                repositoryManager: workspace.repositoryManager,
                 apiDigesterTool: apiDigesterTool,
                 diags: swiftTool.diagnostics
             )
@@ -886,7 +883,7 @@ extension SwiftPackageTool.Config {
         var mirrorURL: String
 
         func run(_ swiftTool: SwiftTool) throws {
-            let config = try swiftTool.getSwiftPMConfig()
+            let config = try swiftTool.getMirrorsConfig()
 
             if packageURL != nil {
                 swiftTool.diagnostics.emit(
@@ -920,7 +917,7 @@ extension SwiftPackageTool.Config {
         var mirrorURL: String?
 
         func run(_ swiftTool: SwiftTool) throws {
-            let config = try swiftTool.getSwiftPMConfig()
+            let config = try swiftTool.getMirrorsConfig()
 
             if packageURL != nil {
                 swiftTool.diagnostics.emit(
@@ -951,7 +948,7 @@ extension SwiftPackageTool.Config {
         var originalURL: String?
 
         func run(_ swiftTool: SwiftTool) throws {
-            let config = try swiftTool.getSwiftPMConfig()
+            let config = try swiftTool.getMirrorsConfig()
 
             if packageURL != nil {
                 swiftTool.diagnostics.emit(

Sources/Commands/SwiftTool.swift

@@ -459,14 +459,14 @@ public class SwiftTool {
         return try getPackageRoot().appending(component: "Packages")
     }
 
-    func resolvedFilePath() throws -> AbsolutePath {
+    func resolvedVersionsFilePath() throws -> AbsolutePath {
         if let multiRootPackageDataFile = options.multirootPackageDataFile {
             return multiRootPackageDataFile.appending(components: "xcshareddata", "swiftpm", "Package.resolved")
         }
         return try getPackageRoot().appending(component: "Package.resolved")
     }
 
-    func configFilePath() throws -> AbsolutePath {
+    func mirrorsConfigFilePath() throws -> AbsolutePath {
         // Look for the override in the environment.
         if let envPath = ProcessEnv.vars["SWIFTPM_MIRROR_CONFIG"] {
             return try AbsolutePath(validating: envPath)
@@ -479,15 +479,15 @@ public class SwiftTool {
         return try getPackageRoot().appending(components: ".swiftpm", "config")
     }
 
-    func getSwiftPMConfig() throws -> Workspace.Configuration {
-        return try _swiftpmConfig.get()
+    func getMirrorsConfig() throws -> Workspace.Configuration {
+        return try _mirrorsConfig.get()
     }
 
-    private lazy var _swiftpmConfig: Result<Workspace.Configuration, Swift.Error> = {
-        return Result(catching: { try Workspace.Configuration(path: try configFilePath()) })
+    private lazy var _mirrorsConfig: Result<Workspace.Configuration, Swift.Error> = {
+        return Result(catching: { try Workspace.Configuration(path: try mirrorsConfigFilePath(), fileSystem: localFileSystem) })
     }()
-    
-    func resolvedNetrcFilePath() throws -> AbsolutePath? {
+
+    func netrcFilePath() throws -> AbsolutePath? {
         guard options.netrc ||
                 options.netrcFilePath != nil ||
                 options.netrcOptional else { return nil }
@@ -551,21 +551,23 @@ public class SwiftTool {
         let cachePath = self.options.useRepositoriesCache ? try self.getCachePath() : .none
         _  = try self.getConfigPath() // TODO: actually use this in the workspace 
         let isXcodeBuildSystemEnabled = self.options.buildSystem == .xcode
-        let workspace = Workspace(
-            dataPath: buildPath,
-            editablesPath: try editablesPath(),
-            pinsFile: try resolvedFilePath(),
-            manifestLoader: try getManifestLoader(),
-            toolsVersionLoader: ToolsVersionLoader(),
-            delegate: delegate,
-            config: try getSwiftPMConfig(),
-            repositoryProvider: provider,
-            netrcFilePath: try resolvedNetrcFilePath(),
+        let workspace = try Workspace(
+            fileSystem: localFileSystem,
+            location: .init(
+                workingDirectory: buildPath,
+                editsDirectory: try editablesPath(),
+                resolvedVersionsFilePath: try resolvedVersionsFilePath(),
+                sharedCacheDirectory: cachePath
+            ),
+            netrcFilePath: try netrcFilePath(),
+            mirrors: self.getMirrorsConfig().mirrors,
+            customManifestLoader: try getManifestLoader(), // FIXME: doe we really need to customize it?
+            customRepositoryProvider: provider, // FIXME: doe we really need to customize it?
             additionalFileRules: isXcodeBuildSystemEnabled ? FileRuleDescription.xcbuildFileTypes : FileRuleDescription.swiftpmFileTypes,
-            isResolverPrefetchingEnabled: options.shouldEnableResolverPrefetching,
-            skipUpdate: options.skipDependencyUpdate,
-            enableResolverTrace: options.enableResolverTrace,
-            cachePath: cachePath
+            resolverUpdateEnabled: !options.skipDependencyUpdate,
+            resolverPrefetchingEnabled: options.shouldEnableResolverPrefetching,
+            resolverTracingEnabled: options.enableResolverTrace,
+            delegate: delegate
         )
         _workspace = workspace
         _workspaceDelegate = delegate
@@ -638,7 +640,7 @@ public class SwiftTool {
             // The `plugins` directory is inside the workspace's main data directory, and contains all temporary
             // files related to all plugins in the workspace.
             let buildEnvironment = try buildParameters().buildEnvironment
-            let dataDir = try self.getActiveWorkspace().dataPath
+            let dataDir = try self.getActiveWorkspace().location.workingDirectory
             let pluginsDir = dataDir.appending(component: "plugins")
 
             // The `cache` directory is in the plugins directory and is where the plugin script runner caches

Sources/PackageGraph/DependencyMirrors.swift

@@ -24,6 +24,7 @@ public final class DependencyMirrors {
 
     private var index: [String: String]
     private var reverseIndex: [String: String]
+    private let lock = Lock()
 
     private init(_ mirrors: [Mirror]) {
         self.index = Dictionary(mirrors.map({ ($0.original, $0.mirror) }), uniquingKeysWith: { first, _ in first })
@@ -32,30 +33,36 @@ public final class DependencyMirrors {
 
     /// Sets a mirror URL for the given URL.
     public func set(mirrorURL: String, forURL url: String) {
-        self.index[url] = mirrorURL
-        self.reverseIndex[mirrorURL] = url
+        self.lock.withLock {
+            self.index[url] = mirrorURL
+            self.reverseIndex[mirrorURL] = url
+        }
     }
 
     /// Unsets a mirror for the given URL.
     /// - Parameter originalOrMirrorURL: The original URL or the mirrored URL
     /// - Throws: `Error.mirrorNotFound` if no mirror exists for the provided URL.
     public func unset(originalOrMirrorURL: String) throws {
-        if let value = self.index[originalOrMirrorURL] {
-            self.index[originalOrMirrorURL] = nil
-            self.reverseIndex[value] = nil
-        } else if let mirror = self.index.first(where: { $0.value == originalOrMirrorURL }) {
-            self.index[mirror.key] = nil
-            self.reverseIndex[originalOrMirrorURL] = nil
-        } else {
-            throw Error.mirrorNotFound
+        try self.lock.withLock {
+            if let value = self.index[originalOrMirrorURL] {
+                self.index[originalOrMirrorURL] = nil
+                self.reverseIndex[value] = nil
+            } else if let mirror = self.index.first(where: { $0.value == originalOrMirrorURL }) {
+                self.index[mirror.key] = nil
+                self.reverseIndex[originalOrMirrorURL] = nil
+            } else {
+                throw Error.mirrorNotFound
+            }
         }
     }
 
     /// Returns the mirrored URL for a package dependency URL.
     /// - Parameter url: The original URL
     /// - Returns: The mirrored URL, if one exists.
     public func mirrorURL(for url: String) -> String? {
-        return self.index[url]
+        self.lock.withLock {
+            return self.index[url]
+        }
     }
 
     /// Returns the effective URL for a package dependency URL.
@@ -69,29 +76,38 @@ public final class DependencyMirrors {
     /// - Parameter url: The mirror URL
     /// - Returns: The original URL, if one exists.
     public func originalURL(for url: String) -> String? {
-        return self.reverseIndex[url]
+        self.lock.withLock {
+            return self.reverseIndex[url]
+        }
     }
-
 }
 
 extension DependencyMirrors: Collection {
     public typealias Index = Dictionary<String, String>.Index
     public typealias Element = String
 
     public var startIndex: Index {
-        self.index.startIndex
+        self.lock.withLock {
+            self.index.startIndex
+        }
     }
 
     public var endIndex: Index {
-        self.index.endIndex
+        self.lock.withLock {
+            self.index.endIndex
+        }
     }
 
     public subscript(index: Index) -> Element {
-        self.index[index].value
+        self.lock.withLock {
+            self.index[index].value
+        }
     }
 
     public func index(after index: Index) -> Index {
-        self.index.index(after: index)
+        self.lock.withLock {
+            self.index.index(after: index)
+        }
     }
 }
 

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

@@ -102,30 +102,30 @@ public struct PubgrubDependencyResolver {
     /// The container provider used to load package containers.
     private let provider: ContainerProvider
 
-    /// Skip updating containers while fetching them.
-    private let skipUpdate: Bool
-
     /// Reference to the package container provider.
     private let packageContainerProvider: PackageContainerProvider
 
     /// Should resolver prefetch the containers.
-    private let isPrefetchingEnabled: Bool
+    private let prefetchingEnabled: Bool
+
+    /// Update containers while fetching them.
+    private let updateEnabled: Bool
 
     /// Resolver delegate
     private let delegate: DependencyResolverDelegate?
 
     public init(
         provider: PackageContainerProvider,
         pinsMap: PinsStore.PinsMap = [:],
-        isPrefetchingEnabled: Bool = false,
-        skipUpdate: Bool = false,
+        updateEnabled: Bool = true,
+        prefetchingEnabled: Bool = false,
         delegate: DependencyResolverDelegate? = nil
     ) {
         self.packageContainerProvider = provider
         self.pinsMap = pinsMap
-        self.isPrefetchingEnabled = isPrefetchingEnabled
-        self.skipUpdate = skipUpdate
-        self.provider = ContainerProvider(provider: self.packageContainerProvider, skipUpdate: self.skipUpdate, pinsMap: self.pinsMap)
+        self.updateEnabled = updateEnabled
+        self.prefetchingEnabled = prefetchingEnabled
+        self.provider = ContainerProvider(provider: self.packageContainerProvider, updateEnabled: self.updateEnabled, pinsMap: self.pinsMap)
         self.delegate = delegate
     }
 
@@ -170,7 +170,7 @@ public struct PubgrubDependencyResolver {
         let inputs = try self.processInputs(root: root, with: constraints)
 
         // Prefetch the containers if prefetching is enabled.
-        if self.isPrefetchingEnabled {
+        if self.prefetchingEnabled {
             // We avoid prefetching packages that are overridden since
             // otherwise we'll end up creating a repository container
             // for them.
@@ -1357,8 +1357,8 @@ private final class ContainerProvider {
     /// The actual package container provider.
     private let underlying: PackageContainerProvider
 
-    /// Wheather to perform update (git fetch) on existing cloned repositories or not.
-    private let skipUpdate: Bool
+    /// Whether to perform update (git fetch) on existing cloned repositories or not.
+    private let updateEnabled: Bool
 
     /// Reference to the pins store.
     private let pinsMap: PinsStore.PinsMap
@@ -1369,9 +1369,9 @@ private final class ContainerProvider {
     //// Store prefetches synchronization
     private var prefetches = ThreadSafeKeyValueStore<PackageReference, DispatchGroup>()
 
-    init(provider underlying: PackageContainerProvider, skipUpdate: Bool, pinsMap: PinsStore.PinsMap) {
+    init(provider underlying: PackageContainerProvider, updateEnabled: Bool, pinsMap: PinsStore.PinsMap) {
         self.underlying = underlying
-        self.skipUpdate = skipUpdate
+        self.updateEnabled = updateEnabled
         self.pinsMap = pinsMap
     }
 
@@ -1404,7 +1404,7 @@ private final class ContainerProvider {
             }
         } else {
             // Otherwise, fetch the container from the provider
-            self.underlying.getContainer(for: package, skipUpdate: skipUpdate, on: .sharedConcurrent) { result in
+            self.underlying.getContainer(for: package, skipUpdate: !self.updateEnabled, on: .sharedConcurrent) { result in
                 let result = result.tryMap { container -> PubGrubPackageContainer in
                     let pubGrubContainer = PubGrubPackageContainer(underlying: container, pinsMap: self.pinsMap)
                     // only cache positive results
@@ -1428,7 +1428,7 @@ private final class ContainerProvider {
                 return group
             }
             if needsFetching {
-                self.underlying.getContainer(for: identifier, skipUpdate: skipUpdate, on: .sharedConcurrent) { result in
+                self.underlying.getContainer(for: identifier, skipUpdate: !self.updateEnabled, on: .sharedConcurrent) { result in
                     defer { self.prefetches[identifier]?.leave() }
                     // only cache positive results
                     if case .success(let container) = result {