Skip to content

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Oct 6, 2025

Right now our Swift CryptoKit bindings will compile with warnings.

The good news is we don't actually have any warnings, but we should guard this at build-time.

Before:

  317 |     #warning("potato!")
      |              `- warning: potato!
  318 |     let ikm = Data(bytesNoCopy: ikmPtr, count: Int(ikmLength), deallocator: Data.Deallocator.none)
  319 |     let salt = Data(bytesNoCopy: saltPtr, count: Int(saltLength), deallocator: Data.Deallocator.none)
  [ 85%] Linking C static library libSystem.Security.Cryptography.Native.Apple.a
  [ 85%] Linking C shared library libSystem.Security.Cryptography.Native.Apple.dylib
  [ 92%] Built target System.Security.Cryptography.Native.Apple-Static
  Verifying System.Security.Cryptography.Native.Apple points against entrypoints.c 
  Stripping symbols from libSystem.Security.Cryptography.Native.Apple.dylib into libSystem.Security.Cryptography.Native.Apple.dylib.dwarf
  [100%] Built target System.Security.Cryptography.Native.Appl

After:

 /Users/vcsjones/Projects/runtime/src/native/libs/System.Security.Cryptography.Native.Apple/pal_swiftbindings.swift:317:14: error: potato!
  315 |     destinationLength: Int32) -> Int32 {
  316 | 
  317 |     #warning("potato!")
      |              `- error: potato!

@vcsjones vcsjones self-assigned this Oct 6, 2025
@Copilot Copilot AI review requested due to automatic review settings October 6, 2025 18:56
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the Swift compilation process by treating warnings as errors during the build process. This ensures that any Swift warnings in the CryptoKit bindings will fail the build, preventing warnings from being silently ignored.

  • Adds the -warnings-as-errors flag to the Swift compiler command in the CMake build configuration

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

It's going to make SDK upgrades really annoying, but I think we already get that from the Objective C APIs... so this is at least consistent.

@vcsjones
Copy link
Member Author

vcsjones commented Oct 8, 2025

It's going to make SDK upgrades really annoying, but I think we already get that from the Objective C APIs... so this is at least consistent

We have yet to get a warning from an SDK update... but when I was working on HKDF I almost had a (correct!) warning slip in, so it seems like a good trade off.

@bartonjs
Copy link
Member

bartonjs commented Oct 8, 2025

We have yet to get a warning from an SDK update...

0a220d5 disagrees 😄. All of the #pragma clang diagnostic ignored "-Wdeprecated-declarations" that got added.

@vcsjones
Copy link
Member Author

vcsjones commented Oct 8, 2025

0a220d5 disagrees 😄.

I meant from CryptoKit / Swift.

@vcsjones vcsjones enabled auto-merge (squash) October 8, 2025 22:42
@vcsjones vcsjones merged commit 8d3c401 into dotnet:main Oct 9, 2025
101 checks passed
@akoeplinger
Copy link
Member

I think we should do

# Suppress warnings-as-errors in release branches to reduce servicing churn
if (PRERELEASE)
add_compile_options(-Werror)
endif(PRERELEASE)

@vcsjones
Copy link
Member Author

vcsjones commented Oct 9, 2025

I think we should do

# Suppress warnings-as-errors in release branches to reduce servicing churn
if (PRERELEASE)
add_compile_options(-Werror)
endif(PRERELEASE)

Fair enough. I can give that a go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants