-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Refactor generation of SHA256 checksums #3116
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
Changes from all commits
0abc687
d5638e1
f47aad0
a986a18
e77c952
f65c8a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/* | ||
This source file is part of the Swift.org open source project | ||
Copyright (c) 2020 Apple Inc. and the Swift project authors | ||
Licensed under Apache License v2.0 with Runtime Library Exception | ||
See http://swift.org/LICENSE.txt for license information | ||
See http://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
*/ | ||
|
||
import TSCBasic | ||
|
||
extension ByteString { | ||
/// A lowercase, hexadecimal representation of the SHA256 hash | ||
/// generated for the byte string's contents. | ||
/// | ||
/// This property uses the CryptoKit implementation of | ||
/// Secure Hashing Algorithm 2 (SHA-2) hashing with a 256-bit digest, when available, | ||
/// falling back on a native implementation in Swift provided by TSCBasic. | ||
public var sha256Checksum: String { | ||
#if canImport(CryptoKit) | ||
if #available(macOS 10.15, *) { | ||
return CryptoKitSHA256().hash(self).hexadecimalRepresentation | ||
} | ||
#endif | ||
|
||
return SHA256().hash(self).hexadecimalRepresentation | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -303,6 +303,13 @@ public class Workspace { | |
self.downloader = downloader | ||
self.netrcFilePath = netrcFilePath | ||
self.archiver = archiver | ||
|
||
var checksumAlgorithm = checksumAlgorithm | ||
#if canImport(CryptoKit) | ||
if checksumAlgorithm is SHA256, #available(macOS 10.15, *) { | ||
checksumAlgorithm = CryptoKitSHA256() | ||
} | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this feels a bit hacky... could we change the ctor argument There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this version has clearer semantics; the default is SHA256, not public init(
dataPath: AbsolutePath,
editablesPath: AbsolutePath,
pinsFile: AbsolutePath,
manifestLoader: ManifestLoaderProtocol,
repositoryManager: RepositoryManager? = nil,
currentToolsVersion: ToolsVersion = ToolsVersion.currentToolsVersion,
toolsVersionLoader: ToolsVersionLoaderProtocol = ToolsVersionLoader(),
delegate: WorkspaceDelegate? = nil,
config: Workspace.Configuration = Workspace.Configuration(),
fileSystem: FileSystem = localFileSystem,
repositoryProvider: RepositoryProvider = GitRepositoryProvider(),
downloader: Downloader = FoundationDownloader(),
netrcFilePath: AbsolutePath? = nil,
archiver: Archiver = ZipArchiver(),
checksumAlgorithm: HashAlgorithm? = nil,
) {
// ...
var checksumAlgorithm = checksumAlgorithm ?? SHA256()
#if canImport(CryptoKit)
if #available(macOS 10.15, *) {
checksumAlgorithm = CryptoKitSHA256()
}
#endif
self.checksumAlgorithm = checksumAlgorithm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my point was to try and avoid allocating memory twice. since this is the workspace and it usually only initialized once I guess this is not very critical. lets take the PR as is and we can modify it later if we decide to |
||
self.checksumAlgorithm = checksumAlgorithm | ||
self.isResolverPrefetchingEnabled = isResolverPrefetchingEnabled | ||
self.skipUpdate = skipUpdate | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/* | ||
This source file is part of the Swift.org open source project | ||
|
||
Copyright (c) 2020 Apple Inc. and the Swift project authors | ||
Licensed under Apache License v2.0 with Runtime Library Exception | ||
|
||
See http://swift.org/LICENSE.txt for license information | ||
See http://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
*/ | ||
|
||
import Basics | ||
import TSCBasic | ||
import XCTest | ||
|
||
final class ByteStringExtensionsTests: XCTestCase { | ||
func testSHA256Checksum() { | ||
let byteString = ByteString(encodingAsUTF8: "abc") | ||
XCTAssertEqual(byteString.contents, [0x61, 0x62, 0x63]) | ||
|
||
// See https://csrc.nist.gov/csrc/media/projects/cryptographic-standards-and-guidelines/documents/examples/sha_all.pdf | ||
XCTAssertEqual(byteString.sha256Checksum, "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad") | ||
} | ||
} |
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 it make sense to return the actual data here, and then the call site can convert it to hex, or base-whatever, etc? The representation seems orthogonal to the computation.
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.
Looking at the existing call sites, we only call
SHA256().hash(bytes)
to produce a hexadecimal representation. ReturningByteString
here would be more flexible, but that may be unnecessary. Really, it's a matter of taste; I'm happy with any solution that makes it easy to opt-in to a faster hash function when available.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.
It's fine to leave this in this PR. I just have a general preference for separating out functionality into orthogonal parts, but since this is already an extension ByteString, it's fine to leave as is.
ByteString came about back before String had a UTF-8 backing, so performance was a problem in going between UTF-8 data and String. My sense is that we could probably transition to String here over time, at which point we might want to revisit 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.
A big 👍 for transitioning away from
ByteString
at some point. When I started working on the codebase for registry support, I found the use of TSC / SPM-specific APIs instead of Swift Standard Library and Foundation (and now System) counterparts to be a frequent point of frustration. Many of these APIs were necessary at the time (e.g.JSON
before Codable), but have now become technical debt.