Fixed compilation on Mac Catalyst #306

pokryfka · 2020-09-11T04:34:39Z

Fix compilation on macCatalyst platform.

Motivation:

Support compilation on macCatalyst platform.

Modifications:

Updated availability guards in TLSConfiguration.

Result:

Fixes compilation on macCatalyst platform.
Closes #304

swift-server-bot · 2020-09-11T04:34:41Z

Can one of the admins verify this patch?

swift-server-bot · 2020-09-11T04:34:41Z

Can one of the admins verify this patch?

swift-server-bot · 2020-09-11T04:34:41Z

Can one of the admins verify this patch?

swift-server-bot · 2020-09-11T04:34:42Z

Can one of the admins verify this patch?

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift

pokryfka · 2020-09-11T04:38:44Z

note that SSLProtocol is depreciated

/**
 * @enum SSLProtocol enumeration
 * @abstract Enumerations for the set of supported TLS and DTLS protocol versions.
 *
 * @note This enumeration is deprecated. Use `tls_protocol_version_t` instead.
 */
public enum SSLProtocol : Int32 {

    
    @available(iOS, introduced: 5.0, deprecated: 13.0)
    @available(macCatalyst, introduced: 13.0, deprecated: 13.0)
   // ...
}

pokryfka · 2020-09-11T04:39:40Z

CC @Lukasa

Lukasa · 2020-09-11T07:07:37Z

Hmm, given your notes above I went back and checked: I have no trouble building the package for macCatalyst. The fact that the macCatalyst guards are redundant suggests that you shouldn't be either. Are you confident that your Xcode build is using the Xcode toolchain and not an OSS one?

Lukasa · 2020-09-11T07:14:45Z

I'm increasingly wondering if some of the platform checking for macCatalyst is not working right, as I'm seeing build warnings about redundant checking which don't make any sense either. Let's see if we can drill down to why this is happening.

Lukasa · 2020-09-11T07:16:53Z

For now we can address the warning about SSLProtocol. If you add @available(iOS, introduced: 5.0, deprecated: 13.0) to TLSVersion.sslProtocol then that will nicely resolve the build warnings for that.

pokryfka · 2020-09-11T07:47:05Z

Hmm, given your notes above I went back and checked: I have no trouble building the package for macCatalyst. The fact that the macCatalyst guards are redundant suggests that you shouldn't be either. Are you confident that your Xcode build is using the Xcode toolchain and not an OSS one?

How do you build it?

I have only Xcode installed, OSS Swift running typically in Docker;
I tested compilation using Xcode 11 and 12beta.
I have not generated XCode project opening the package manifest file Package.swift in Xcode.

The problem came up after importing AHC as Swift package to another iOS Xcode project (supporting macCatalyst platform).

Lukasa · 2020-09-11T07:48:09Z

I've been building directly by opening the Package.swift in Xcode using Xcode 12 beta.

pokryfka · 2020-09-11T07:54:05Z

I've been building directly by opening the Package.swift in Xcode using Xcode 12 beta.

Fails 100% time for me...

Lukasa · 2020-09-11T07:55:25Z

This is when opening async-http-client directly, not when using it as a dependency?

pokryfka · 2020-09-11T07:57:44Z

This is when opening async-http-client directly, not when using it as a dependency?

yes

Lukasa · 2020-09-11T08:04:10Z

Hmm, let me get hold of a machine that matches your setup and I'll try to replicate it.

pokryfka · 2020-09-11T08:39:19Z

Hmm, let me get hold of a machine that matches your setup and I'll try to replicate it.

Thank you, please let me know if/what to check/change on my end.
Not sure how our environments could be different.

Lukasa · 2020-09-11T11:31:47Z

Ok, I have validated this definitely occurs. I'm going to reach out to some colleagues to see if I can understand this better. So far as I know the macCatalyst availability is strictly the same as the iOS availability, so I think this is just a bug in Xcode.

Lukasa · 2020-09-14T05:49:24Z

Ok, after some chatting with my colleagues this appears to be due to the awkward combination of the following: @available(macCatalyst 13) on the replacement declaration, with @unavailable on the fallback. Strictly macCatalyst version guards are redundant with iOS version guards (the macCatalyst version is assumed from the iOS version). However, the difference is that there is no macCatalyst version before 13. This has some weird effects: we see warnings about redundant version guards in this code, and we get that weird issue where we still have to sort out an else block that has no real fallback.

I propose we change the block to this:

            if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) {
                sec_protocol_options_set_min_tls_protocol_version(options.securityProtocolOptions, self.minimumTLSVersion.nwTLSProtocolVersion)
            } else {
                #if !targetEnvironment(macCatalyst)
                sec_protocol_options_set_tls_min_version(options.securityProtocolOptions, self.minimumTLSVersion.sslProtocol)
                #else
                preconditionFailure("macCatalyst 13 is the first version of macCatalyst")
                #endif
            }

This code is pretty hideous, but it avoids the concern I have about that block scaling up to being too big. We can do the same in the other block. Want to update to that?

pokryfka · 2020-09-14T07:22:40Z

@Lukasa thank you for following up on that,

updated PR as suggested, let me know if you have more comments
feel free to close this PR and submit a patch on your own if its faster that way;

BTW when compiling for Mac Catalyst there is still warning:

Unnecessary check for 'iOS'; enclosing scope ensures guard will always be true

Not sure if it makes sense to try to fix it (for example by putting the availability inside of targetEnvironment condition),
IMO it would make the logic even less clear

Lukasa · 2020-09-14T07:35:26Z

BTW when compiling for Mac Catalyst there is still warning:

Unnecessary check for 'iOS'; enclosing scope ensures guard will always be true

Not sure if it makes sense to try to fix it (for example by putting the availability inside of targetEnvironment condition),
IMO it would make the logic even less clear

Yeah, I'm aware of it: I've reported it as a bug upstream. For now I think we have to tolerate it.

Lukasa

Great, LGTM. Thanks!

Lukasa · 2020-09-14T07:40:03Z

@swift-server-bot test this please

Lukasa · 2020-09-14T07:41:12Z

Just some minor formatting issues to cleanup there.

pokryfka · 2020-09-14T11:00:01Z

@swift-server-bot test this please

Lukasa · 2020-09-14T11:14:58Z

@swift-server-bot test this please

Lukasa · 2020-09-14T11:22:32Z

Aaand now we bump into SR-11560.

Lukasa · 2020-09-14T12:58:57Z

There are probably no good routes out of this. Linux doesn't believe macCatalyst exists, so any reference to it will warn, but we have to refer to it in order to get a safe macCatalyst build because it has an explicit unavailable marker for the nonexistent macCatalyst 12-and-earlier version.

pokryfka · 2020-09-14T13:19:22Z

There are probably no good routes out of this. Linux doesn't believe macCatalyst exists, so any reference to it will warn, but we have to refer to it in order to get a safe macCatalyst build because it has an explicit unavailable marker for the nonexistent macCatalyst 12-and-earlier version.

how to define macCatalyst without using its name lol

one way to solve this would be to drop support for iOS12.
is it feasible once iOS14 is officially released (which is days)?

that way AHC would support 2 stable iOS versions and Mac Catalyst

Lukasa · 2020-09-17T10:11:18Z

Ok, after some digging I've come up with a proposal.

If we wrap the #if targetEnvironment block in #if swift(>=5.3) this should not emit warnings: Swift 5.3 understands that macCatalyst is a real environment. So I think what we're going to do is say that macCatalyst is only supported on Swift 5.3, and wrap this in #if swift(>=5.3).

Does that sound acceptable to you @pokryfka?

pokryfka · 2020-09-22T02:22:33Z

@Lukasa

Thank you for following on that.
Amazing weather here and trying to stay away from computer, I am sorry for late response.

So I think what we're going to do is say that macCatalyst is only supported on Swift 5.3,

To be honest supporting only iOS14/Sift5.3 is not ideal but an improvement nevertheless.

If we wrap the #if targetEnvironment block in #if swift(>=5.3) this should not emit warning

I am not sure I understood it correctly, just tried (ugly!):

            if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) {
                sec_protocol_options_set_min_tls_protocol_version(options.securityProtocolOptions, self.minimumTLSVersion.nwTLSProtocolVersion)
            } else {
                #if swift(>=5.3)
                    #if !targetEnvironment(macCatalyst)
                        sec_protocol_options_set_tls_min_version(options.securityProtocolOptions, self.minimumTLSVersion.sslProtocol)
                    #else
                        preconditionFailure("macCatalyst 13 is the first version of macCatalyst")
                    #endif
                #endif
            }

but it still fails on Linux with Swift 5.2:

docker-compose -f docker/docker-compose.yaml -f docker/docker-compose.1804.52.yaml -p async-http-client-swift52-prb run --rm test

/code/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift:72:44: error: unknown target environment for build configuration 'targetEnvironment'
                    #if !targetEnvironment(macCatalyst)
                                           ^
/code/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift:72:44: note: did you mean 'simulator'?
                    #if !targetEnvironment(macCatalyst)
                                           ^~~~~~~~~~~
                                           simulator

Lukasa · 2020-09-22T07:41:50Z

Try #if compiler instead of #if swift

pokryfka · 2020-09-22T10:00:52Z

Try #if compiler instead of #if swift

                #if compiler(>=5.3)
                    #if !targetEnvironment(macCatalyst)
                        sec_protocol_options_set_tls_min_version(options.securityProtocolOptions, self.minimumTLSVersion.sslProtocol)
                    #else
                        preconditionFailure("macCatalyst 13 is the first version of macCatalyst")
                    #endif
                #endif

Same, does not compile on Linux and Swift5.2 tested using the docker compose file in the repo:

docker-compose -f docker/docker-compose.yaml -f docker/docker-compose.1804.52.yaml -p async-http-client-swift52-prb run --rm test

weissi · 2021-01-18T21:19:22Z

@swift-server-bot test this please

weissi · 2021-01-18T21:24:24Z

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift

