Expose a low-level BSP interface wrapping SWBBuildServiceSession #782
Conversation
owenv
requested review from
aciidgh,
jakepetroules,
mirza-garibovic,
mhrawdon and
neonichu
as code owners
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
| } | ||
| state = .running | ||
| case is OnWatchedFilesDidChangeNotification: | ||
| guard state == .running else { |
There was a problem hiding this comment.
I don’t think there’s any harm in handling file change notifications while the server is initializing, right? I would log an error if we are in the wrong state but still continue handling it, just makes the BSP server more resilient for badly behaving clients.
| if !(request.params is InitializeBuildRequest) { | ||
| let state = self.state | ||
| guard state == .running else { | ||
| await request.reply { throw ResponseError.unknown("Request received while the build server is \(state.description)") } |
There was a problem hiding this comment.
Similarly here, if we are in the waitingForInitializationNotification state, we should be able to handle those requests, I would personally try and make the BSP server handle requests even if they don’t quite come in the right order.
| baseDirectory: nil, | ||
| tags: tags, | ||
| capabilities: BuildTargetCapabilities(), | ||
| languageIds: [.c, .cpp, .objective_c, .objective_cpp, .swift], |
There was a problem hiding this comment.
If there was an easy way to tell which languages are present in the target, it would be nice to only report those but if not, it’s fine. SourceKit-LSP doesn’t end up caring about this. I’d just add a comment explaining why we return all languages here.
Related, worth reporting Metal as well?
| import Foundation | ||
|
|
||
| /// Wraps a `SWBBuildServiceSession` to expose Build Server Protocol functionality. | ||
| public actor SWBBuildServer: QueueBasedMessageHandler { |
There was a problem hiding this comment.
Can we have a nested type that conforms to QueueBasedMessageHandler instead? I don't think clients should be calling didReceive, handle, etc., on this.
Probably also allows SWBBuildServerProtocol to become an internal import?
There was a problem hiding this comment.
At a minimum we need to expose a type with a MessageHandler conformance so we can hand it off to a connection object like we do in the tests
|
@ahoppen did a first pass through your feedback, thanks. I'm waiting to address the rest until I've had more time to think about the logging strategy |
Thanks. There was nothing blocking in my comments. Mostly just thoughts on how things could be improved. |
SWBBuildServer implements BSP message handling and is intended to back a higher level BSP which manages the project model and generates PIF and a build request (SwiftPM). As part of this change, many of the core BSP types are vendored into the new SWBBuildServerProtocol target from SourceKitLSP with minor modifications.
SWBBuildServer implements BSP message handling and is intended to back a higher level BSP which manages the project model and generates PIF and a build request (SwiftPM).
As part of this change, many of the core BSP types are vendored into the new SWBBuildServerProtocol target from SourceKitLSP with minor modifications. The only significant changes to that code are:
The new code in this PR is largely in SWBBuildServer and SWBBuildServerTests