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

Add support for visionOS #79

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

Add support for visionOS #79

wants to merge 1 commit into from

Conversation

mman
Copy link
Contributor

@mman mman commented Nov 29, 2023

Description

This PR makes Cryptor compile cleanly for visionOS targets. It's a work in progress but should be fairly straightforward and clean.

How Has This Been Tested?

Compiling my visionOS app target with Cryptor added as a dependency.

Checklist:

  • I have submitted a CLA form
  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

@dannys42
Copy link
Contributor

Thanks! Looks like CI is not currently running. Do you happen to know whether this compiles successfully on Swift 5.0?

@mman
Copy link
Contributor Author

mman commented Dec 1, 2023

@dannys42 Good catch, Looks like os(visionOS) is compiler specific and thus can not be used before Swift 5.9.

/src/Sources/Cryptor/Utilities.swift:209:61: warning: unknown operating system for build configuration 'os'
                #if os(macOS) || os(iOS) || os(tvOS) || os(watchOS) || os(visionOS)
                                                                          ^

It can not be used inside a compound single line statement like this either:

x.swift:1:24: warning: unknown operating system for build configuration 'os'
#if swift(>=5.9) && os(visionOS)

This is actually a bummer because testing for visionOS requires #if swift(>=5.9) standing on its own line... so every check will look like this:

#if swift(>=5.9)
#if os(visionOS)
#endif
#endif

Ideas? Perhaps we can turn the checks the other way around, that is test for Linux:

#if os(Linux)
// use OpenSSL
#else
// use CommonCrypto
#endif

or test for #if canImport(CommonCrypto)

#if canImport(CommonCrypto)
// use CommonCrypto
#else
// use OpenSSL
#endif

ideas?

@dannys42
Copy link
Contributor

dannys42 commented Dec 1, 2023

@mman Thanks for checking! Yes, swift is a bit annoying in this space. I think your last option is probably the safest approach, though:

#if canImport(CommonCrypto)
// use CommonCrypto
#else
// use OpenSSL
#endif

Also it seems closest to the actual intent -- i.e. it's not really the OS that we are about, it is the library we can import.

Copy link

sonarcloud bot commented Dec 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mman
Copy link
Contributor Author

mman commented Dec 1, 2023

@dannys42 Reworked to use #if canImport(CommonCrypto) where appropriate. Tested using Xcode 15 against iOS, macOS, watchOS, tvOS, and visionOS Simulators. All tests pass.

Tested on Linux arm64 swift 5.7, 5.8, 5.9 via Docker (older docker swift images are Intel only and that I do not have anymore).

Build OK, Tests fail on:

CryptorTests/CryptorTests.swift:409: Fatal error: Unexpectedly found nil while unwrapping an Optional value

Not sure what is that, needs more investigation...

@dannys42
Copy link
Contributor

dannys42 commented Dec 5, 2023

@mman thanks so much for running those tests! Can you clarify which platform / version where you saw the unit test failure? (I don't see an issue on macOS with Xcode 15.) Is it all Linux versions you were able to test?

@mman
Copy link
Contributor Author

mman commented Dec 5, 2023

Can you clarify which platform / version where you saw the unit test failure? (I don't see an issue on macOS with Xcode 15.) Is it all Linux versions you were able to test?

Docker swift versions 5.0, 5.1, 5.2, 5.3, 5.4, 5.6 on linux/x86_64 pass!

Docker swift versions 5.7, 5.8, 5.9 on linux/x86_64 fail consistently.

Docker swift 5.7, 5.8, 5.9 on linux/arm64 fail consistently.

Test Case 'CryptorTests.test_Cryptor_DES_EBC_1' started at 2023-12-10 15:35:51.001
CryptorTests/CryptorTests.swift:409: Fatal error: Unexpectedly found nil while unwrapping an Optional value

Will debug it further and let you know what I found @dannys42.

@Jeehut
Copy link

Jeehut commented Jul 9, 2024

I'd love to see this getting merged. Is there anything holding it off? The changes seem quite safe and shouldn't break anything. Just the merge conflict might need to get fixed. I'd love to be able to finally use Swift-JWT on visionOS! 🎉

@mman Thanks for your work, any help needed?

@mman
Copy link
Contributor Author

mman commented Jul 12, 2024

@Jeehut Yes, the visionOS changes are super simple and isolated, yet as you can see from my last comment test suite is failing on a lot of systems - probably not because of my changes but that was something I wanted a clear opinion on.

@mman
Copy link
Contributor Author

mman commented Jul 12, 2024

My debugging so far went nowhere. The lowest level C calls are returning unexpected error codes and I was not able to trace down why/what is failing so far.

@Jeehut
Copy link

Jeehut commented Jul 12, 2024

@mman Thank you for the quick answer. Have you considered checking if the same test suite fails happen if you run from the main branch? If yes, then it should be a separate issue, no?

@mman
Copy link
Contributor Author

mman commented Jul 19, 2024

@Jeehut Yes, the master branch fails consistently since swift 5.7. I have filed a separate issue for this.

Copy link

sonarcloud bot commented Nov 20, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants