Add a new BuildDescription.load API for use in SourceKit-LSP
#8286
Conversation
bnbarham
requested review from
MaxDesiatov,
jakepetroules,
xedin,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand,
owenv and
bkhouri
as code owners
|
Still have to update the tests here, just wanted to kick off the SourceKit LSP tests before the weekend. |
| import PackageLoading | ||
| internal import PackageModel | ||
| import SPMBuildCore | ||
| import Workspace |
There was a problem hiding this comment.
Nit: I think the convention is to only import what is used instead of whole module. Is that right, @MaxDesiatov ?
There was a problem hiding this comment.
I'm open to changing it specifically for TSC (as a way of knowing which TSC APIs are being used), but there's really not point doing it for the SwiftPM APIs.
There was a problem hiding this comment.
ideally, we would enabled "include what you use" (IWYU) against the project, and enforce it in CI.
https://github.com/swiftlang/swift/blob/main/docs/HowToGuides/RunningIncludeWhatYouUse.md
There was a problem hiding this comment.
There is no include what you use for Swift, it's a clang tool
| private import class PackageLoading.ManifestLoader | ||
| internal import struct PackageModel.ToolsVersion | ||
| internal import protocol PackageModel.Toolchain | ||
| import Basics |
bnbarham
force-pushed
the
plan-should-build-tool-plugins
branch
2 times, most recently
from
764b3b0 to
16e7747
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
Build tool plugins need to be built and run as part of the build plan generation. Rather than relying on clients to do so, add a new API that does both. This does unfortunately expose much more of the SwiftPM API than previously, but given it is all required to a generate a `BuildPlan`, this was really already the case. Long term this module should be replaced by a long-running BSP server that can prepare and provide build settings in the one process. Resolves rdar://102242345.
bnbarham
force-pushed
the
plan-should-build-tool-plugins
branch
from
16e7747 to
4fa0769
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
| ) | ||
|
|
||
| let plan = try await operation.generatePlan() | ||
| return (BuildDescription(buildPlan: plan), bufferedOutput.bytes.description) |
There was a problem hiding this comment.
question: The single test added validate a single load call, more specifically, the description, which is of type BuildDescription.
- Do other tests exist which validate
BuildOperation.generatePlan()in isolation? - Do other tests exists that validate
bufferedOutput.bytes.descriptionis populated as expected based on the inputs toBuildOperationand the actiongeneratePlan()?
The test is only validation a single case for the BuildDescription. Can we add a test that validate the error: String , or do other tests validate the
There was a problem hiding this comment.
generatePlan only has package visibility and is an internal implementation detail, I don't see a point adding a test for it.
outputStream is necessarily tested basically everywhere since it's how we output anything. Adding a test for it would just be testing BufferedOutputByteStream/ThreadSafeOutputByteStream, which seems pointless.
| ) | ||
| let scratchDirectory = tmpDir.appending(".build") | ||
|
|
||
| let loaded = try await BuildDescription.load( |
There was a problem hiding this comment.
question: Is there value in adding additional tests that will validate the BuildDescription.load behaves as expected considering different destination Build Parameter, different graph, filesystem layout, scratch directory?
There was a problem hiding this comment.
It is just calling into BuildOperation, which we have a billion other tests for. So no, I don't think there's particular value in that here.
Build tool plugins need to be built and run as part of the build plan generation. Replace the existing API taking a
BuildPlanwith one that generates it instead. This unfortunately exposes much more of the SwiftPM API than previously, but given it is all required to a generate aBuildPlan, this was really the case for all clients anyway.Long term this module should be replaced by a long-running BSP server that can prepare and provide build settings in the one process.
Resolves rdar://102242345.