-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 MacOS build on 11.x SDK and Catalyst build #54506
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some comments on why we need to set these flags differently so we remember in the future and don't regress this again?
Sure, I actually wanted to do that and forgotten. |
@jkoritzinsky I've added the comment. |
There is some problem in the CI, I am looking into it. |
Update: I cannot repro any of these issues on my local x64 and arm64 macOS devices. So it looks like some issue with older SDKs. I'll try to get access to one of the CI macs and see what's causing the failures. |
@janvorli I'm running into this build failure on my newer updated MacBook with Big Sur and XCode 12. |
The configure.cmake was not getting the minimum supported OS version because it was being set via set_compile_options and the config functions can only get options from CMAKE_XXX_FLAGS.
I run into this build error on my recently upgraded MacBook to Big Sur/11.4 and XCode 12.5. It is holding up some createdump changes I've been working for MacOS. |
I was unable to figure out what's wrong yet. I cannot get access to the lab machines and this change works fine on both of my local macs (x64 and arm64). |
e7fb666
to
17d251b
Compare
17d251b
to
581ca38
Compare
I've found the problem, the catalyst builds are now passing. |
The remaining failure is really strange. coreclr tests are failing with this error:
When I've googled for this error, some people were hitting it when compiling for older macOS version on Apple BigSur. Someone mentioned that it might be due to the missing specification of the minimum target version, but I have downloaded the artifacts from the CI and verified that the minimum supported version in the corerun is set correctly to 10.13. |
It seems everything is fixed now, only the Windows ARM64 checked leg keeps failing due to infra problems. |
Finally, I was able to rerun the legs without the infra issues and all is green. @jkoritzinsky do you want to take another look after the few changes I've made to fix the CI issues? |
@@ -77,7 +77,11 @@ fi | |||
|
|||
if [[ "$__TargetOS" == OSX ]]; then | |||
# set default OSX deployment target | |||
__CMakeArgs="-DCMAKE_OSX_DEPLOYMENT_TARGET=10.13 $__CMakeArgs" | |||
if [[ "$__BuildArch" == x64 ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not for this PR but just curious) Does it always eventually get overwritten (with same value) in src/libraries/Native/Unix/CMakeLists.txt
? If so, we can delete the if
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not. The src/libraries/Native/Unix/CMakeLists.txt doesn't set the variable nor the underlying option for macOS x64/arm64. I actually wonder why don't we use the settings from the eng/native/configurecompiler.cmake here. I have thought that we have unified all three subrepos to use those, but I cannot see it getting included. Do you remember when you were unifying this stuff if there was a reason to not to use the configurecompiler.cmake here? Or was it being used before and some change has removed it? Maybe the fact that mono uses this stuff too was the reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you are right. I was confusing MACOS_VERSION_MIN_FLAGS
with CMAKE_OSX_DEPLOYMENT_TARGET
.
Around the time when we were consolidating native configurations, mono builds were being added (Android/iOS etc.), so the progress stopped to avoid conflicts with other PRs. We had cleaned up most of the initial redundancies from scripts and cmake, but this part of libraires' cmake configurations was left alone because mono targets were frequently updating these files. Now that we have relatively a stable build matrix, that work can be continued to accomplish DRY.
The configure.cmake was not getting the minimum supported OS version because
it was being set via set_compile_options and the config functions can only
get options from CMAKE_XXX_FLAGS.
Also, the recently added preadv / pwritew require
_DARWIN_C_SOURCE
to be defined, otherwise they are not exposed via the header files in the recent macOS SDK.Close #54505
Close #54424