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.

Sign up for GitHub

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

Jump to bottom

Add Mac Catalyst (iOS API, Mac ABI) runtime #47823

Merged
merged 32 commits into from
Feb 8, 2021
Merged

Add Mac Catalyst (iOS API, Mac ABI) runtime #47823

merged 32 commits into from
Feb 8, 2021

Conversation

directhex
Copy link
Contributor

@directhex directhex commented Feb 3, 2021 •
edited by steveisok
Loading

This ties into several concurrent discussions:

#47645
#47517
#47518
#44882
dotnet/designs#174
#47768
dotnet/xamarin#29

I've made a few assumptions for now:

  • RIDs will be maccatalyst-x64 and maccatalyst-arm64
  • TFM is net6.0-maccatalyst
  • TargetsMacCatalyst gets defined in the build
  • IsMacCatalyst() and IsMacCatalystVersionAtLeast() get added to System.Runtime (an API change in need of review)
  • --os maccatalyst gets passed to build.sh
  • xharness support for maccatalyst Add Support for Mac Catalyst  xharness#435

It should work well enough right now to happily build a maccatalyst-x64 runtime pack. maccatalyst-arm64 requires an LLVM build, which we don't have as of today, plus a newer macOS/Xcode than my current dev machine. I know I've made some incorrect assumptions regarding ARM64 (e.g. the target iOS version for Catalyst needs to map to macOS 11.0+, but the number I'm using maps to 10.15.1)

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Feb 3, 2021

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

This ties into several concurrent discussions:

#47645
#47517
#47518
#44882
dotnet/designs#174
#47768
dotnet/xamarin#29

I've made a few assumptions for now:

  • RIDs will be maccatalyst-x64 and maccatalyst-arm64
  • TFM is net6.0-maccatalyst
  • TargetsMacCatalyst gets defined in the build
  • IsMacCatalyst() and IsMacCatalystVersionAtLeast() get added to System.Runtime (an API change in need of review)
  • --os maccatalyst gets passed to build.sh
  • xharness ios will work with Catalyst apps, through addition of a maccatalyst test target (TODO)

It should work well enough right now to happily build a maccatalyst-x64 runtime pack. maccatalyst-arm64 requires an LLVM build, which we don't have as of today, plus a newer macOS/Xcode than my current dev machine. I know I've made some incorrect assumptions regarding ARM64 (e.g. the target iOS version for Catalyst needs to map to macOS 11.0+, but the number I'm using maps to 10.15.1)

Author: directhex
Assignees: -
Labels:

area-Infrastructure-libraries, new-api-needs-documentation
Milestone: -

@directhex
Copy link
Contributor Author

I haven't added yml to build the new stuff yet

safern
safern reviewed Feb 3, 2021
View reviewed changes
eng/build.sh Show resolved Hide resolved
@directhex
Copy link
Contributor Author

Doesn't build with updated cmake. How exciting!

@directhex
Disable -Woverriding-t-option on Mac Catalyst builds
8daf812 
Somewhere between CMake 3.17 and 3.19.4, a decision was made to force
setting of -mmacosx-version-min when building with an OSX SDK. Leave
The relevant cmake deployment variable blank, and it just picks its
own default. You can't bypass it, as far as I can tell.

Mac Catalyst requires a -target flag, which is incompatible with
a -mmacosx-version-min flag. Sadly, our best option is to suppress
the warning/error, as the bug is in CMake not with us.
@directhex
Copy link
Contributor Author

Relevant CMake issue is https://gitlab.kitware.com/cmake/cmake/-/issues/20132. I have a workaround, pushing to branch

@directhex directhex marked this pull request as ready for review February 4, 2021 19:11
akoeplinger
akoeplinger requested changes Feb 5, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems Outdated Show resolved Hide resolved
src/libraries/Native/Unix/configure.cmake Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
eng/native/configurecompiler.cmake Show resolved Hide resolved
src/mono/mono/mini/graph.c Outdated Show resolved Hide resolved
src/mono/mono/mini/helpers.c Outdated Show resolved Hide resolved
src/tasks/AppleAppBuilder/Templates/runtime.m Show resolved Hide resolved
src/tasks/AppleAppBuilder/Xcode.cs Outdated Show resolved Hide resolved
src/tasks/AppleAppBuilder/Xcode.cs Outdated Show resolved Hide resolved
@directhex
Copy link
Contributor Author

ARM64 builds blocked on:

  /Users/directhex/Projects/runtime/src/mono/mono/mini/exceptions-arm64.c:552:2: warning: incompatible pointer to integer conversion assigning to '__uint64_t' (aka 'unsigned long long') from 'gpointer' (aka 'void *') [-Wint-conversion]
          UCONTEXT_REG_SET_PC (sigctx, addr);
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /Users/directhex/Projects/runtime/src/mono/mono/mini/../../mono/utils/mono-sigcontext.h:498:24: note: expanded from macro 'UCONTEXT_REG_SET_PC'
          UCONTEXT_REG_PC (ctx) = (val); \
                                ^ ~~~~~
  1 warning generated.

@directhex
Copy link
Contributor Author

CI is boned today, and it's not my fault

@directhex
Copy link
Contributor Author

ARM64 will be turned on when #47891 is fixed and merged

@runfoapp runfoapp bot mentioned this pull request Feb 5, 2021
Closed
@directhex
Copy link
Contributor Author

AppleAppBuilder needs serious work for Mac Catalyst ARM64, but should be good on x64

akoeplinger
akoeplinger requested changes Feb 5, 2021
View reviewed changes
src/mono/cmake/configure.cmake Outdated Show resolved Hide resolved
src/mono/mono.proj Outdated Show resolved Hide resolved
src/mono/mono.proj Outdated Show resolved Hide resolved
src/mono/mono.proj Outdated Show resolved Hide resolved
@runfoapp runfoapp bot mentioned this pull request Feb 8, 2021
Closed
@directhex
Copy link
Contributor Author

I've dispatched an official build, since my "oops it broke official builds" success rate isn't great

@directhex
Copy link
Contributor Author

passed on internal

@directhex directhex merged commit 44f8f0f into dotnet:master Feb 8, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern left review comments

@CoffeeFlux CoffeeFlux CoffeeFlux left review comments

@rolfbjarne rolfbjarne rolfbjarne left review comments

@akoeplinger akoeplinger akoeplinger approved these changes

@steveisok steveisok steveisok approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

@marek-safar marek-safar Awaiting requested review from marek-safar

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
area-Infrastructure-libraries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@directhex @rolfbjarne @marek-safar @steveisok @akoeplinger @CoffeeFlux @safern