-
Notifications
You must be signed in to change notification settings - Fork 174
Typealias CryptoKitError #285
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?
Conversation
@Lukasa is there anything more that needs to be done on 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 a lower-impact way of doing this would be to avoid renaming the file, and to put the typealiases in a file whose name ends in _boring.swift
. That extension makes it clear to the tooling that keeps this in sync with CryptoKit that it's an extension for Crypto only, and so would ensure it's preserved over API syncs.
Does that work for you?
@Lukasa ok updated the PR to avoid any filename changes and kept the renames to just the API docs (as opposed to changing the code to throw a I think the CryptoExtras code should be updated to remove any references to CryptoKit but that can be done in a separate PR |
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.
Agreed on the follow-up PR as well.
@@ -0,0 +1,4 @@ | |||
@available(iOS 13.0, macOS 10.15, watchOS 6.0, tvOS 13.0, *) |
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.
Sorry to be a bore here, but you'll need the license header on this file.
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.
Ah good point, added
Ah shoot, can you rebuild the CMake pieces? There's a script. |
gen/bcm/sha512-armv8-linux.S | ||
gen/bcm/vpaes-armv8-linux.S | ||
gen/crypto/chacha-armv8-linux.S | ||
gen/crypto/chacha20_poly1305_armv8-linux.S) |
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.
Hmm, this looks incorrect. Oh shoot, I think we did some upstream work on this. Try this one? https://github.com/apple/swift-nio/blob/main/scripts/update-cmake-lists.sh
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.
That's the script I copied from when running. Do you get the same result running the script?
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 I had the same problem, do you also get the error scripts/update_cmakelists.sh: line 58: gfind: command not found
or something similar? In my case the solution was to download GNU find with brew install findutils
Provides a typealias for
CryptoKitError
to hide the implementation detail of CryptoKit. Resolves #274Checklist
If you've made changes to
gyb
files.script/generate_boilerplate_files_with_gyb
and included updated generated files in a commit of this pull requestMotivation:
CryptoKit is an implementation detail on Apple platforms - this makes it clearer for users coming from other platforms
Modifications:
Added a type alias and updated documentation
Result:
[After your change, what will change.]