Allow opting in to using the new SwiftPM BSP server when opening a package #2378
Conversation
owenv
requested review from
ahoppen,
bnbarham,
hamishknight and
rintaro
as code owners
|
I haven't done much porting of tests yet, because I want to take a look at which ones should stay in sourcekit-lsp vs sink into SwiftPM. |
| } | ||
| return BuildServerConfig( | ||
| name: "SwiftPM Build Server", | ||
| version: "", |
There was a problem hiding this comment.
use Swift version tag?
There was a problem hiding this comment.
We don't have it in sourcekit-lsp right now (we have no --version) and until we have a common version that all the projects can use, I'd prefer not to add it.
| if options.swiftPMOrDefault.useExperimentalSwiftPMBuildServer == true { | ||
| expectArgumentsContain( | ||
| "-target", | ||
| // Account for differences in macOS naming canonicalization |
There was a problem hiding this comment.
I assume this is a built-in vs swift-build difference rather than a built-in SwiftPM support vs build server? Given this depends on PackageModel anyway, this will probably be one of the tests that will end up moving. For in sourcekit-lsp, checking we have -target/-sdk/-I/-F (and not their values) is probably enough. And to be clear, no need to block the PR on this/moving tests in general.
There was a problem hiding this comment.
yep, one uses macosx, the other macos. It's pretty annoying but hard to easily align them.
| connectionToSourceKitLSP: connectionToSourceKitLSP, | ||
| testHooks: buildServerHooks.swiftPMTestHooks | ||
| ) | ||
| if options.swiftPMOrDefault.useExperimentalSwiftPMBuildServer == true { |
There was a problem hiding this comment.
| if options.swiftPMOrDefault.useExperimentalSwiftPMBuildServer == true { | |
| if options.swiftPMOrDefault.useExperimentalSwiftPMBuildServer ?? false { |
I just find this more readable because it specifies which value nil should default to and then you’re back to a normal boolean check.
Tests/BuildServerIntegrationTests/SwiftPMBuildServerTests.swift
Outdated
Show resolved
Hide resolved
Tests/BuildServerIntegrationTests/SwiftPMBuildServerTests.swift
Outdated
Show resolved
Hide resolved
Tests/BuildServerIntegrationTests/SwiftPMBuildServerTests.swift
Outdated
Show resolved
Hide resolved
|
|
||
| @Test | ||
| func testBasicSwiftArgs() async throws { | ||
| @Test(arguments: [SourceKitLSPOptions(), .forTestingExperimentalSwiftPMBuildServer]) |
There was a problem hiding this comment.
Did you try running all tests that use SwiftPMTestProject with the new build server?
There was a problem hiding this comment.
I haven't audited everything yet but I've gone through a decent number. Some failures due to tests making assumptions that don't hold under the new build system, and a few which look like bugs or bits of missing functionality that'll need to be fixed up
There was a problem hiding this comment.
Nice. I’d like to find some way to run tests using both configurations at some point (maybe an environment variable and just run swift test twice 🤔) but that’s something for the future.
owenv
force-pushed
the
owenv/bsp
branch
2 times, most recently
from
322248a to
bc4e4c5
Compare
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Just #2378 (comment) and two nits, otherwise LGTM.
4 participants
Depends on swiftlang/swift-package-manager#9129
Add a SourceKit-LSP option to allow opting in to use of the new SwiftPM build server, using the existing ExternalBuildServerAdapter with a pre-generated configuration. This allows us to integrate The Swift Build build system backend without having SourceKitLSP need to continue linking parts of SwiftPM directly