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

Fix build on non-macOS Apple platforms #164

Merged

Conversation

simonjbeaumont
Copy link
Contributor

Motivation

When building this project for Apple platforms other than macOS, the build failed for a couple of reasons. First, NIOSSHServer requires Foundation.Process which is only available on macOS. Second, the tests are making use of an CryptoKit API that's only present on iOS 13.2 (and aligned other versions), which is newer than the 13.0 stated in the Package.swift:

error: Cannot find type 'Process' in scope
...
error: 'isValidAuthenticationCode(_:authenticating:using:)' is only available in iOS 13.2 or newer

Modifications

  • Wrap all the code in NIOSSHServer with #if canImport(Foundation.Process) and, in main.swift, throw a fatalError with a helpful message.
  • Add missing availability guards in test code. I've tried to keep them as scoped as possible.

Result

Source and tests can now build on Apple platforms other than macOS.

@@ -12,6 +12,8 @@
//
//===----------------------------------------------------------------------===//

#if canImport(Foundation.Process)
Copy link
Member

Choose a reason for hiding this comment

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

TIL that you can import check types and not only modules.

With that being said, why don't we use @available instead like we do in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that doesn't stop it trying to compile the code. If Foundation.Process was present on e.g. iOS but not available, then we could use @available here too, but it is simply not present in the iOS SDK (and others), so we must use a compiler directive.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense! Thanks

@simonjbeaumont simonjbeaumont merged commit d7d410f into apple:main Dec 18, 2023
7 of 8 checks passed
@Lukasa Lukasa added the needs-no-version-bump For PRs that when merged do not need a bump in version number. label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-no-version-bump For PRs that when merged do not need a bump in version number.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants