Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

System.Native: Make configure.cmake checks work for iOS#42408

Merged
stephentoub merged 1 commit intodotnet:masterfrom
akoeplinger:cmake-ios
Nov 6, 2019
Merged

System.Native: Make configure.cmake checks work for iOS#42408
stephentoub merged 1 commit intodotnet:masterfrom
akoeplinger:cmake-ios

Conversation

@akoeplinger
Copy link
Member

Apple platforms like macOS/iOS allow targeting older operating system versions with a single SDK, the mere presence of a symbol in the SDK doesn't tell us whether the deployment target really supports it. The compiler raises a warning when using an unsupported API, turn that into an error so check_symbol_exists() can correctly identify whether the API is supported on the target.

This fixes compile errors after #37583 was merged since clonefile() is iOS10+.

Replace usages of check_function_exists() since it only tests for existence of functions and the CMake docs recommend using check_symbol_exists(): https://cmake.org/cmake/help/v3.15/module/CheckFunctionExists.html

Apple platforms like macOS/iOS allow targeting older operating system versions with a single SDK,
the mere presence of a symbol in the SDK doesn't tell us whether the deployment target really supports it.
The compiler raises a warning when using an unsupported API, turn that into an error so `check_symbol_exists()`
can correctly identify whether the API is supported on the target.

This fixes compile errors after dotnet#37583 was merged since `clonefile()` is iOS10+.

Replace usages of `check_function_exists()` since it only tests for existence of functions and the CMake docs
recommend using `check_symbol_exists()`: https://cmake.org/cmake/help/v3.15/module/CheckFunctionExists.html
Copy link
Member

@filipnavara filipnavara left a comment

Choose a reason for hiding this comment

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

The change itself looks fine to me.

I do, however, think that the macOS and iOS minimum version requirements should be aligned for CoreFX - ie. macOS 10.X minumum requirement should map to iOS X-2 minimum requirement. At the moment the minumum requirement for macOS is 10.13 which would map to iOS 11. Anything older than iOS 10 is going to be difficult to support because of Crypto API differences. It's also bit unfortunate to lose clonefile support even on platforms that support it.

@akoeplinger
Copy link
Member Author

akoeplinger commented Nov 6, 2019

Agreed. It looks like we currently don't pass any minimum target version for macOS to CMake so it uses the version of the build host. That should be fixed.

It's also bit unfortunate to lose clonefile support even on platforms that support it.

For the reason mentioned above this doesn't happen.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks.

@stephentoub stephentoub merged commit 879127e into dotnet:master Nov 6, 2019
@akoeplinger akoeplinger deleted the cmake-ios branch November 6, 2019 15:10
akoeplinger added a commit to akoeplinger/corefx that referenced this pull request Nov 6, 2019
As noticed in dotnet#42408 we don't pass any minimum OSX target version to CMake.
This means it falls back to the host version, e.g. if you build on Mojave it would pass `-mmacosx-version-min=10.14` to the compiler.

.NET Core's minimum supported OSX version is 10.13 so pass that to CMake.
stephentoub pushed a commit that referenced this pull request Nov 6, 2019
As noticed in #42408 we don't pass any minimum OSX target version to CMake.
This means it falls back to the host version, e.g. if you build on Mojave it would pass `-mmacosx-version-min=10.14` to the compiler.

.NET Core's minimum supported OSX version is 10.13 so pass that to CMake.
@karelz karelz added this to the 5.0 milestone Dec 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants