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 MacOS build on 11.x SDK and Catalyst build #54506

Merged
merged 6 commits into from
Jun 27, 2021

Conversation

janvorli
Copy link
Member

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

@janvorli janvorli added this to the 6.0.0 milestone Jun 21, 2021
@janvorli janvorli self-assigned this Jun 21, 2021
@janvorli
Copy link
Member Author

cc: @adamsitnik, @filipnavara, @sandreenko

Copy link
Member

@jkoritzinsky jkoritzinsky left a 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?

@janvorli
Copy link
Member Author

Sure, I actually wanted to do that and forgotten.

@janvorli
Copy link
Member Author

@jkoritzinsky I've added the comment.

@janvorli
Copy link
Member Author

There is some problem in the CI, I am looking into it.

@janvorli
Copy link
Member Author

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.

@mikem8361
Copy link
Member

@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.
@mikem8361
Copy link
Member

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.

@janvorli
Copy link
Member Author

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).

@janvorli janvorli force-pushed the fix-macos-build-on-11.x-sdk branch from e7fb666 to 17d251b Compare June 25, 2021 16:41
@janvorli janvorli force-pushed the fix-macos-build-on-11.x-sdk branch from 17d251b to 581ca38 Compare June 25, 2021 16:44
@janvorli
Copy link
Member Author

I've found the problem, the catalyst builds are now passing.

@janvorli
Copy link
Member Author

The remaining failure is really strange. coreclr tests are failing with this error:

dyld: lazy symbol binding failed: Symbol not found: ____chkstk_darwin
Referenced from: /tmp/helix/working/B95D0A45/p/corerun
Expected in: /usr/lib/libSystem.B.dylib

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.
So it is a mystery.

@janvorli
Copy link
Member Author

It seems everything is fixed now, only the Windows ARM64 checked leg keeps failing due to infra problems.

@janvorli
Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

@janvorli janvorli merged commit 385b6f3 into dotnet:main Jun 27, 2021
@janvorli janvorli deleted the fix-macos-build-on-11.x-sdk branch June 27, 2021 15:13
@ghost ghost locked as resolved and limited conversation to collaborators Jul 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants