-
Notifications
You must be signed in to change notification settings - Fork 98
Simplify cross-platform hashing implementations #388
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 |
@swift-ci test macOS |
6bbc242
to
52b4ae3
Compare
@swift-ci test |
@swift-ci test linux |
@swift-ci test Windows |
52b4ae3
to
03eed0b
Compare
@swift-ci test |
03eed0b
to
9a2d662
Compare
@swift-ci test |
super.init(impl: SwiftCryptoHashContext<Insecure.MD5>()) | ||
#else | ||
super.init(impl: VendoredSHA256HashContext()) |
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.
This seems a little shaky to use MD5 on Windows and Apple but SHA256 on other platforms. Do you plan to unify this further? I'm even surprised the tests pass like this.
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 think we should probably standardize on SHA256 everywhere in the future. Very few tests actually depend on the specific hash values, it's just important that they remain stable across builds performed by the same build system executable
Eliminate the package dependency on swift-crypto to simplify the bootstrapped build. Instead, vendor the SHA256 implementation from swift-tools-support-core and use it as a fallback if neither WinSDK nor CryptoKit is available.
9a2d662
to
4e3949f
Compare
@swift-ci test |
Eliminate the package dependency on swift-crypto to simplify the bootstrapped build. Instead, vendor the SHA256 implementation from swift-tools-support-core and use it as a fallback if neither WinSDK nor CryptoKit is available.