Skip to content

Fix create symbol graph plugin entrypoint #7629

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix create symbol graph plugin entrypoint #7629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 31 additions & 6 deletions Sources/Commands/Utilities/PluginDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import Basics
import CoreCommands
import Foundation
import PackageGraph
import PackageModel
import SPMBuildCore

Expand Down Expand Up @@ -383,14 +384,38 @@ final class PluginDelegate: PluginInvocationDelegate {
// Create a build system for building the target., skipping the the cache because we need the build plan.
let buildSystem = try swiftCommandState.createBuildSystem(explicitBuildSystem: .native, cacheBuildManifest: false)

// Find the target in the build operation's package graph; it's an error if we don't find it.
// Find the target in the build operation's package graph
// 1. First look for a matching destination module
// 2. If not found, search for a matching tools module, only allowing
// modules that should be built for the host (macro, plugin, test).
// 3. Error if no matching targets are found.
//
// FIXME: dynamic graph
// This should work by requesting a build for a target w/ destination:
// .destination and generating the graph needed on demand instead of
// looking for a preexisting matching target.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add a func buildsetFor(target: String) throws -> Buildset to BuildSystem protocol which would encapsulate most of the logic here and allow you to select appropriate build parameters based on resulting buildset.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the client should be able to request any module built for the target/destination platform and we should create the graph that does that. Is that what you're suggesting here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, client would pass a name and we'd start with .destination and if that doesn't exist produce a .host if it's appropriate.

let target: ResolvedModule
let destination: BuildParameters.Destination
let packageGraph = try buildSystem.getPackageGraph()
guard let target = packageGraph.target(for: targetName, destination: .destination) else {
throw StringError("could not find a target named “\(targetName)”")
if let _target = packageGraph.target(for: targetName, destination: .destination) {
target = _target
destination = .target
} else if let _target = packageGraph.target(for: targetName, destination: .tools),
(_target.type == .macro || _target.type == .plugin || _target.type == .test) {
target = _target
destination = .host
} else {
throw StringError("Could not find a target named “\(targetName)”")
}

// Build the target, if needed.
try buildSystem.build(subset: .target(target.name))
try buildSystem.build(subset: .target(target.name, for: destination))

let buildParameters: BuildParameters = try if destination == .target {
buildSystem.buildPlan.destinationBuildParameters
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation looks off here

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is correct for if expressions. swift-format depending on options can reformat this to:

let buildParameters: BuildParameters =
  try if destination == .target {
    buildSystem.buildPlan.destinationBuildParameters
  } else {
    buildSystem.buildPlan.toolsBuildParameters
  }

buildSystem.buildPlan.toolsBuildParameters
}

// Configure the symbol graph extractor.
var symbolGraphExtractor = try SymbolGraphExtract(
Expand Down Expand Up @@ -419,7 +444,7 @@ final class PluginDelegate: PluginInvocationDelegate {
guard let package = packageGraph.package(for: target) else {
throw StringError("could not determine the package for target “\(target.name)”")
}
let outputDir = try buildSystem.buildPlan.toolsBuildParameters.dataPath.appending(
let outputDir = buildParameters.dataPath.appending(
components: "extracted-symbols",
package.identity.description,
target.name
Expand All @@ -430,7 +455,7 @@ final class PluginDelegate: PluginInvocationDelegate {
let result = try symbolGraphExtractor.extractSymbolGraph(
module: target,
buildPlan: try buildSystem.buildPlan,
buildParameters: buildSystem.buildPlan.destinationBuildParameters,
buildParameters: buildParameters,
outputRedirection: .collect,
outputDirectory: outputDir,
verboseOutput: self.swiftCommandState.logLevel <= .info
Expand Down
11 changes: 11 additions & 0 deletions Sources/PackagePlugin/PackageManagerProxy.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,17 @@ public enum PackageManagerProxyError: Error {
case unspecified(_ message: String)
}

extension PackageManagerProxyError: CustomStringConvertible {
public var description: String {
switch self {
case .unimplemented(let message):
"Unimplemented: \(message)"
case .unspecified(let message):
"Unspecified: \(message)"
}
}
}

fileprivate extension PluginToHostMessage.BuildSubset {
init(_ subset: PackageManager.BuildSubset) {
switch subset {
Expand Down