motivation: API cleanup towards identity changes (#3234)

chamges:
* reorder and rename the parameters of ManifestLoader and make them explicit
* reorder and rename the parameters of test utilities related to ManifestLoader
* deprecate abstract ManifestBuild and refactor it as ManifestJSONParser which is more true to what it actually does
* rename manifest::url to manifest::pacakgeLocation which is more true to its real meaning
* adjust call sites

note: no funcitonal changes here, API refactoring to make the next step easaier and follow on PRs more focused

Author: tomerd

Examples/package-info/Sources/package-info/main.swift

@@ -19,7 +19,7 @@ let swiftCompiler: AbsolutePath = {
 
 // We need a package to work with.
 // This assumes there is one in the current working directory:
-let package = localFileSystem.currentWorkingDirectory!
+let packagePath = localFileSystem.currentWorkingDirectory!
 
 // LOADING
 // =======
@@ -31,9 +31,9 @@ let package = localFileSystem.currentWorkingDirectory!
 // 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 manifest = try ManifestLoader.loadManifest(packagePath: package, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], packageKind: .local)
-let loadedPackage = try PackageBuilder.loadPackage(packagePath: package, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], diagnostics: diagnostics)
-let graph = try Workspace.loadGraph(packagePath: package, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], diagnostics: diagnostics)
+let manifest = try tsc_await { ManifestLoader.loadManifest(at: packagePath, kind: .local, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], on: .global(), completion: $0) }
+let loadedPackage = try tsc_await { PackageBuilder.loadPackage(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], diagnostics: diagnostics, on: .global(), completion: $0) }
+let graph = try Workspace.loadGraph(packagePath: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], diagnostics: diagnostics)
 
 // EXAMPLES
 // ========

Sources/Commands/show-dependencies.swift

@@ -44,7 +44,7 @@ private final class PlainTextDumper: DependenciesDumper {
 
                 let pkgVersion = package.manifest.version?.description ?? "unspecified"
 
-                stream <<< "\(hanger)\(package.name)<\(package.manifest.url)@\(pkgVersion)>\n"
+                stream <<< "\(hanger)\(package.name)<\(package.manifest.packageLocation)@\(pkgVersion)>\n"
 
                 if !package.dependencies.isEmpty {
                     let replacement = (index == packages.count - 1) ?  "    " : "│   "
@@ -85,7 +85,7 @@ private final class DotDumper: DependenciesDumper {
     func dump(dependenciesOf rootpkg: ResolvedPackage, on stream: OutputByteStream) {
         var nodesAlreadyPrinted: Set<String> = []
         func printNode(_ package: ResolvedPackage) {
-            let url = package.manifest.url
+            let url = package.manifest.packageLocation
             if nodesAlreadyPrinted.contains(url) { return }
             let pkgVersion = package.manifest.version?.description ?? "unspecified"
             stream <<< #""\#(url)" [label="\#(package.name)\n\#(url)\n\#(pkgVersion)"]"# <<< "\n"
@@ -100,8 +100,8 @@ private final class DotDumper: DependenciesDumper {
         func recursiveWalk(rootpkg: ResolvedPackage) {
             printNode(rootpkg)
             for dependency in rootpkg.dependencies {
-                let rootURL = rootpkg.manifest.url
-                let dependencyURL = dependency.manifest.url
+                let rootURL = rootpkg.manifest.packageLocation
+                let dependencyURL = dependency.manifest.packageLocation
                 let urlPair = DependencyURLs(root: rootURL, dependency: dependencyURL)
                 if dependenciesAlreadyPrinted.contains(urlPair) { continue }
 
@@ -131,7 +131,7 @@ private final class JSONDumper: DependenciesDumper {
         func convert(_ package: ResolvedPackage) -> JSON {
             return .orderedDictionary([
                 "name": .string(package.name),
-                "url": .string(package.manifest.url),
+                "url": .string(package.manifest.packageLocation),
                 "version": .string(package.manifest.version?.description ?? "unspecified"),
                 "path": .string(package.path.pathString),
                 "dependencies": .array(package.dependencies.map(convert)),

Sources/PackageGraph/LocalPackageContainer.swift

@@ -47,12 +47,14 @@ public final class LocalPackageContainer: PackageContainer {
             // Load the manifest.
             // FIXME: this should not block
             return try temp_await {
-                manifestLoader.load(package: AbsolutePath(package.location),
-                                    baseURL: package.location,
+                manifestLoader.load(at: AbsolutePath(package.location),
+                                    packageKind: package.kind,
+                                    packageLocation: package.location,
                                     version: nil,
+                                    revision: nil,
                                     toolsVersion: toolsVersion,
-                                    packageKind: package.kind,
                                     fileSystem: fileSystem,
+                                    diagnostics: nil,
                                     on: .global(),
                                     completion: $0)
             }

Sources/PackageGraph/PackageGraph+Loading.swift

@@ -39,7 +39,7 @@ extension PackageGraph {
         // the URL but that shouldn't be needed after <rdar://problem/33693433>
         // Ensure that identity and package name are the same once we have an
         // API to specify identity in the manifest file
-        let manifestMapSequence = (root.manifests + externalManifests).map({ (PackageIdentity(url: $0.url), $0) })
+        let manifestMapSequence = (root.manifests + externalManifests).map({ (PackageIdentity(url: $0.packageLocation), $0) })
         let manifestMap = Dictionary(uniqueKeysWithValues: manifestMapSequence)
         let successors: (GraphLoadingNode) -> [GraphLoadingNode] = { node in
             node.requiredDependencies().compactMap({ dependency in
@@ -201,7 +201,7 @@ private func createResolvedPackages(
         guard let package = manifestToPackage[node.manifest] else {
             return nil
         }
-        let isAllowedToVendUnsafeProducts = unsafeAllowedPackages.contains{ $0.location == package.manifest.url }
+        let isAllowedToVendUnsafeProducts = unsafeAllowedPackages.contains{ $0.location == package.manifest.packageLocation }
         return ResolvedPackageBuilder(
             package,
             productFilter: node.productFilter,
@@ -211,7 +211,7 @@ private func createResolvedPackages(
 
     // Create a map of package builders keyed by the package identity.
     let packageMapByIdentity: [PackageIdentity: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{
-        let identity = PackageIdentity(url: $0.package.manifest.url)
+        let identity = PackageIdentity(url: $0.package.manifest.packageLocation)
         return (identity, $0)
     }
     let packageMapByName: [String: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{ ($0.package.name, $0) }
@@ -235,7 +235,7 @@ private func createResolvedPackages(
                     let error = PackageGraphError.dependencyAlreadySatisfiedByName(
                         dependencyPackageName: package.name,
                         dependencyURL: dependencyURL,
-                        otherDependencyURL: resolvedPackage.package.manifest.url,
+                        otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
                         name: explicitDependencyName)
                     let diagnosticLocation = PackageLocation.Local(name: package.name, packagePath: package.path)
                     return diagnostics.emit(error, location: diagnosticLocation)
@@ -252,7 +252,7 @@ private func createResolvedPackages(
                     let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
                         dependencyPackageName: package.name,
                         dependencyURL: dependencyURL,
-                        otherDependencyURL: resolvedPackage.package.manifest.url,
+                        otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
                         identity: dependencyIdentity)
                     let diagnosticLocation = PackageLocation.Local(name: package.name, packagePath: package.path)
                     return diagnostics.emit(error, location: diagnosticLocation)
@@ -262,11 +262,11 @@ private func createResolvedPackages(
                     // check if this resolvedPackage url is the same as the dependency one
                     // if not, this means that the dependencies share the same identity
                     // FIXME: this works but the way we find out about this is based on a side effect, need to improve it when working on identity
-                    if resolvedPackage.package.manifest.url != dependencyURL {
+                    if resolvedPackage.package.manifest.packageLocation != dependencyURL {
                         let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
                             dependencyPackageName: package.name,
                             dependencyURL: dependencyURL,
-                            otherDependencyURL: resolvedPackage.package.manifest.url,
+                            otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
                             identity: dependencyIdentity)
                         let diagnosticLocation = PackageLocation.Local(name: package.name, packagePath: package.path)
                         return diagnostics.emit(error, location: diagnosticLocation)
@@ -276,7 +276,7 @@ private func createResolvedPackages(
                             dependencyName: explicitDependencyName,
                             dependencyURL: dependencyURL,
                             resolvedPackageName: resolvedPackage.package.name,
-                            resolvedPackageURL: resolvedPackage.package.manifest.url)
+                            resolvedPackageURL: resolvedPackage.package.manifest.packageLocation)
                         let diagnosticLocation = PackageLocation.Local(name: package.name, packagePath: package.path)
                         return diagnostics.emit(error, location: diagnosticLocation)
                     }
@@ -414,7 +414,7 @@ private func createResolvedPackages(
                     // explicitly reference the package containing the product, or for the product, package and
                     // dependency to share the same name. We don't check this in manifest loading for root-packages so
                     // we can provide a more detailed diagnostic here.
-                    let referencedPackageURL = mirrors.effectiveURL(forURL: product.packageBuilder.package.manifest.url)
+                    let referencedPackageURL = mirrors.effectiveURL(forURL: product.packageBuilder.package.manifest.packageLocation)
                     let referencedPackageIdentity = PackageIdentity(url: referencedPackageURL)
                     guard let packageDependency = (packageBuilder.package.manifest.dependencies.first { package in
                         let packageURL = mirrors.effectiveURL(forURL: package.url)

Sources/PackageGraph/PackageGraphRoot.swift

@@ -48,7 +48,7 @@ public struct PackageGraphRoot {
     /// Create a package graph root.
     public init(input: PackageGraphRootInput, manifests: [Manifest], explicitProduct: String? = nil) {
         self.packageRefs = zip(input.packages, manifests).map { (path, manifest) in
-            let identity = PackageIdentity(url: manifest.url)
+            let identity = PackageIdentity(url: manifest.packageLocation)
             return .root(identity: identity, path: path)
         }
         self.manifests = manifests

Sources/PackageGraph/RepositoryPackageContainer.swift

@@ -269,24 +269,26 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
     private func loadManifest(at revision: Revision, version: Version?) throws -> Manifest {
         try self.manifestsCache.memoize(revision) {
             let fs = try repository.openFileView(revision: revision)
-            let packageURL = package.repository.url
+            let packageLocation = package.repository.url
 
             // Load the tools version.
             let toolsVersion = try toolsVersionLoader.load(at: .root, fileSystem: fs)
 
             // Validate the tools version.
             try toolsVersion.validateToolsVersion(
-                self.currentToolsVersion, version: revision.identifier, packagePath: packageURL)
+                self.currentToolsVersion, version: revision.identifier, packagePath: packageLocation)
 
             // Load the manifest.
             // FIXME: this should not block
             return try temp_await {
-                manifestLoader.load(package: AbsolutePath.root,
-                                    baseURL: packageURL,
+                manifestLoader.load(at: AbsolutePath.root,
+                                    packageKind: package.kind,
+                                    packageLocation: packageLocation,
                                     version: version,
+                                    revision: nil,
                                     toolsVersion: toolsVersion,
-                                    packageKind: package.kind,
                                     fileSystem: fs,
+                                    diagnostics: nil,
                                     on: .global(),
                                     completion: $0)
             }

Sources/PackageLoading/CMakeLists.txt

@@ -8,7 +8,6 @@
 
 add_library(PackageLoading
   Diagnostics.swift
-  ManifestBuilder.swift
   ManifestLoader.swift
   MinimumDeploymentTarget.swift
   ModuleMapGenerator.swift