Skip to content
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

Remove inlinable from init() to support library evolution on linux #294

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

2horse9sun
Copy link

Motivation:

I'm currently working on a swift package project which depends on the swift-crypto library. The package is distributed to other teams and built on multiple platforms (macos + linux). Most importantly, the package is required to enable swift library evolution.

However, after the library evolution is enabled, the linux build failed with following error:

error: emit-module command failed with exit code 1 (use -v to see invocation)
/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/Ed25519_boring.swift:90:27: error: 'self' used before 'self.init' call or assignment to 'self'
 88 |         @inlinable
 89 |         init<D: ContiguousBytes>(rawRepresentation: D) throws {
 90 |             self.keyBytes = try rawRepresentation.withUnsafeBytes { keyBytesPtr in
    |                           `- error: 'self' used before 'self.init' call or assignment to 'self'
 91 |                 guard keyBytesPtr.count == 32 else {
 92 |                     throw CryptoKitError.incorrectKeySize

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/Ed25519_boring.swift:96:9: error: 'self.init' isn't called on all paths before returning from initializer
 94 |                 return Array(keyBytesPtr)
 95 |             }
 96 |         }
    |         `- error: 'self.init' isn't called on all paths before returning from initializer
 97 | 
 98 |         init(_ keyBytes: [UInt8]) {

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/NISTCurvesKeys_boring.swift:109:18: error: 'self' used before 'self.init' call or assignment to 'self'
107 |     @inlinable
108 |     init(wrapping key: BoringSSLECPublicKeyWrapper<Curve>) {
109 |         self.key = key
    |                  `- error: 'self' used before 'self.init' call or assignment to 'self'
110 |     }
111 | 

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/NISTCurvesKeys_boring.swift:110:5: error: 'self.init' isn't called on all paths before returning from initializer
108 |     init(wrapping key: BoringSSLECPublicKeyWrapper<Curve>) {
109 |         self.key = key
110 |     }
    |     `- error: 'self.init' isn't called on all paths before returning from initializer
111 | 
112 |     @inlinable

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/X25519Keys_boring.swift:32:27: error: 'self' used before 'self.init' call or assignment to 'self'
 30 |         @inlinable
 31 |         init<D: ContiguousBytes>(rawRepresentation: D) throws {
 32 |             self.keyBytes = try rawRepresentation.withUnsafeBytes { dataPtr in
    |                           `- error: 'self' used before 'self.init' call or assignment to 'self'
 33 |                 guard dataPtr.count == Curve25519.KeyAgreement.keySizeBytes else {
 34 |                     throw CryptoKitError.incorrectKeySize

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/X25519Keys_boring.swift:39:9: error: 'self.init' isn't called on all paths before returning from initializer
 37 |                 return Array(dataPtr)
 38 |             }
 39 |         }
    |         `- error: 'self.init' isn't called on all paths before returning from initializer
 40 | 
 41 |         @usableFromInline
/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/NISTCurvesKeys_boring.swift:109:18: error: 'self' used before 'self.init' call or assignment to 'self'
107 |     @inlinable
108 |     init(wrapping key: BoringSSLECPublicKeyWrapper<Curve>) {
109 |         self.key = key
    |                  `- error: 'self' used before 'self.init' call or assignment to 'self'
110 |     }
111 | 

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/NISTCurvesKeys_boring.swift:110:5: error: 'self.init' isn't called on all paths before returning from initializer
108 |     init(wrapping key: BoringSSLECPublicKeyWrapper<Curve>) {
109 |         self.key = key
110 |     }
    |     `- error: 'self.init' isn't called on all paths before returning from initializer
111 | 
112 |     @inlinable

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/X25519Keys_boring.swift:32:27: error: 'self' used before 'self.init' call or assignment to 'self'
 30 |         @inlinable
 31 |         init<D: ContiguousBytes>(rawRepresentation: D) throws {
 32 |             self.keyBytes = try rawRepresentation.withUnsafeBytes { dataPtr in
    |                           `- error: 'self' used before 'self.init' call or assignment to 'self'
 33 |                 guard dataPtr.count == Curve25519.KeyAgreement.keySizeBytes else {
 34 |                     throw CryptoKitError.incorrectKeySize

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/X25519Keys_boring.swift:39:9: error: 'self.init' isn't called on all paths before returning from initializer
 37 |                 return Array(dataPtr)
 38 |             }
 39 |         }
    |         `- error: 'self.init' isn't called on all paths before returning from initializer
 40 | 
 41 |         @usableFromInline

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/Ed25519_boring.swift:90:27: error: 'self' used before 'self.init' call or assignment to 'self'
 88 |         @inlinable
 89 |         init<D: ContiguousBytes>(rawRepresentation: D) throws {
 90 |             self.keyBytes = try rawRepresentation.withUnsafeBytes { keyBytesPtr in
    |                           `- error: 'self' used before 'self.init' call or assignment to 'self'
 91 |                 guard keyBytesPtr.count == 32 else {
 92 |                     throw CryptoKitError.incorrectKeySize

/tmp/MyLibrary/.build/checkouts/swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/Ed25519_boring.swift:96:9: error: 'self.init' isn't called on all paths before returning from initializer
 94 |                 return Array(keyBytesPtr)
 95 |             }
 96 |         }
    |         `- error: 'self.init' isn't called on all paths before returning from initializer
 97 | 
 98 |         init(_ keyBytes: [UInt8]) {

The above error can be easily reproduced by:

  1. Create a new swift package project on a Linux OS.
  2. Add swift-crypto to the target dependency
  3. Import Crypto somewhere in the source code
  4. Run this command to build with library evolution: swift build -c release -Xswiftc -emit-module-interface -Xswiftc -enable-library-evolution

Modifications:

After some investigation on this error, I found a useful github issue that may be relevant: swiftlang/swift#62507

As shown in the issue discussion:

it is not safe to have an inlinable public initializer that initializes storage directly in a non-@Frozen struct

Therefore, I just remove "inlinable" from init() of three structs.

Result:

After the modification, it successfully builds on Linux. (At least fix my problem :)

But feel free to give more suggestions on how to support library evolution in the long term instead of just removing these 'inlinable'. Somehow I feel like it's not elegant enough.

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Nov 18, 2024
@Lukasa
Copy link
Contributor

Lukasa commented Nov 18, 2024

Thanks for this ticket!

I'm a little bit surprised to hear you're compiling with library evolution mode on Linux. Swift's ABI is not stable on Linux, and so there is no promise that library evolution mode does anything at all. Can you elaborate a bit on what you're trying to achieve?

@2horse9sun
Copy link
Author

Thanks for this ticket!

I'm a little bit surprised to hear you're compiling with library evolution mode on Linux. Swift's ABI is not stable on Linux, and so there is no promise that library evolution mode does anything at all. Can you elaborate a bit on what you're trying to achieve?

Sorry for the delay :)

Here's the brief description of what we're trying to achieve. Out team is responsible for building the infrastructure and platform for company internal use. We're distributing Swift SDKs for plugin development to different teams across the company. Later, we load those plugins (dylibs) to integrate with our platform. One basic requirement is to enable library evolution to ensure backward compatibility and avoid breaking all plugins.

It all works fine under macOS environment. However, we have to also make it work under Linux environment for the following reasons:

  1. We have limited number of macOS CI machines. It's usually easier to implement CI pipelines on Linux VMs. Also, it is not possible for our team to request more macOS machines to merely support library evolution.
  2. We have also built a web service to run those plugins. For the most of the time, web services run under Linux environment and we're not able to migrate our web service to macOS environment to merely support library evolution.

As you said, "Swift's ABI is not stable on Linux, and so there is no promise that library evolution mode does anything at all". It is true and we have already noticed that every time a new version of our SDK is released, it might break some plugins under Linux environment but not under macOS environment. Currently, our team is also trying to figure out other solutions including disabling library evolution under Linux environment.

Anyway, our current workaround to use swift-crypto under Linux environment with library evolution enabled is to take advantage of dependency injection to set the crypto instance at the application level and provide only the protocol in the SDK. It works fine at least for now.

Feel free to give any more suggestions :)

@Lukasa
Copy link
Contributor

Lukasa commented Dec 2, 2024

Thanks for explaining your use-case, it helps with my understanding.

Within a single compiler version, you should be able to do what you want without library evolution mode at all. Given that library evolution mode is not typically supported across the wider Package ecosystem, you may find that the best strategy available to you is to disregard library evolution mode entirely. You can still use dynamic linking, you just have to make sure the compiler and runtime versions all line up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants