-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add iOS build configurations #33292
Add iOS build configurations #33292
Changes from 13 commits
3ab2446
3ed6789
4543209
9c051a9
06cd416
7f8d779
804c34d
4e10780
7b1cbaa
ab6b26b
ca62be6
810ffcf
c074d97
77a3c94
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,20 @@ if(CLR_CMAKE_HOST_OS STREQUAL Darwin) | |
set(CMAKE_ASM_COMPILE_OBJECT "${CMAKE_C_COMPILER} <FLAGS> <DEFINES> <INCLUDES> -o <OBJECT> -c <SOURCE>") | ||
endif(CLR_CMAKE_HOST_OS STREQUAL Darwin) | ||
|
||
if(CLR_CMAKE_HOST_OS STREQUAL iOS) | ||
set(CLR_CMAKE_HOST_UNIX 1) | ||
set(CLR_CMAKE_HOST_IOS 1) | ||
if(CMAKE_OSX_ARCHITECTURES MATCHES "x86_64") | ||
set(CLR_CMAKE_HOST_UNIX_AMD64 1) | ||
elseif(CMAKE_OSX_ARCHITECTURES MATCHES "armv7") | ||
set(CLR_CMAKE_HOST_UNIX_ARM 1) | ||
elseif(CMAKE_OSX_ARCHITECTURES MATCHES "arm64") | ||
set(CLR_CMAKE_HOST_UNIX_ARM64 1) | ||
else() | ||
clr_unknown_arch() | ||
endif() | ||
endif(CLR_CMAKE_HOST_OS STREQUAL iOS) | ||
|
||
if(CLR_CMAKE_HOST_OS STREQUAL FreeBSD) | ||
set(CLR_CMAKE_HOST_UNIX 1) | ||
set(CLR_CMAKE_HOST_UNIX_AMD64 1) | ||
|
@@ -230,6 +244,11 @@ if(CLR_CMAKE_TARGET_OS STREQUAL Darwin) | |
set(CLR_CMAKE_TARGET_DARWIN 1) | ||
endif(CLR_CMAKE_TARGET_OS STREQUAL Darwin) | ||
|
||
if(CLR_CMAKE_TARGET_OS STREQUAL iOS) | ||
set(CLR_CMAKE_TARGET_UNIX 1) | ||
set(CLR_CMAKE_TARGET_IOS 1) | ||
endif(CLR_CMAKE_TARGET_OS STREQUAL iOS) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd expect iOS to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes please, at least on cmake side, we are consistent with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a conscious decision because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do the renaming in a follow-up PR. |
||
|
||
if(CLR_CMAKE_TARGET_OS STREQUAL FreeBSD) | ||
set(CLR_CMAKE_TARGET_UNIX 1) | ||
set(CLR_CMAKE_TARGET_FREEBSD 1) | ||
|
@@ -302,3 +321,5 @@ if(NOT CLR_CMAKE_HOST_ARCH_WASM) | |
endif() | ||
set(CMAKE_POSITION_INDEPENDENT_CODE ON) | ||
endif(NOT CLR_CMAKE_HOST_ARCH_WASM) | ||
|
||
set(CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED 1) |
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.
Should we error if
SubsetCategory
is not empty and contains either installer or coreclr whenTargetOS
isiOS
?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.
I feel like if you manually override subset category that way you deserve to get the failures :)
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.
I get that, but it is good to help developers get readable failures? Sometimes because of the lack of those validations we get a lot of questions, why this doesn't work, why am I getting this error, etc? We already have a target that does validation for the subset categories, if I remember correctly.
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.
I think it's ok to set RuntimeFlavor based on the passed in
SubsetCategory
but I don't think thatSubsetCategory
should be inferred fromTargetOS
. TheSubsetCategory
can also contain values likemono
, ormono-coreclr-libraries-installer
therefore I think it's necessary to be intentional here and require the category to be passed in.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.
@ViktorHofer why not?
coreclr
orinstaller
are not valid subset categories when you have TargetOS=iOS so having a default when nothing is set explicitly makes a lot of sense.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.
I thought about this more. I'm fine with providing a default value for the subset categories when a configuration is passed in that isn't supported on one of those categories but I would not want to exclude installer which brings me back to my question: Why did you choose
mono-libraries
and notmono-libraries-installer
?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.
@ViktorHofer installer doesn't make sense at all, there won't be an installer for the iOS bits (nuget only). The default of
mono-libraries
is everything that we want to build by default.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.
I can move the change to set
DefaultSubsetCategories
forTargetOS==iOS
instead if that helps making it clearer.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.
I discussed this with Viktor offline and there was some misunderstanding.
installer
is not just for building the .deb/.msi/.pkg installers but also does other things like creating runtime packs and ref packs. So we might eventually need it for iOS too.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.
Regarding the subset validation: Viktor and me took a look and it seems a little more complicated to add than expected. We'll do it in a separate PR.