Skip to content

fix package describe to handle binary targets #3810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Sources/Build/BuildPlan.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
76 changes: 30 additions & 46 deletions Sources/Commands/SwiftPackageTool.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Standardizing all the various tool actions on loading at least the root package first seems like a good thing, so this seems like great cleanup regardless.

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
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion Sources/PackageLoading/ManifestLoader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion Sources/PackageLoading/PackageBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions Sources/PackageModel/Target.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}

Expand Down
19 changes: 18 additions & 1 deletion Sources/Workspace/Workspace.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down