Expose a SwiftPM BSP interface based on Swift Build #9129
Conversation
owenv
requested review from
bkhouri,
bripeticca,
cmcgee1024,
daveyc123,
dschaefer2,
jakepetroules,
plemarquand and
shawnhyam
as code owners
|
This is very much a WIP right now, it just exposes the bare minimum functionality so we can discuss API design |
owenv
force-pushed
the
owenv/bsp
branch
3 times, most recently
from
fda8c7b to
7f310d2
Compare
owenv
changed the title
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test Windows |
|
@swift-ci test |
|
@swift-ci test Windows |
| _ = try await connection.send(BuildShutdownRequest()) | ||
| connection.send(OnBuildExitNotification()) | ||
| connection.close() | ||
| bspProcess.waitUntilExit() |
There was a problem hiding this comment.
Please don't use waitUntilExit as it spins the run loop and is dangerous in Swift Concurrency. If you need to do work in between launch() and waitUntilExit() (which is why I assume you're not using AsyncProcess), then you can borrow this technique from Swift Build:
extension Process {
/// Runs the process and returns a promise which is fulfilled when the process exits.
///
/// - note: This method sets the process's termination handler, if one is set.
/// - throws: Rethrows the error from ``Process/run`` if the task could not be launched.
public func launch() throws -> Promise<Processes.ExitStatus, any Error> {
let promise = Promise<Processes.ExitStatus, any Error>()
terminationHandler = { process in
do {
promise.fulfill(with: try .init(process))
} catch {
promise.fail(throwing: error)
}
}
do {
try run()
} catch {
terminationHandler = nil
// If `run` throws, the terminationHandler won't be called.
throw error
}
return promise
}
}There was a problem hiding this comment.
I updated this to use a regular continuation which I think should also do the job here
There was a problem hiding this comment.
There are a few edge cases with that, commented inline.
|
@swift-ci test |
| import ToolsProtocolsSwiftExtensions | ||
|
|
||
| extension Connection { | ||
| public func send<Request: RequestType>(_ request: Request) async throws -> Request.Response { |
There was a problem hiding this comment.
This won’t cancel the request if the task is cancelled. I think we should just move https://github.com/swiftlang/sourcekit-lsp/blob/main/Sources/LanguageServerProtocolExtensions/Connection%2BSend.swift to swift-tools-protocols and use that.
There was a problem hiding this comment.
I copied the code for now, I'll open a PR to add it to tools-protocols later today and begin adopting that in the toolchain
There was a problem hiding this comment.
Great. Thank you. If you get a chance to also adopt it in SourceKit-LSP, that would be greatly appreciated.
| } | ||
| } | ||
| case is OnBuildLogMessageNotification: | ||
| // If we receive a build log message notification, forward it on to the client |
There was a problem hiding this comment.
OnBuildLogMessageNotification is only sent from the server to the client according the the LSP spec, just bouncing it back to the client doesn’t really make sense. I’d just ignore it.
There was a problem hiding this comment.
This is here so that if we receive this notification from the Swift Build build server, we forward it to the SwiftPM build server client (LSP), so I think we still want it
There was a problem hiding this comment.
OK, I’m confused now. Isn’t this method handling notifications sent from the client (ie. SourceKit-LSP) to the server (ie. the SwiftPM BSP server)?
There was a problem hiding this comment.
It's handling both notifications from the underlying server to itself over one connection, and notifications from the client to itself on another, and they're both funneled to the same protocol requirement
There was a problem hiding this comment.
Oh, interesting. I would encourage you to split these up (not in this PR, just in general). In SourceKit-LSP I invested a bit of time clearly documenting which methods receive data from the client and which ones send data to the client and it helped readability a lot IMO.
|
|
||
| private func initialize(request: InitializeBuildRequest) async throws -> InitializeBuildResponse { | ||
| if state != .waitingForInitializeRequest { | ||
| logToClient(.warning, "Received initialization request while the build server is \(state)") |
There was a problem hiding this comment.
Should these be log messages using logger so that they end up in the build server logs, eg. inside a sysdiagnose?
There was a problem hiding this comment.
For now I'm consistently sending everything to the client, but I'll probably do a pass over all this as a follow up and decide what should also/instead go to the system log
|
@swift-ci test |
| bspProcess.executableURL = URL(filePath: execPath) | ||
| bspProcess.arguments = ["experimental-build-server", "--build-system", "swiftbuild"] | ||
| bspProcess.currentDirectoryURL = URL(filePath: fixture.pathString) | ||
| bspProcess.launch() as Void |
There was a problem hiding this comment.
launch() is effectively deprecated as it throws Objective-C exceptions instead of Swift errors; it's replaced by run.
| bspProcess.launch() as Void | |
| try bspProcess.run() as Void |
| try await body(connection, notificationCollector, fixture) | ||
| _ = try await connection.send(BuildShutdownRequest()) | ||
| await withCheckedContinuation { continuation in | ||
| bspProcess.terminationHandler = { _ in |
There was a problem hiding this comment.
This is too late, the terminationHandler needs to be installed before run/launch is called or you may miss the notification, leading to a hang.
You also need to make sure you uninstall the terminationHandler if run() throws.
I suggested my launch promise because it handles all of this for you and I expect we're going to want those abstractions in SwiftPM at some point or another anyways...
There was a problem hiding this comment.
Or, why not just use AsyncProcess? It looks like it has a launch() method and an async-compatible waitUntilExit() overload.
There was a problem hiding this comment.
did this with run + async let instead. AsyncProcess makes the I/O kinda annoying
There was a problem hiding this comment.
Oh yeah, async let. That works, thanks!
| _ = try await connection.send(BuildShutdownRequest()) | ||
| connection.send(OnBuildExitNotification()) | ||
| connection.close() | ||
| bspProcess.waitUntilExit() |
There was a problem hiding this comment.
There are a few edge cases with that, commented inline.
|
@swift-ci test |
|
@swift-ci test Windows |
|
@swift-ci test windows |
4 participants
Add a
swift package bspcommand which builds on the Swift Build BSP interface to provide preparation + compiler args for packages as an out of process build server.