-
Notifications
You must be signed in to change notification settings - Fork 1.4k
SwiftBuildSystem: Expose public API for reuse by SourceKit-LSP/BSP #8842
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.
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
Conversation
@swift-ci test |
Reorganize some of the SwiftBuildSystem code so we can share the implementation of - session creation - writing PIF - build request construction
a518189
to
6fa6634
Compare
@swift-ci test |
@swift-ci test Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general idea, however I believe we need to make some changes to the API, and add automated tests to ensure we preserve the contract behaviour.
@@ -57,44 +57,52 @@ func withService( | |||
await service.close() | |||
} | |||
|
|||
func withSession( | |||
public func createSession( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (blocking): if we are exposing a public API, we need to add automated tests ensuring we don't break the "contract".
would you mind adding tests that verify each public API in isolation. This would provide an indication of whether the API makes sense to the caller.
This comment applies to all public API where it makes sense.
@@ -700,6 +707,16 @@ public final class SwiftBuildSystem: SPMBuildCore.BuildSystem { | |||
} | |||
} | |||
|
|||
public func writePIF(buildParameters: BuildParameters) async throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (blocking): looking at the API in isolation without looking at the implementation, there is nothing that provide where the PIF is being written to. As consumer of the API, how can we get the location of where the PIF was written?
The API should be update to either accept a path where to write the PIF or maybe a return either (1) the path where the PIF, or (2) the pif contents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The location should be opaque to the client (e.g. maybe in future it's not written to the filesystem at all), perhaps the API could be called something more general, like "constructPIF"?
Reorganize some of the SwiftBuildSystem code so we can share the implementation of
There should be no functional change to SwiftPM