deprecate static APIs with inefficient defaults

tomerd · tomerd · commit bfa5ae9fe5e2 · 2021-08-10T11:09:20.000-07:00
motivation: configuration work has pointed out that some of the static APIs (which are questionable to begin with) use inefficient defaults

changes:
* deprecate Workspace.create,  Workspace.loadRootGraph, PackageBuilder.loadRootPackage, ManifestLoader.loadRootManifest and replace them with instance methods on workspace
* add a simple constructor to workspace, that uses the host toolchain by default
* adjust call-sites, test and examples
diff --git a/Examples/package-info/Package.swift b/Examples/package-info/Package.swift
@@ -1,8 +1,13 @@
-// swift-tools-version:4.2
+// swift-tools-version:5.1
+
 import PackageDescription
 
 let package = Package(
     name: "package-info",
+    platforms: [
+        .macOS(.v10_15),
+        .iOS(.v13)
+    ],
     dependencies: [
         // This just points to the SwiftPM at the root of this repository.
         .package(path: "../../"),
diff --git a/Examples/package-info/Sources/package-info/main.swift b/Examples/package-info/Sources/package-info/main.swift
@@ -6,35 +6,29 @@ import Workspace
 // PREREQUISITES
 // ============
 
-// We will need to know where the Swift compiler is.
-let swiftCompiler: AbsolutePath = {
-    let string: String
-    #if os(macOS)
-    string = try! Process.checkNonZeroExit(args: "xcrun", "--sdk", "macosx", "-f", "swiftc").spm_chomp()
-    #else
-    string = try! Process.checkNonZeroExit(args: "which", "swiftc").spm_chomp()
-    #endif
-    return AbsolutePath(string)
-}()
-
 // We need a package to work with.
-// This assumes there is one in the current working directory:
-let packagePath = localFileSystem.currentWorkingDirectory!
+// This computes the path of this package root based on the file location
+let packagePath = AbsolutePath(#file).parentDirectory.parentDirectory.parentDirectory
 
 // LOADING
 // =======
 
-// Note:
-// This simplified API has been added since 0.4.0 was released.
-// See older revisions for examples that work with 0.4.0.
-
 // There are several levels of information available.
 // Each takes longer to load than the level above it, but provides more detail.
 let diagnostics = DiagnosticsEngine()
-let identityResolver = DefaultIdentityResolver()
-let manifest = try tsc_await { ManifestLoader.loadRootManifest(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, on: .global(), completion: $0) }
-let loadedPackage = try tsc_await { PackageBuilder.loadRootPackage(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, diagnostics: diagnostics, on: .global(), completion: $0) }
-let graph = try Workspace.loadRootGraph(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, diagnostics: diagnostics)
+let workspace = try Workspace(forRootPackage: packagePath)
+let manifest = try tsc_await { workspace.loadRootManifest(at: packagePath, diagnostics: diagnostics, completion: $0) }
+
+let package = try tsc_await { workspace.loadRootPackage(at: packagePath, diagnostics: diagnostics, completion: $0) }
+guard !diagnostics.hasErrors else {
+    fatalError("error package manifest: \(diagnostics)")
+}
+
+let graph = try workspace.loadPackageGraph(rootPath: packagePath, diagnostics: diagnostics)
+guard !diagnostics.hasErrors else {
+    fatalError("error loading package dependencies: \(diagnostics)")
+}
+
 
 // EXAMPLES
 // ========
@@ -46,7 +40,7 @@ let targets = manifest.targets.map({ $0.name }).joined(separator: ", ")
 print("Targets:", targets)
 
 // Package
-let executables = loadedPackage.targets.filter({ $0.type == .executable }).map({ $0.name })
+let executables = package.targets.filter({ $0.type == .executable }).map({ $0.name })
 print("Executable targets:", executables)
 
 // PackageGraph
diff --git a/Sources/Commands/APIDigester.swift b/Sources/Commands/APIDigester.swift
@@ -104,7 +104,7 @@ struct APIDigesterBaselineDumper {
         try workingCopy.checkout(revision: baselineRevision)
 
         // Create the workspace for this package.
-        let workspace = Workspace.create(
+        let workspace = try Workspace(
             forRootPackage: baselinePackageRoot,
             manifestLoader: manifestLoader,
             repositoryManager: repositoryManager
diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift
@@ -140,6 +140,8 @@ public final class ManifestLoader: ManifestLoaderProtocol {
     ///   - diagnostics: Optional.  The diagnostics engine.
     ///   - on: The dispatch queue to perform asynchronous operations on.
     ///   - completion: The completion handler .
+    // deprecated 8/2021
+    @available(*, deprecated, message: "use workspace API instead")
     public static func loadRootManifest(
         at path: AbsolutePath,
         swiftCompiler: AbsolutePath,
@@ -151,7 +153,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
         completion: @escaping (Result<Manifest, Error>) -> Void
     ) {
         do {
-            let toolchain = try ToolchainConfiguration(swiftCompiler: swiftCompiler, swiftCompilerFlags: swiftCompilerFlags)
+            let toolchain = ToolchainConfiguration(swiftCompiler: swiftCompiler, swiftCompilerFlags: swiftCompilerFlags)
             let loader = ManifestLoader(toolchain: toolchain)
             let toolsVersion = try ToolsVersionLoader().load(at: path, fileSystem: fileSystem)
             let packageLocation = fileSystem.isFile(path) ? path.parentDirectory : path
diff --git a/Sources/PackageLoading/PackageBuilder.swift b/Sources/PackageLoading/PackageBuilder.swift
@@ -242,7 +242,7 @@ public final class PackageBuilder {
     /// Create the special REPL product for this package.
     private let createREPLProduct: Bool
 
-    /// The additionla file detection rules.
+    /// The additional file detection rules.
     private let additionalFileRules: [FileRuleDescription]
 
     /// Minimum deployment target of XCTest per platform.
@@ -296,6 +296,8 @@ public final class PackageBuilder {
     ///   - diagnostics: Optional.  The diagnostics engine.
     ///   - on: The dispatch queue to perform asynchronous operations on.
     ///   - completion: The completion handler .
+    // deprecated 8/2021
+    @available(*, deprecated, message: "use workspace API instead")
     public static func loadRootPackage(
         at path: AbsolutePath,
         swiftCompiler: AbsolutePath,
diff --git a/Sources/PackageModel/ToolchainConfiguration.swift b/Sources/PackageModel/ToolchainConfiguration.swift
@@ -35,35 +35,31 @@ public struct ToolchainConfiguration {
     /// XCTest Location
     public let xctestLocation: AbsolutePath?
 
+    /// Creates the set of manifest resources associated with a `swiftc` executable.
+    ///
+    /// - Parameters:
+    ///     - swiftCompiler: The absolute path of the associated `swiftc` executable.
+    ///     - swiftCompilerFlags: Extra flags to pass the Swift compiler.: Extra flags to pass the Swift compiler.
+    ///     - libDir: The path of the library resources.
+    ///     - binDir: The bin directory.
+    ///     - sdkRoot: The path to SDK root.
+    ///     - xctestLocation: XCTest Location
     public init(
         swiftCompiler: AbsolutePath,
-        swiftCompilerFlags: [String],
-        libDir: AbsolutePath,
+        swiftCompilerFlags: [String] = [],
+        libDir: AbsolutePath? = nil,
         binDir: AbsolutePath? = nil,
         sdkRoot: AbsolutePath? = nil,
         xctestLocation: AbsolutePath? = nil
     ) {
         self.swiftCompiler = swiftCompiler
         self.swiftCompilerFlags = swiftCompilerFlags
-        self.libDir = libDir
+        self.libDir = libDir ?? Self.libDir(forBinDir: swiftCompiler.parentDirectory)
         self.binDir = binDir
         self.sdkRoot = sdkRoot
         self.xctestLocation = xctestLocation
     }
 
-    /// Creates the set of manifest resources associated with a `swiftc` executable.
-    ///
-    /// - Parameters:
-    ///     - swiftCompiler: The absolute path of the associated `swiftc` executable.
-    public init(swiftCompiler: AbsolutePath, swiftCompilerFlags: [String]) throws {
-        let binDir = swiftCompiler.parentDirectory
-        self.init(
-            swiftCompiler: swiftCompiler,
-            swiftCompilerFlags: swiftCompilerFlags,
-            libDir: Self.libDir(forBinDir: binDir)
-        )
-    }
-
     public static func libDir(forBinDir binDir: AbsolutePath) -> AbsolutePath {
         return binDir.parentDirectory.appending(components: "lib", "swift", "pm")
     }
diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift
@@ -252,24 +252,44 @@ public class Workspace {
         pinsFile: AbsolutePath,
         manifestLoader: ManifestLoaderProtocol,
         repositoryManager: RepositoryManager? = nil,
-        currentToolsVersion: ToolsVersion = ToolsVersion.currentToolsVersion,
-        toolsVersionLoader: ToolsVersionLoaderProtocol = ToolsVersionLoader(),
+        currentToolsVersion: ToolsVersion? = nil,
+        toolsVersionLoader: ToolsVersionLoaderProtocol? = nil,
         delegate: WorkspaceDelegate? = nil,
-        config: Workspace.Configuration = Workspace.Configuration(),
-        fileSystem: FileSystem = localFileSystem,
-        repositoryProvider: RepositoryProvider = GitRepositoryProvider(),
+        config: Workspace.Configuration? = nil,
+        fileSystem: FileSystem? = nil,
+        repositoryProvider: RepositoryProvider? = nil,
         identityResolver: IdentityResolver? = nil,
-        httpClient: HTTPClient = HTTPClient(),
+        httpClient: HTTPClient? = nil,
         netrcFilePath: AbsolutePath? = nil,
-        archiver: Archiver = ZipArchiver(),
-        checksumAlgorithm: HashAlgorithm = SHA256(),
-        additionalFileRules: [FileRuleDescription] = [],
-        isResolverPrefetchingEnabled: Bool = false,
-        enablePubgrubResolver: Bool = false,
-        skipUpdate: Bool = false,
-        enableResolverTrace: Bool = false,
+        archiver: Archiver? = nil,
+        checksumAlgorithm: HashAlgorithm? = nil,
+        additionalFileRules: [FileRuleDescription]? = nil,
+        isResolverPrefetchingEnabled: Bool? = nil,
+        enablePubgrubResolver: Bool? = nil,
+        skipUpdate: Bool? = nil,
+        enableResolverTrace: Bool? = nil,
         cachePath: AbsolutePath? = nil
     ) {
+        // defaults
+        let currentToolsVersion = currentToolsVersion ?? ToolsVersion.currentToolsVersion
+        let toolsVersionLoader = toolsVersionLoader ?? ToolsVersionLoader()
+        let config = config ?? Workspace.Configuration()
+        let fileSystem = fileSystem ?? localFileSystem
+        let repositoryProvider = repositoryProvider ?? GitRepositoryProvider()
+        let httpClient = httpClient ?? HTTPClient()
+        let archiver = archiver ?? ZipArchiver()
+        var checksumAlgorithm = checksumAlgorithm ?? SHA256()
+        #if canImport(CryptoKit)
+        if checksumAlgorithm is SHA256, #available(macOS 10.15, *) {
+            checksumAlgorithm = CryptoKitSHA256()
+        }
+        #endif
+        let additionalFileRules = additionalFileRules ?? []
+        let isResolverPrefetchingEnabled = isResolverPrefetchingEnabled ?? false
+        let skipUpdate = skipUpdate ?? false
+        let enableResolverTrace = enableResolverTrace ?? false
+
+        // initialize
         self.delegate = delegate
         self.dataPath = dataPath
         self.config = config
@@ -281,12 +301,6 @@ public class Workspace {
         self.netrcFilePath = netrcFilePath
         self.archiver = archiver
 
-        var checksumAlgorithm = checksumAlgorithm
-        #if canImport(CryptoKit)
-        if checksumAlgorithm is SHA256, #available(macOS 10.15, *) {
-            checksumAlgorithm = CryptoKitSHA256()
-        }
-        #endif
         self.checksumAlgorithm = checksumAlgorithm
         self.isResolverPrefetchingEnabled = isResolverPrefetchingEnabled
         self.skipUpdate = skipUpdate
@@ -329,21 +343,66 @@ public class Workspace {
     ///
     /// The root package path is used to compute the build directory and other
     /// default paths.
-    public static func create(
+    public convenience init(
+        forRootPackage packagePath: AbsolutePath,
+        toolchain: UserToolchain? = nil,
+        repositoryManager: RepositoryManager? = nil,
+        delegate: WorkspaceDelegate? = nil
+    ) throws {
+        // 👀 is this correct (default toolchain)
+        let toolchain = try toolchain ?? UserToolchain(destination: .hostDestination())
+        let manifestLoader = ManifestLoader(toolchain: toolchain.configuration)
+
+        try self.init(
+            forRootPackage: packagePath,
+            manifestLoader: manifestLoader,
+            repositoryManager: repositoryManager,
+            delegate: delegate
+        )
+    }
+
+    /// A convenience method for creating a workspace for the given root
+    /// package path.
+    ///
+    /// The root package path is used to compute the build directory and other
+    /// default paths.
+    public convenience init(
         forRootPackage packagePath: AbsolutePath,
         manifestLoader: ManifestLoaderProtocol,
         repositoryManager: RepositoryManager? = nil,
-        delegate: WorkspaceDelegate? = nil,
-        identityResolver: IdentityResolver? = nil
-    ) -> Workspace {
-        return Workspace(
+        delegate: WorkspaceDelegate? = nil
+    ) throws {
+
+        self .init(
             dataPath: packagePath.appending(component: ".build"),
             editablesPath: packagePath.appending(component: "Packages"),
             pinsFile: packagePath.appending(component: "Package.resolved"),
             manifestLoader: manifestLoader,
             repositoryManager: repositoryManager,
-            delegate: delegate,
-            identityResolver: identityResolver
+            delegate: delegate
+        )
+    }
+
+    /// A convenience method for creating a workspace for the given root
+    /// package path.
+    ///
+    /// The root package path is used to compute the build directory and other
+    /// default paths.
+    // FIXME: this one is kind of messy to backwards support, hopefully we can remove quickly
+    // deprecated 8/2021
+    @available(*, deprecated, message: "use constructor instead")
+    public static func create(
+        forRootPackage packagePath: AbsolutePath,
+        manifestLoader: ManifestLoaderProtocol,
+        repositoryManager: RepositoryManager? = nil,
+        delegate: WorkspaceDelegate? = nil,
+        identityResolver: IdentityResolver? = nil
+    ) -> Workspace {
+        return try! .init(forRootPackage: packagePath,
+                     manifestLoader: manifestLoader,
+                     repositoryManager: repositoryManager,
+                     delegate: delegate//,
+                     //identityResolver: identityResolver
         )
     }
 }
@@ -628,14 +687,16 @@ extension Workspace {
     ///   - diagnostics: Optional.  The diagnostics engine.
     ///   - on: The dispatch queue to perform asynchronous operations on.
     ///   - completion: The completion handler .
+    // deprecated 8/2021
+    @available(*, deprecated, message: "use workspace instance API instead")
     public static func loadRootGraph(
         at packagePath: AbsolutePath,
         swiftCompiler: AbsolutePath,
         swiftCompilerFlags: [String],
         identityResolver: IdentityResolver? = nil,
         diagnostics: DiagnosticsEngine
     ) throws -> PackageGraph {
-        let toolchain = try ToolchainConfiguration(swiftCompiler: swiftCompiler, swiftCompilerFlags: swiftCompilerFlags)
+        let toolchain = ToolchainConfiguration(swiftCompiler: swiftCompiler, swiftCompilerFlags: swiftCompilerFlags)
         let loader = ManifestLoader(toolchain: toolchain)
         let workspace = Workspace.create(forRootPackage: packagePath, manifestLoader: loader, identityResolver: identityResolver)
         return try workspace.loadPackageGraph(rootPath: packagePath, diagnostics: diagnostics)
@@ -752,6 +813,49 @@ extension Workspace {
         }
     }
 
+    /// Loads and returns manifest at the given path.
+    public func loadRootManifest(
+        at path: AbsolutePath,
+        diagnostics: DiagnosticsEngine,
+        completion: @escaping(Result<Manifest, Error>) -> Void
+    ) {
+        self.loadRootManifests(packages: [path], diagnostics: diagnostics) { result in
+            completion(result.tryMap{
+                // normally, we call loadRootManifests which attempts to load any manifest it can and report errors via diagnostics
+                // in this case, we want to load a specific manifest, so if the diagnostics contains an error we want to throw
+                guard !diagnostics.hasErrors else {
+                    // not sure about this one
+                    throw StringError("\(diagnostics)")
+                }
+                guard let manifest = $0[path] else {
+                    throw InternalError("Unknown manifest for '\(path)'")
+                }
+                return manifest
+            })
+        }
+    }
+    
+    public func loadRootPackage(
+        at path: AbsolutePath,
+        diagnostics: DiagnosticsEngine,
+        completion: @escaping(Result<Package, Error>) -> Void
+    ) {
+        self.loadRootManifest(at: path, diagnostics: diagnostics) { result in
+            let result = result.tryMap { manifest -> Package in
+                let identity = self.identityResolver.resolveIdentity(for: manifest.packageLocation)
+                let builder = PackageBuilder(
+                    identity: identity,
+                    manifest: manifest,
+                    productFilter: .everything,
+                    path: path,
+                    xcTestMinimumDeploymentTargets: MinimumDeploymentTarget.default.xcTestMinimumDeploymentTargets,
+                    diagnostics: diagnostics)
+                return try builder.construct()
+            }
+            completion(result)
+        }
+    }
+
     /// Generates the checksum
     public func checksum(
         forBinaryArtifactAt path: AbsolutePath,
diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift
