fix package describe to handle binary targets (#3810)

* fix pacakge describe to handle binary targets

motivation: package describe command fails for packages with binary targets

changes:
* PackageBuilder requires remote artifacts to be set to compute the model, as such make it a required argument
* do not call PackageBuilder directly from describe and format commands, instead use  Workspace::loadRootPackage API for loading a root package
* update Workspace::loadRootPackage to compute artifacts

Author: tomerd

Sources/Build/BuildPlan.swift

@@ -1832,6 +1832,8 @@ public class BuildPlan {
                     case .artifactsArchive:
                         let tools = try self.parseArtifactsArchive(for: binaryTarget)
                         tools.forEach { availableTools[$0.name] = $0.executablePath  }
+                    case.unknown:
+                        throw InternalError("unknown binary target '\(target.name)' type")
                     }
                 case .plugin:
                     continue

Sources/Commands/SwiftPackageTool.swift

@@ -159,55 +159,49 @@ extension SwiftPackageTool {
     struct Describe: SwiftCommand {
         static let configuration = CommandConfiguration(
             abstract: "Describe the current package")
-
+        
         @OptionGroup(_hiddenFromHelp: true)
         var swiftOptions: SwiftToolOptions
-
+        
         @Option(help: "json | text")
         var type: DescribeMode = .text
-
+        
         func run(_ swiftTool: SwiftTool) throws {
             let workspace = try swiftTool.getActiveWorkspace()
-            let root = try swiftTool.getWorkspaceRoot()
-
-            let rootManifests = try temp_await {
-                workspace.loadRootManifests(packages: root.packages, diagnostics: swiftTool.observabilityScope.makeDiagnosticsEngine(), completion: $0)
+            
+            guard let packagePath = try swiftTool.getWorkspaceRoot().packages.first else {
+                throw StringError("unknown package")
             }
-            guard let rootManifest = rootManifests.values.first else {
-                throw StringError("invalid manifests at \(root.packages)")
+            
+            let package = try tsc_await {
+                workspace.loadRootPackage(
+                    at: packagePath,
+                    observabilityScope: swiftTool.observabilityScope,
+                    completion: $0
+                )
             }
-
-            let builder = PackageBuilder(
-                identity: .plain(rootManifest.name),
-                manifest: rootManifest,
-                productFilter: .everything,
-                path: try swiftTool.getPackageRoot(),
-                xcTestMinimumDeploymentTargets: MinimumDeploymentTarget.default.xcTestMinimumDeploymentTargets,
-                fileSystem: localFileSystem,
-                observabilityScope: swiftTool.observabilityScope
-            )
-            let package = try builder.construct()
-            self.describe(package, in: type, on: swiftTool.outputStream)
+            
+            try self.describe(package, in: type, on: swiftTool.outputStream)
         }
-
+        
         /// Emits a textual description of `package` to `stream`, in the format indicated by `mode`.
-        func describe(_ package: Package, in mode: DescribeMode, on stream: OutputByteStream) {
+        func describe(_ package: Package, in mode: DescribeMode, on stream: OutputByteStream) throws {
             let desc = DescribedPackage(from: package)
             let data: Data
             switch mode {
             case .json:
                 let encoder = JSONEncoder.makeWithDefaults()
                 encoder.keyEncodingStrategy = .convertToSnakeCase
-                data = try! encoder.encode(desc)
+                data = try encoder.encode(desc)
             case .text:
                 var encoder = PlainTextEncoder()
                 encoder.formattingOptions = [.prettyPrinted]
-                data = try! encoder.encode(desc)
+                data = try encoder.encode(desc)
             }
             stream <<< String(decoding: data, as: UTF8.self) <<< "\n"
             stream.flush()
         }
-
+        
         enum DescribeMode: String, ExpressibleByArgument {
             /// JSON format (guaranteed to be parsable and stable across time).
             case json
@@ -269,28 +263,18 @@ extension SwiftPackageTool {
 
             // Get the root package.
             let workspace = try swiftTool.getActiveWorkspace()
-            let root = try swiftTool.getWorkspaceRoot()
-            let rootManifests = try temp_await {
-                workspace.loadRootManifests(
-                    packages: root.packages,
-                    diagnostics: swiftTool.observabilityScope.makeDiagnosticsEngine(),
+
+            guard let packagePath = try swiftTool.getWorkspaceRoot().packages.first else {
+                throw StringError("unknown package")
+            }
+
+            let package = try tsc_await {
+                workspace.loadRootPackage(
+                    at: packagePath,
+                    observabilityScope: swiftTool.observabilityScope,
                     completion: $0
                 )
             }
-            guard let rootManifest = rootManifests.values.first else {
-                throw StringError("invalid manifests at \(root.packages)")
-            }
-
-            let builder = PackageBuilder(
-                identity: .plain(rootManifest.name),
-                manifest: rootManifest,
-                productFilter: .everything,
-                path: try swiftTool.getPackageRoot(),
-                xcTestMinimumDeploymentTargets: [:], // Minimum deployment target does not matter for this operation.
-                fileSystem: localFileSystem,
-                observabilityScope: swiftTool.observabilityScope
-            )
-            let package = try builder.construct()
 
             // Use the user provided flags or default to formatting mode.
             let formatOptions = swiftFormatFlags.isEmpty
@@ -304,7 +288,7 @@ extension SwiftPackageTool {
                 }
             }.map { $0.pathString }
 
-            let args = [swiftFormat.pathString] + formatOptions + [rootManifest.path.pathString] + paths
+            let args = [swiftFormat.pathString] + formatOptions + [packagePath.pathString] + paths
             print("Running:", args.map{ $0.spm_shellEscaped() }.joined(separator: " "))
 
             let result = try Process.popen(arguments: args)

Sources/PackageLoading/ManifestLoader.swift

@@ -405,7 +405,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
 
             var validExtensions = [self.supportedArchiveExtension]
             if target.isLocal {
-                validExtensions += BinaryTarget.Kind.allCases.map { $0.fileExtension }
+                validExtensions += BinaryTarget.Kind.allCases.filter{ $0 != .unknown }.map { $0.fileExtension }
             }
 
             if !validExtensions.contains(location.pathExtension) {

Sources/PackageLoading/PackageBuilder.swift

@@ -263,7 +263,7 @@ public final class PackageBuilder {
         productFilter: ProductFilter,
         path: AbsolutePath,
         additionalFileRules: [FileRuleDescription] = [],
-        binaryArtifacts: [BinaryArtifact] = [],
+        binaryArtifacts: [BinaryArtifact],
         xcTestMinimumDeploymentTargets: [PackageModel.Platform:PlatformVersion],
         shouldCreateMultipleTestProducts: Bool = false,
         warnAboutImplicitExecutableTargets: Bool = true,
@@ -322,6 +322,7 @@ public final class PackageBuilder {
                     manifest: manifest,
                     productFilter: .everything,
                     path: path,
+                    binaryArtifacts: [], // this will fail for packages with binary artifacts, but this API is deprecated and the replacement API was fixed
                     xcTestMinimumDeploymentTargets: xcTestMinimumDeploymentTargets,
                     fileSystem: localFileSystem,
                     observabilityScope: ObservabilitySystem(diagnosticEngine: diagnostics).topScope

Sources/PackageModel/Target.swift

@@ -551,13 +551,16 @@ public final class BinaryTarget: Target {
     public enum Kind: String, RawRepresentable, Codable, CaseIterable {
         case xcframework
         case artifactsArchive
+        case unknown // for non-downloaded artifacts
 
         public var fileExtension: String {
             switch self {
             case .xcframework:
                 return "xcframework"
             case .artifactsArchive:
                 return "artifactbundle"
+            case .unknown:
+                return "unknown"
             }
         }
 

Sources/Workspace/Workspace.swift

@@ -1015,14 +1015,31 @@ extension Workspace {
         observabilityScope: ObservabilityScope,
         completion: @escaping(Result<Package, Error>) -> Void
     ) {
-        self.loadRootManifest(at: path, diagnostics: observabilityScope.makeDiagnosticsEngine()) { result in
+        self.loadRootManifest(at: path, observabilityScope: observabilityScope) { result in
             let result = result.tryMap { manifest -> Package in
                 let identity = try self.identityResolver.resolveIdentity(for: manifest.packageKind)
+
+                // radar/82263304
+                // compute binary artifacts for the sake of constructing a project model
+                // note this does not actually download remote artifacts and as such does not have the artifact's type or path
+                let binaryArtifacts = try manifest.targets.filter{ $0.type == .binary }.map { target -> BinaryArtifact in
+                    if let path = target.path {
+                        let absolutePath = try manifest.path.parentDirectory.appending(RelativePath(validating: path))
+                        return try BinaryArtifact(kind: .forFileExtension(absolutePath.extension ?? "unknown") , originURL: .none, path: absolutePath)
+                    } else if let url = target.url.flatMap(URL.init(string:)) {
+                        let fakePath = try manifest.path.parentDirectory.appending(components: "remote", "archive").appending(RelativePath(validating: url.lastPathComponent))
+                        return BinaryArtifact(kind: .unknown, originURL: url.absoluteString, path: fakePath)
+                    } else {
+                        throw InternalError("a binary target should have either a path or a URL and a checksum")
+                    }
+                }
+
                 let builder = PackageBuilder(
                     identity: identity,
                     manifest: manifest,
                     productFilter: .everything,
                     path: path,
+                    binaryArtifacts: binaryArtifacts,
                     xcTestMinimumDeploymentTargets: MinimumDeploymentTarget.default.xcTestMinimumDeploymentTargets,
                     fileSystem: self.fileSystem,
                     observabilityScope: observabilityScope