Skip to content

Conversation

0xTim
Copy link
Contributor

@0xTim 0xTim commented Sep 8, 2025

Following on from #281, opened as a new PR as the conflicts were too many

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

If you've made changes to gyb files

  • I've run .script/generate_boilerplate_files_with_gyb and included updated generated files in a commit of this pull request

Motivation:

[Explain here the context, and why you're making that change. What is the problem you're trying to solve.]

Modifications:

[Describe the modifications you've done.]

Result:

[After your change, what will change.]

@0xTim 0xTim mentioned this pull request Sep 8, 2025
5 tasks
@@ -31,14 +31,12 @@ public import Foundation


// MARK: - AES.GCM + Nonce
@available(macOS 10.15, iOS 13, watchOS 6, tvOS 13, macCatalyst 13, visionOS 1.0, *)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lukasa this was the GYB script removing all of the availability annotations - I'm not sure if this is intended or not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, those annotations should stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want a separate issue for the gyb file? Looks like it hasn't been touched in a while since it was Python 2.7 up to now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please. 😄

@Lukasa
Copy link
Contributor

Lukasa commented Sep 9, 2025

Thanks Tim, I'll review this shortly. I'm trying to get a catch-up merge to land to the wwdc-25 branch I'm afraid, and this patch will make that nearly impossible: do you mind rebuilding this shortly after that lands? #403

@0xTim
Copy link
Contributor Author

0xTim commented Sep 9, 2025

Thanks Tim, I'll review this shortly. I'm trying to get a catch-up merge to land to the wwdc-25 branch I'm afraid, and this patch will make that nearly impossible: do you mind rebuilding this shortly after that lands? #403

Yep no problem

@Lukasa
Copy link
Contributor

Lukasa commented Sep 10, 2025

Ok, the branch is up-to-date, feel free to do the fixup.

@0xTim 0xTim force-pushed the rename-crypto-extras-2 branch from 61239b7 to 96ccc4e Compare September 10, 2025 22:58
@0xTim
Copy link
Contributor Author

0xTim commented Sep 10, 2025

@Lukasa ok updated!

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To minimise churn, can we add a _CryptoExtras module that only contains an @_exported import CryptoExtras? That will make it easier for folks to continue to support a wide range of versions of swift-crypto.

@0xTim
Copy link
Contributor Author

0xTim commented Sep 11, 2025

To minimise churn, can we add a _CryptoExtras module that only contains an @_exported import CryptoExtras? That will make it easier for folks to continue to support a wide range of versions of swift-crypto.

Won't having a product named _CryptoExtras that points to the CryptoExtras target solve that?

@Lukasa
Copy link
Contributor

Lukasa commented Sep 12, 2025

I am...honestly not sure. Historically I've avoided wanting to do that to avoid any risk of the code being generated twice.

@0xTim
Copy link
Contributor Author

0xTim commented Sep 12, 2025

Ok updated it, I tried to use it with JWTKit without the new module and the compiler fell over so looks like it is needed

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Sep 15, 2025
@@ -0,0 +1 @@
@_exported import CryptoExtras
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs a license header on it, strangely.

@Lukasa
Copy link
Contributor

Lukasa commented Sep 15, 2025

The format checker has some notes.

@Lukasa
Copy link
Contributor

Lukasa commented Sep 15, 2025

API breakage is expected, merging over it.

@Lukasa Lukasa merged commit 4e662b8 into apple:wwdc-25 Sep 15, 2025
39 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants