-
Notifications
You must be signed in to change notification settings - Fork 20
Add support for generating Swift SDKs with multiple host triples #232
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
base: main
Are you sure you want to change the base?
Add support for generating Swift SDKs with multiple host triples #232
Conversation
365715a
to
f0f1892
Compare
This is necessary for correctness on macOS, where the concept of multi-arch binaries exists, and all swift.org toolchains for macOS are multi-arch.
f0f1892
to
f7c60ef
Compare
@swift-ci test |
@@ -121,3 +133,10 @@ struct DownloadableArtifacts: Sendable { | |||
) | |||
} | |||
} | |||
|
|||
struct StringError: Error, CustomStringConvertible { |
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 don't see any value in stringly typed APIs in Swift, especially for errors. These are harder to test against, and go against the grain of the static typing approach that Swift takes. IMO a nested enum Error: Swift.Error
within DownloadableArtifacts
with explicit cases for each error path would work, and would make it easier to adopt typed throws in the future.
try _testItemsToDownloadForMacOSHost(hostTriples: [Triple("arm64-apple-macos"), Triple("x86_64-apple-macos")]) | ||
} | ||
|
||
func _testItemsToDownloadForMacOSHost(hostTriples: [Triple]) 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.
Is this meant to be private
or fileprivate
? We should use the corresponding access modifier here if so. Otherwise what's the underscore for?
func _testItemsToDownloadForMacOSHost(hostTriples: [Triple]) throws { | |
func testItemsToDownloadForMacOSHost(hostTriples: [Triple]) 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.
Would you mind adding test cases to verify that newly added error paths are handled correctly and error cases are thrown as expected? That would ensure that the overall relative test coverage is not reduced after this PR is merged.
This is necessary for correctness on macOS, where the concept of multi-arch binaries exists, and all swift.org toolchains for macOS are multi-arch.