@@ -67,15 +68,23 @@
            if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) {
                sec_protocol_options_set_min_tls_protocol_version(options.securityProtocolOptions, self.minimumTLSVersion.nwTLSProtocolVersion)
            } else {
-                sec_protocol_options_set_tls_min_version(options.securityProtocolOptions, self.minimumTLSVersion.sslProtocol)
+                #if !targetEnvironment(macCatalyst)


@pokryfka you'll need to do

#if compiler(>=5.3) #if !targetEnvironment(macCatalyst) sec_protocol_options_set_tls_min_version(options.securityProtocolOptions, self.minimumTLSVersion.sslProtocol) #else preconditionFailure("macCatalyst 13 is the first version of macCatalyst") #endif #else sec_protocol_options_set_tls_min_version(options.securityProtocolOptions, self.minimumTLSVersion.sslProtocol) #endif

friendly ping @pokryfka

thank you for the reminder and previous comment

my (personal) project using Catalyst and AHC, and in fact swift-server in general, is waiting for my professional contract to finish, which is in a few weeks 🎉

anyway, its a small change, and would be great to be able to use official AHC in my project

will test it and update the PR tomorrow

pokryfka · 2021-05-12T15:21:33Z

Try #if compiler instead of #if swift

                #if compiler(>=5.3)
                    #if !targetEnvironment(macCatalyst)
                        sec_protocol_options_set_tls_min_version(options.securityProtocolOptions, self.minimumTLSVersion.sslProtocol)
                    #else
                        preconditionFailure("macCatalyst 13 is the first version of macCatalyst")
                    #endif
                #endif

Same, does not compile on Linux and Swift5.2 tested using the docker compose file in the repo:

docker-compose -f docker/docker-compose.yaml -f docker/docker-compose.1804.52.yaml -p async-http-client-swift52-prb run --rm test

@weissi actually, I did not push the change back then but I did try #if compiler(>=5.3) locally,
re-tested and pushed the change now, failing with:

/code/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift:88:48: note: did you mean 'simulator'?
                        #if !targetEnvironment(macCatalyst)

weissi · 2021-05-12T15:42:09Z

@swift-server-bot test this please

weissi · 2021-10-26T08:53:20Z

@swift-server-bot test this please

Fixed compilation on Mac Catalyst

a230b4a

pokryfka commented Sep 11, 2020

View reviewed changes

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift Outdated Show resolved Hide resolved

pokryfka commented Sep 11, 2020

View reviewed changes

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift Outdated Show resolved Hide resolved

Lukasa added the semver/patch For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Sep 11, 2020

pokryfka added 2 commits September 14, 2020 15:11

Updates after review

cd3396a

Fix SSLProtocol depreciation warning

c01e323

Lukasa approved these changes Sep 14, 2020

View reviewed changes

Formatted code using swiftformat-44.6

f04d4bf

weissi reviewed Jan 18, 2021

View reviewed changes

pokryfka added 2 commits May 12, 2021 22:54

Merge branch 'master' into fix/mac_catalyst

05c69fb

Limit macCatalyst check to Swift 5.3 or later

0106719

Fixed compilation on Mac Catalyst #306

Are you sure you want to change the base?

Fixed compilation on Mac Catalyst #306

Conversation

pokryfka commented Sep 11, 2020 • edited Loading

swift-server-bot commented Sep 11, 2020

swift-server-bot commented Sep 11, 2020

swift-server-bot commented Sep 11, 2020

swift-server-bot commented Sep 11, 2020

pokryfka commented Sep 11, 2020

pokryfka commented Sep 11, 2020

Lukasa commented Sep 11, 2020

Lukasa commented Sep 11, 2020

Lukasa commented Sep 11, 2020

pokryfka commented Sep 11, 2020

Lukasa commented Sep 11, 2020

pokryfka commented Sep 11, 2020

Lukasa commented Sep 11, 2020

pokryfka commented Sep 11, 2020

Lukasa commented Sep 11, 2020

pokryfka commented Sep 11, 2020

Lukasa commented Sep 11, 2020

Lukasa commented Sep 14, 2020

pokryfka commented Sep 14, 2020

Lukasa commented Sep 14, 2020 • edited Loading

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa commented Sep 14, 2020

Lukasa commented Sep 14, 2020

pokryfka commented Sep 14, 2020

Lukasa commented Sep 14, 2020

Lukasa commented Sep 14, 2020

Lukasa commented Sep 14, 2020

pokryfka commented Sep 14, 2020 • edited Loading

Lukasa commented Sep 17, 2020

pokryfka commented Sep 22, 2020

Lukasa commented Sep 22, 2020

pokryfka commented Sep 22, 2020

weissi commented Jan 18, 2021

weissi Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

weissi May 11, 2021

Choose a reason for hiding this comment

pokryfka May 11, 2021

Choose a reason for hiding this comment

pokryfka commented May 12, 2021

weissi commented May 12, 2021

weissi commented Oct 26, 2021

pokryfka commented Sep 11, 2020 •

edited

Loading

Lukasa commented Sep 14, 2020 •

edited

Loading

pokryfka commented Sep 14, 2020 •

edited

Loading

weissi Jan 18, 2021 •

edited

Loading