Skip to content

Add iOS build configurations - #33292

Merged
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios
Mar 11, 2020
Merged

Add iOS build configurations#33292
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios

Conversation

@akoeplinger

Copy link
Copy Markdown
Member

This adds support for iOS using the Mono runtime to the build system.

I'm only adding the build integration right now, CI support will be done separately.

Comment threadeng/Subsets.props
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd expect iOS to set CLR_CMAKE_TARGET_DARWIN too. Not sure whether it would make the rest of the diff with if conditions smaller or larger though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes please, at least on cmake side, we are consistent with Darwin naming. It's only the code and RIDs where the (obsolete) osx name is used. 😁

@akoeplingerakoeplingerMar 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This was a conscious decision because CLR_CMAKE_TARGET_DARWIN really means macOS in the rest of the build scripts. We could rename it to CLR_CMAKE_TARGET_OSX?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll do the renaming in a follow-up PR.

# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the minimum target iOS? 9 stopped getting patches 3 years ago (modulo one patch for GPS rollover last year). Even 10 looks to be out of support other than the GPS fix.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The minimum target that Xamarin supports today is iOS 7 for devices and iOS 8 for the simulator so I replicated that here. We're discussing with PM but it looks like we won't be able to meaningfully bump the minimum version, definitely not to 10.

# Conflicts:
#	src/libraries/pkg/Directory.Build.props
#	src/mono/netcore/nuget/Directory.Build.props
It is only used for interacting with OpenSSL which isn't useful on iOS.
@marek-safar

Copy link
Copy Markdown
Contributor

@ViktorHofer any objections?

@ViktorHofer

Copy link
Copy Markdown
Member

I hadn't yet that time to review the PR. Please wait until #33242 is in (should be in ~1 hour) on which I was working the last few days to avoid conflicts. I will now review the PR.

# Conflicts:
#	eng/native/configuretools.cmake
#	src/installer/corehost/CMakeLists.txt
#	src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
@akoeplinger

Copy link
Copy Markdown
MemberAuthor

Sure. I'll need to push another merge anyway due to conflicts so I can wait for that PR to land.

@ViktorHofer
ViktorHofer requested a review from a teamMarch 10, 2020 14:21
@ViktorHofer

Copy link
Copy Markdown
Member

@ViktorHofer any objections?

a lot's going on today. I just requested review from dotnet/runtime-infrastructure as well. I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable. Please don't merge the change yet before me and the infra teams has taken a closer look. Thanks for your understanding.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable.

@ViktorHofer let me know if what I proposed in #33292 (comment) makes more sense, thanks.

@directhex

Copy link
Copy Markdown
Contributor

I'm branching off this to try and get the AzDO YAML going. Will rebase as and when.

@directhexdirecthex mentioned this pull request Mar 10, 2020
Comment on lines +1 to +3
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR "configuretools.cmake needs to be included after configureplatforms.cmake")
endif()

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we instead:

Suggested change
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR"configuretools.cmake needs to be included after configureplatforms.cmake")
endif()
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)

and remove three occurrences of include(${CLR_ENG_NATIVE_DIR}/configureplatform.cmake) under :/src/? This way we would only use configuretools.cmake as an entrypoint cmake for eng/native (and rest of the files will become internal detail, which however may get refactored in the future).

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or perhaps for all call sites, we can add an explicit :/eng/native/entrypoint.cmake enlisting:

include(${CMAKE_CURRENT_LIST_DIR}/functions.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configuretools.cmake)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes I think this makes sense. I'd prefer if we do it in a separate PR though :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have addressed it as part of Android fixes #32800.

Comment threadsrc/libraries/Directory.Build.props
Comment threadsrc/libraries/restore/runtime/runtime.depproj Outdated
@steveisok

Copy link
Copy Markdown
Member

@dotnet/runtime-infrastructure Can one or more of you give this a review? I'd like to have this merged today if possible.

Comment threadeng/Subsets.props Outdated

<_subsetCategory Condition="'$(SubsetCategory)' != ''">$(SubsetCategory.ToLowerInvariant())</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == ''">$(DefaultSubsetCategories)</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == '' and '$(TargetOS)' == 'iOS'">libraries-mono</_subsetCategory>

Copy link
Copy Markdown
Member

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 when TargetOS is iOS?

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
Member

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.

@ViktorHoferViktorHoferMar 11, 2020

Copy link
Copy Markdown
Member

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 that SubsetCategory should be inferred from TargetOS. The SubsetCategory can also contain values like mono, or mono-coreclr-libraries-installer therefore I think it's necessary to be intentional here and require the category to be passed in.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ViktorHofer why not? coreclr or installer are not valid subset categories when you have TargetOS=iOS so having a default when nothing is set explicitly makes a lot of sense.

Copy link
Copy Markdown
Member

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 not mono-libraries-installer?

@akoeplingerakoeplingerMar 11, 2020

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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 for TargetOS==iOS instead if that helps making it clearer.

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadeng/native/naming.props Outdated
Comment threadsrc/libraries/Native/build-native.sh Outdated
<watchOS64_32VersionMin>5.1</watchOS64_32VersionMin>
<macOSVersionMin>10.13</macOSVersionMin>

<!-- Version of the OS SDK we target -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious, why are these set to empty?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Some Apple tools embed certain pieces of metadata differently depending on whether you build against /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk or /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk (the former is a symlink to the latter).

On CI we want to build with the explicit version so we pass in the versions installed on the bots. Locally we don't really care about that and hardcoding the version in the file would just mean you need to change it whenever Xcode is updated, so we rely on the symlink there.

I know MSBuild doesn't require you to declare properties if they're empty but I felt it serves as a good documentation that they're used/available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining.

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than a few comments/questions, the infra changes LGTM.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

dotnet-runtime-perf failures are due to #33082.

@akoeplinger
akoeplinger merged commit a54d391 into dotnet:masterMar 11, 2020
@akoeplinger
akoeplinger deleted the add-ios branch March 11, 2020 23:55
@steveisoksteveisok mentioned this pull request Mar 11, 2020
24 tasks
@@ -0,0 +1,272 @@
/*
* Copyright (c) 2000-2008 Apple Inc. All rights reserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we discussed with @richlander and we'll take care of that.

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

discussed various additions to this PR offline with @akoeplinger, like the necessity of the installer subset at a later point for the runtime pack to build. The subset validation as well should be enabled later. LGTM. Thanks!

akoeplinger pushed a commit that referenced this pull request Mar 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto 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.

11 participants

@akoeplinger@marek-safar@ViktorHofer@directhex@steveisok@filipnavara@am11@jkotas@bartonjs@safern@Dotnet-GitSync-Bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Add iOS build configurations by akoeplinger · Pull Request #33292 · dotnet/runtime · GitHub
Skip to content

Add iOS build configurations - #33292

Merged
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios
Mar 11, 2020
Merged

Add iOS build configurations#33292
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios

Conversation

@akoeplinger

Copy link
Copy Markdown
Member

This adds support for iOS using the Mono runtime to the build system.

I'm only adding the build integration right now, CI support will be done separately.

Comment threadeng/Subsets.props
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd expect iOS to set CLR_CMAKE_TARGET_DARWIN too. Not sure whether it would make the rest of the diff with if conditions smaller or larger though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes please, at least on cmake side, we are consistent with Darwin naming. It's only the code and RIDs where the (obsolete) osx name is used. 😁

@akoeplingerakoeplingerMar 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This was a conscious decision because CLR_CMAKE_TARGET_DARWIN really means macOS in the rest of the build scripts. We could rename it to CLR_CMAKE_TARGET_OSX?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll do the renaming in a follow-up PR.

# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the minimum target iOS? 9 stopped getting patches 3 years ago (modulo one patch for GPS rollover last year). Even 10 looks to be out of support other than the GPS fix.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The minimum target that Xamarin supports today is iOS 7 for devices and iOS 8 for the simulator so I replicated that here. We're discussing with PM but it looks like we won't be able to meaningfully bump the minimum version, definitely not to 10.

# Conflicts:
#	src/libraries/pkg/Directory.Build.props
#	src/mono/netcore/nuget/Directory.Build.props
It is only used for interacting with OpenSSL which isn't useful on iOS.
@marek-safar

Copy link
Copy Markdown
Contributor

@ViktorHofer any objections?

@ViktorHofer

Copy link
Copy Markdown
Member

I hadn't yet that time to review the PR. Please wait until #33242 is in (should be in ~1 hour) on which I was working the last few days to avoid conflicts. I will now review the PR.

# Conflicts:
#	eng/native/configuretools.cmake
#	src/installer/corehost/CMakeLists.txt
#	src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
@akoeplinger

Copy link
Copy Markdown
MemberAuthor

Sure. I'll need to push another merge anyway due to conflicts so I can wait for that PR to land.

@ViktorHofer
ViktorHofer requested a review from a teamMarch 10, 2020 14:21
@ViktorHofer

Copy link
Copy Markdown
Member

@ViktorHofer any objections?

a lot's going on today. I just requested review from dotnet/runtime-infrastructure as well. I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable. Please don't merge the change yet before me and the infra teams has taken a closer look. Thanks for your understanding.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable.

@ViktorHofer let me know if what I proposed in #33292 (comment) makes more sense, thanks.

@directhex

Copy link
Copy Markdown
Contributor

I'm branching off this to try and get the AzDO YAML going. Will rebase as and when.

@directhexdirecthex mentioned this pull request Mar 10, 2020
Comment on lines +1 to +3
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR "configuretools.cmake needs to be included after configureplatforms.cmake")
endif()

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we instead:

Suggested change
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR"configuretools.cmake needs to be included after configureplatforms.cmake")
endif()
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)

and remove three occurrences of include(${CLR_ENG_NATIVE_DIR}/configureplatform.cmake) under :/src/? This way we would only use configuretools.cmake as an entrypoint cmake for eng/native (and rest of the files will become internal detail, which however may get refactored in the future).

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or perhaps for all call sites, we can add an explicit :/eng/native/entrypoint.cmake enlisting:

include(${CMAKE_CURRENT_LIST_DIR}/functions.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configuretools.cmake)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes I think this makes sense. I'd prefer if we do it in a separate PR though :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have addressed it as part of Android fixes #32800.

Comment threadsrc/libraries/Directory.Build.props
Comment threadsrc/libraries/restore/runtime/runtime.depproj Outdated
@steveisok

Copy link
Copy Markdown
Member

@dotnet/runtime-infrastructure Can one or more of you give this a review? I'd like to have this merged today if possible.

Comment threadeng/Subsets.props Outdated

<_subsetCategory Condition="'$(SubsetCategory)' != ''">$(SubsetCategory.ToLowerInvariant())</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == ''">$(DefaultSubsetCategories)</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == '' and '$(TargetOS)' == 'iOS'">libraries-mono</_subsetCategory>

Copy link
Copy Markdown
Member

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 when TargetOS is iOS?

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
Member

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.

@ViktorHoferViktorHoferMar 11, 2020

Copy link
Copy Markdown
Member

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 that SubsetCategory should be inferred from TargetOS. The SubsetCategory can also contain values like mono, or mono-coreclr-libraries-installer therefore I think it's necessary to be intentional here and require the category to be passed in.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ViktorHofer why not? coreclr or installer are not valid subset categories when you have TargetOS=iOS so having a default when nothing is set explicitly makes a lot of sense.

Copy link
Copy Markdown
Member

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 not mono-libraries-installer?

@akoeplingerakoeplingerMar 11, 2020

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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 for TargetOS==iOS instead if that helps making it clearer.

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadeng/native/naming.props Outdated
Comment threadsrc/libraries/Native/build-native.sh Outdated
<watchOS64_32VersionMin>5.1</watchOS64_32VersionMin>
<macOSVersionMin>10.13</macOSVersionMin>

<!-- Version of the OS SDK we target -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious, why are these set to empty?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Some Apple tools embed certain pieces of metadata differently depending on whether you build against /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk or /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk (the former is a symlink to the latter).

On CI we want to build with the explicit version so we pass in the versions installed on the bots. Locally we don't really care about that and hardcoding the version in the file would just mean you need to change it whenever Xcode is updated, so we rely on the symlink there.

I know MSBuild doesn't require you to declare properties if they're empty but I felt it serves as a good documentation that they're used/available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining.

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than a few comments/questions, the infra changes LGTM.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

dotnet-runtime-perf failures are due to #33082.

@akoeplinger
akoeplinger merged commit a54d391 into dotnet:masterMar 11, 2020
@akoeplinger
akoeplinger deleted the add-ios branch March 11, 2020 23:55
@steveisoksteveisok mentioned this pull request Mar 11, 2020
24 tasks
@@ -0,0 +1,272 @@
/*
* Copyright (c) 2000-2008 Apple Inc. All rights reserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we discussed with @richlander and we'll take care of that.

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

discussed various additions to this PR offline with @akoeplinger, like the necessity of the installer subset at a later point for the runtime pack to build. The subset validation as well should be enabled later. LGTM. Thanks!

akoeplinger pushed a commit that referenced this pull request Mar 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto 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.

11 participants

@akoeplinger@marek-safar@ViktorHofer@directhex@steveisok@filipnavara@am11@jkotas@bartonjs@safern@Dotnet-GitSync-Bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add iOS build configurations by akoeplinger · Pull Request #33292 · dotnet/runtime · GitHub
Skip to content

Add iOS build configurations - #33292

Merged
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios
Mar 11, 2020
Merged

Add iOS build configurations#33292
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios

Conversation

@akoeplinger

Copy link
Copy Markdown
Member

This adds support for iOS using the Mono runtime to the build system.

I'm only adding the build integration right now, CI support will be done separately.

Comment threadeng/Subsets.props
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd expect iOS to set CLR_CMAKE_TARGET_DARWIN too. Not sure whether it would make the rest of the diff with if conditions smaller or larger though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes please, at least on cmake side, we are consistent with Darwin naming. It's only the code and RIDs where the (obsolete) osx name is used. 😁

@akoeplingerakoeplingerMar 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This was a conscious decision because CLR_CMAKE_TARGET_DARWIN really means macOS in the rest of the build scripts. We could rename it to CLR_CMAKE_TARGET_OSX?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll do the renaming in a follow-up PR.

# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the minimum target iOS? 9 stopped getting patches 3 years ago (modulo one patch for GPS rollover last year). Even 10 looks to be out of support other than the GPS fix.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The minimum target that Xamarin supports today is iOS 7 for devices and iOS 8 for the simulator so I replicated that here. We're discussing with PM but it looks like we won't be able to meaningfully bump the minimum version, definitely not to 10.

# Conflicts:
#	src/libraries/pkg/Directory.Build.props
#	src/mono/netcore/nuget/Directory.Build.props
It is only used for interacting with OpenSSL which isn't useful on iOS.
@marek-safar

Copy link
Copy Markdown
Contributor

@ViktorHofer any objections?

@ViktorHofer

Copy link
Copy Markdown
Member

I hadn't yet that time to review the PR. Please wait until #33242 is in (should be in ~1 hour) on which I was working the last few days to avoid conflicts. I will now review the PR.

# Conflicts:
#	eng/native/configuretools.cmake
#	src/installer/corehost/CMakeLists.txt
#	src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
@akoeplinger

Copy link
Copy Markdown
MemberAuthor

Sure. I'll need to push another merge anyway due to conflicts so I can wait for that PR to land.

@ViktorHofer
ViktorHofer requested a review from a teamMarch 10, 2020 14:21
@ViktorHofer

Copy link
Copy Markdown
Member

@ViktorHofer any objections?

a lot's going on today. I just requested review from dotnet/runtime-infrastructure as well. I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable. Please don't merge the change yet before me and the infra teams has taken a closer look. Thanks for your understanding.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable.

@ViktorHofer let me know if what I proposed in #33292 (comment) makes more sense, thanks.

@directhex

Copy link
Copy Markdown
Contributor

I'm branching off this to try and get the AzDO YAML going. Will rebase as and when.

@directhexdirecthex mentioned this pull request Mar 10, 2020
Comment on lines +1 to +3
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR "configuretools.cmake needs to be included after configureplatforms.cmake")
endif()

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we instead:

Suggested change
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR"configuretools.cmake needs to be included after configureplatforms.cmake")
endif()
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)

and remove three occurrences of include(${CLR_ENG_NATIVE_DIR}/configureplatform.cmake) under :/src/? This way we would only use configuretools.cmake as an entrypoint cmake for eng/native (and rest of the files will become internal detail, which however may get refactored in the future).

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or perhaps for all call sites, we can add an explicit :/eng/native/entrypoint.cmake enlisting:

include(${CMAKE_CURRENT_LIST_DIR}/functions.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configuretools.cmake)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes I think this makes sense. I'd prefer if we do it in a separate PR though :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have addressed it as part of Android fixes #32800.

Comment threadsrc/libraries/Directory.Build.props
Comment threadsrc/libraries/restore/runtime/runtime.depproj Outdated
@steveisok

Copy link
Copy Markdown
Member

@dotnet/runtime-infrastructure Can one or more of you give this a review? I'd like to have this merged today if possible.

Comment threadeng/Subsets.props Outdated

<_subsetCategory Condition="'$(SubsetCategory)' != ''">$(SubsetCategory.ToLowerInvariant())</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == ''">$(DefaultSubsetCategories)</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == '' and '$(TargetOS)' == 'iOS'">libraries-mono</_subsetCategory>

Copy link
Copy Markdown
Member

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 when TargetOS is iOS?

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
Member

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.

@ViktorHoferViktorHoferMar 11, 2020

Copy link
Copy Markdown
Member

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 that SubsetCategory should be inferred from TargetOS. The SubsetCategory can also contain values like mono, or mono-coreclr-libraries-installer therefore I think it's necessary to be intentional here and require the category to be passed in.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ViktorHofer why not? coreclr or installer are not valid subset categories when you have TargetOS=iOS so having a default when nothing is set explicitly makes a lot of sense.

Copy link
Copy Markdown
Member

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 not mono-libraries-installer?

@akoeplingerakoeplingerMar 11, 2020

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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 for TargetOS==iOS instead if that helps making it clearer.

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadeng/native/naming.props Outdated
Comment threadsrc/libraries/Native/build-native.sh Outdated
<watchOS64_32VersionMin>5.1</watchOS64_32VersionMin>
<macOSVersionMin>10.13</macOSVersionMin>

<!-- Version of the OS SDK we target -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious, why are these set to empty?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Some Apple tools embed certain pieces of metadata differently depending on whether you build against /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk or /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk (the former is a symlink to the latter).

On CI we want to build with the explicit version so we pass in the versions installed on the bots. Locally we don't really care about that and hardcoding the version in the file would just mean you need to change it whenever Xcode is updated, so we rely on the symlink there.

I know MSBuild doesn't require you to declare properties if they're empty but I felt it serves as a good documentation that they're used/available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining.

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than a few comments/questions, the infra changes LGTM.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

dotnet-runtime-perf failures are due to #33082.

@akoeplinger
akoeplinger merged commit a54d391 into dotnet:masterMar 11, 2020
@akoeplinger
akoeplinger deleted the add-ios branch March 11, 2020 23:55
@steveisoksteveisok mentioned this pull request Mar 11, 2020
24 tasks
@@ -0,0 +1,272 @@
/*
* Copyright (c) 2000-2008 Apple Inc. All rights reserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we discussed with @richlander and we'll take care of that.

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

discussed various additions to this PR offline with @akoeplinger, like the necessity of the installer subset at a later point for the runtime pack to build. The subset validation as well should be enabled later. LGTM. Thanks!

akoeplinger pushed a commit that referenced this pull request Mar 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto 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.

11 participants

@akoeplinger@marek-safar@ViktorHofer@directhex@steveisok@filipnavara@am11@jkotas@bartonjs@safern@Dotnet-GitSync-Bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add iOS build configurations by akoeplinger · Pull Request #33292 · dotnet/runtime · GitHub
Skip to content

Add iOS build configurations - #33292

Merged
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios
Mar 11, 2020
Merged

Add iOS build configurations#33292
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios

Conversation

@akoeplinger

Copy link
Copy Markdown
Member

This adds support for iOS using the Mono runtime to the build system.

I'm only adding the build integration right now, CI support will be done separately.

Comment threadeng/Subsets.props
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd expect iOS to set CLR_CMAKE_TARGET_DARWIN too. Not sure whether it would make the rest of the diff with if conditions smaller or larger though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes please, at least on cmake side, we are consistent with Darwin naming. It's only the code and RIDs where the (obsolete) osx name is used. 😁

@akoeplingerakoeplingerMar 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This was a conscious decision because CLR_CMAKE_TARGET_DARWIN really means macOS in the rest of the build scripts. We could rename it to CLR_CMAKE_TARGET_OSX?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll do the renaming in a follow-up PR.

# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the minimum target iOS? 9 stopped getting patches 3 years ago (modulo one patch for GPS rollover last year). Even 10 looks to be out of support other than the GPS fix.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The minimum target that Xamarin supports today is iOS 7 for devices and iOS 8 for the simulator so I replicated that here. We're discussing with PM but it looks like we won't be able to meaningfully bump the minimum version, definitely not to 10.

# Conflicts:
#	src/libraries/pkg/Directory.Build.props
#	src/mono/netcore/nuget/Directory.Build.props
It is only used for interacting with OpenSSL which isn't useful on iOS.
@marek-safar

Copy link
Copy Markdown
Contributor

@ViktorHofer any objections?

@ViktorHofer

Copy link
Copy Markdown
Member

I hadn't yet that time to review the PR. Please wait until #33242 is in (should be in ~1 hour) on which I was working the last few days to avoid conflicts. I will now review the PR.

# Conflicts:
#	eng/native/configuretools.cmake
#	src/installer/corehost/CMakeLists.txt
#	src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
@akoeplinger

Copy link
Copy Markdown
MemberAuthor

Sure. I'll need to push another merge anyway due to conflicts so I can wait for that PR to land.

@ViktorHofer
ViktorHofer requested a review from a teamMarch 10, 2020 14:21
@ViktorHofer

Copy link
Copy Markdown
Member

@ViktorHofer any objections?

a lot's going on today. I just requested review from dotnet/runtime-infrastructure as well. I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable. Please don't merge the change yet before me and the infra teams has taken a closer look. Thanks for your understanding.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable.

@ViktorHofer let me know if what I proposed in #33292 (comment) makes more sense, thanks.

@directhex

Copy link
Copy Markdown
Contributor

I'm branching off this to try and get the AzDO YAML going. Will rebase as and when.

@directhexdirecthex mentioned this pull request Mar 10, 2020
Comment on lines +1 to +3
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR "configuretools.cmake needs to be included after configureplatforms.cmake")
endif()

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we instead:

Suggested change
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR"configuretools.cmake needs to be included after configureplatforms.cmake")
endif()
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)

and remove three occurrences of include(${CLR_ENG_NATIVE_DIR}/configureplatform.cmake) under :/src/? This way we would only use configuretools.cmake as an entrypoint cmake for eng/native (and rest of the files will become internal detail, which however may get refactored in the future).

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or perhaps for all call sites, we can add an explicit :/eng/native/entrypoint.cmake enlisting:

include(${CMAKE_CURRENT_LIST_DIR}/functions.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configuretools.cmake)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes I think this makes sense. I'd prefer if we do it in a separate PR though :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have addressed it as part of Android fixes #32800.

Comment threadsrc/libraries/Directory.Build.props
Comment threadsrc/libraries/restore/runtime/runtime.depproj Outdated
@steveisok

Copy link
Copy Markdown
Member

@dotnet/runtime-infrastructure Can one or more of you give this a review? I'd like to have this merged today if possible.

Comment threadeng/Subsets.props Outdated

<_subsetCategory Condition="'$(SubsetCategory)' != ''">$(SubsetCategory.ToLowerInvariant())</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == ''">$(DefaultSubsetCategories)</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == '' and '$(TargetOS)' == 'iOS'">libraries-mono</_subsetCategory>

Copy link
Copy Markdown
Member

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 when TargetOS is iOS?

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
Member

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.

@ViktorHoferViktorHoferMar 11, 2020

Copy link
Copy Markdown
Member

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 that SubsetCategory should be inferred from TargetOS. The SubsetCategory can also contain values like mono, or mono-coreclr-libraries-installer therefore I think it's necessary to be intentional here and require the category to be passed in.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ViktorHofer why not? coreclr or installer are not valid subset categories when you have TargetOS=iOS so having a default when nothing is set explicitly makes a lot of sense.

Copy link
Copy Markdown
Member

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 not mono-libraries-installer?

@akoeplingerakoeplingerMar 11, 2020

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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 for TargetOS==iOS instead if that helps making it clearer.

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadeng/native/naming.props Outdated
Comment threadsrc/libraries/Native/build-native.sh Outdated
<watchOS64_32VersionMin>5.1</watchOS64_32VersionMin>
<macOSVersionMin>10.13</macOSVersionMin>

<!-- Version of the OS SDK we target -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious, why are these set to empty?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Some Apple tools embed certain pieces of metadata differently depending on whether you build against /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk or /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk (the former is a symlink to the latter).

On CI we want to build with the explicit version so we pass in the versions installed on the bots. Locally we don't really care about that and hardcoding the version in the file would just mean you need to change it whenever Xcode is updated, so we rely on the symlink there.

I know MSBuild doesn't require you to declare properties if they're empty but I felt it serves as a good documentation that they're used/available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining.

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than a few comments/questions, the infra changes LGTM.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

dotnet-runtime-perf failures are due to #33082.

@akoeplinger
akoeplinger merged commit a54d391 into dotnet:masterMar 11, 2020
@akoeplinger
akoeplinger deleted the add-ios branch March 11, 2020 23:55
@steveisoksteveisok mentioned this pull request Mar 11, 2020
24 tasks
@@ -0,0 +1,272 @@
/*
* Copyright (c) 2000-2008 Apple Inc. All rights reserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we discussed with @richlander and we'll take care of that.

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

discussed various additions to this PR offline with @akoeplinger, like the necessity of the installer subset at a later point for the runtime pack to build. The subset validation as well should be enabled later. LGTM. Thanks!

akoeplinger pushed a commit that referenced this pull request Mar 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto 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.

11 participants

@akoeplinger@marek-safar@ViktorHofer@directhex@steveisok@filipnavara@am11@jkotas@bartonjs@safern@Dotnet-GitSync-Bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Add iOS build configurations by akoeplinger · Pull Request #33292 · dotnet/runtime · GitHub
Skip to content

Add iOS build configurations - #33292

Merged
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios
Mar 11, 2020
Merged

Add iOS build configurations#33292
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios

Conversation

@akoeplinger

Copy link
Copy Markdown
Member

This adds support for iOS using the Mono runtime to the build system.

I'm only adding the build integration right now, CI support will be done separately.

Comment threadeng/Subsets.props
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd expect iOS to set CLR_CMAKE_TARGET_DARWIN too. Not sure whether it would make the rest of the diff with if conditions smaller or larger though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes please, at least on cmake side, we are consistent with Darwin naming. It's only the code and RIDs where the (obsolete) osx name is used. 😁

@akoeplingerakoeplingerMar 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This was a conscious decision because CLR_CMAKE_TARGET_DARWIN really means macOS in the rest of the build scripts. We could rename it to CLR_CMAKE_TARGET_OSX?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll do the renaming in a follow-up PR.

# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the minimum target iOS? 9 stopped getting patches 3 years ago (modulo one patch for GPS rollover last year). Even 10 looks to be out of support other than the GPS fix.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The minimum target that Xamarin supports today is iOS 7 for devices and iOS 8 for the simulator so I replicated that here. We're discussing with PM but it looks like we won't be able to meaningfully bump the minimum version, definitely not to 10.

# Conflicts:
#	src/libraries/pkg/Directory.Build.props
#	src/mono/netcore/nuget/Directory.Build.props
It is only used for interacting with OpenSSL which isn't useful on iOS.
@marek-safar

Copy link
Copy Markdown
Contributor

@ViktorHofer any objections?

@ViktorHofer

Copy link
Copy Markdown
Member

I hadn't yet that time to review the PR. Please wait until #33242 is in (should be in ~1 hour) on which I was working the last few days to avoid conflicts. I will now review the PR.

# Conflicts:
#	eng/native/configuretools.cmake
#	src/installer/corehost/CMakeLists.txt
#	src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
@akoeplinger

Copy link
Copy Markdown
MemberAuthor

Sure. I'll need to push another merge anyway due to conflicts so I can wait for that PR to land.

@ViktorHofer
ViktorHofer requested a review from a teamMarch 10, 2020 14:21
@ViktorHofer

Copy link
Copy Markdown
Member

@ViktorHofer any objections?

a lot's going on today. I just requested review from dotnet/runtime-infrastructure as well. I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable. Please don't merge the change yet before me and the infra teams has taken a closer look. Thanks for your understanding.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable.

@ViktorHofer let me know if what I proposed in #33292 (comment) makes more sense, thanks.

@directhex

Copy link
Copy Markdown
Contributor

I'm branching off this to try and get the AzDO YAML going. Will rebase as and when.

@directhexdirecthex mentioned this pull request Mar 10, 2020
Comment on lines +1 to +3
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR "configuretools.cmake needs to be included after configureplatforms.cmake")
endif()

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we instead:

Suggested change
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR"configuretools.cmake needs to be included after configureplatforms.cmake")
endif()
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)

and remove three occurrences of include(${CLR_ENG_NATIVE_DIR}/configureplatform.cmake) under :/src/? This way we would only use configuretools.cmake as an entrypoint cmake for eng/native (and rest of the files will become internal detail, which however may get refactored in the future).

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or perhaps for all call sites, we can add an explicit :/eng/native/entrypoint.cmake enlisting:

include(${CMAKE_CURRENT_LIST_DIR}/functions.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configuretools.cmake)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes I think this makes sense. I'd prefer if we do it in a separate PR though :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have addressed it as part of Android fixes #32800.

Comment threadsrc/libraries/Directory.Build.props
Comment threadsrc/libraries/restore/runtime/runtime.depproj Outdated
@steveisok

Copy link
Copy Markdown
Member

@dotnet/runtime-infrastructure Can one or more of you give this a review? I'd like to have this merged today if possible.

Comment threadeng/Subsets.props Outdated

<_subsetCategory Condition="'$(SubsetCategory)' != ''">$(SubsetCategory.ToLowerInvariant())</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == ''">$(DefaultSubsetCategories)</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == '' and '$(TargetOS)' == 'iOS'">libraries-mono</_subsetCategory>

Copy link
Copy Markdown
Member

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 when TargetOS is iOS?

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
Member

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.

@ViktorHoferViktorHoferMar 11, 2020

Copy link
Copy Markdown
Member

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 that SubsetCategory should be inferred from TargetOS. The SubsetCategory can also contain values like mono, or mono-coreclr-libraries-installer therefore I think it's necessary to be intentional here and require the category to be passed in.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ViktorHofer why not? coreclr or installer are not valid subset categories when you have TargetOS=iOS so having a default when nothing is set explicitly makes a lot of sense.

Copy link
Copy Markdown
Member

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 not mono-libraries-installer?

@akoeplingerakoeplingerMar 11, 2020

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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 for TargetOS==iOS instead if that helps making it clearer.

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadeng/native/naming.props Outdated
Comment threadsrc/libraries/Native/build-native.sh Outdated
<watchOS64_32VersionMin>5.1</watchOS64_32VersionMin>
<macOSVersionMin>10.13</macOSVersionMin>

<!-- Version of the OS SDK we target -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious, why are these set to empty?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Some Apple tools embed certain pieces of metadata differently depending on whether you build against /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk or /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk (the former is a symlink to the latter).

On CI we want to build with the explicit version so we pass in the versions installed on the bots. Locally we don't really care about that and hardcoding the version in the file would just mean you need to change it whenever Xcode is updated, so we rely on the symlink there.

I know MSBuild doesn't require you to declare properties if they're empty but I felt it serves as a good documentation that they're used/available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining.

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than a few comments/questions, the infra changes LGTM.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

dotnet-runtime-perf failures are due to #33082.

@akoeplinger
akoeplinger merged commit a54d391 into dotnet:masterMar 11, 2020
@akoeplinger
akoeplinger deleted the add-ios branch March 11, 2020 23:55
@steveisoksteveisok mentioned this pull request Mar 11, 2020
24 tasks
@@ -0,0 +1,272 @@
/*
* Copyright (c) 2000-2008 Apple Inc. All rights reserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we discussed with @richlander and we'll take care of that.

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

discussed various additions to this PR offline with @akoeplinger, like the necessity of the installer subset at a later point for the runtime pack to build. The subset validation as well should be enabled later. LGTM. Thanks!

akoeplinger pushed a commit that referenced this pull request Mar 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto 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.

11 participants

@akoeplinger@marek-safar@ViktorHofer@directhex@steveisok@filipnavara@am11@jkotas@bartonjs@safern@Dotnet-GitSync-Bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add iOS build configurations by akoeplinger · Pull Request #33292 · dotnet/runtime · GitHub
Skip to content

Add iOS build configurations - #33292

Merged
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios
Mar 11, 2020
Merged

Add iOS build configurations#33292
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios

Conversation

@akoeplinger

Copy link
Copy Markdown
Member

This adds support for iOS using the Mono runtime to the build system.

I'm only adding the build integration right now, CI support will be done separately.

Comment threadeng/Subsets.props
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd expect iOS to set CLR_CMAKE_TARGET_DARWIN too. Not sure whether it would make the rest of the diff with if conditions smaller or larger though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes please, at least on cmake side, we are consistent with Darwin naming. It's only the code and RIDs where the (obsolete) osx name is used. 😁

@akoeplingerakoeplingerMar 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This was a conscious decision because CLR_CMAKE_TARGET_DARWIN really means macOS in the rest of the build scripts. We could rename it to CLR_CMAKE_TARGET_OSX?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll do the renaming in a follow-up PR.

# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the minimum target iOS? 9 stopped getting patches 3 years ago (modulo one patch for GPS rollover last year). Even 10 looks to be out of support other than the GPS fix.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The minimum target that Xamarin supports today is iOS 7 for devices and iOS 8 for the simulator so I replicated that here. We're discussing with PM but it looks like we won't be able to meaningfully bump the minimum version, definitely not to 10.

# Conflicts:
#	src/libraries/pkg/Directory.Build.props
#	src/mono/netcore/nuget/Directory.Build.props
It is only used for interacting with OpenSSL which isn't useful on iOS.
@marek-safar

Copy link
Copy Markdown
Contributor

@ViktorHofer any objections?

@ViktorHofer

Copy link
Copy Markdown
Member

I hadn't yet that time to review the PR. Please wait until #33242 is in (should be in ~1 hour) on which I was working the last few days to avoid conflicts. I will now review the PR.

# Conflicts:
#	eng/native/configuretools.cmake
#	src/installer/corehost/CMakeLists.txt
#	src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
@akoeplinger

Copy link
Copy Markdown
MemberAuthor

Sure. I'll need to push another merge anyway due to conflicts so I can wait for that PR to land.

@ViktorHofer
ViktorHofer requested a review from a teamMarch 10, 2020 14:21
@ViktorHofer

Copy link
Copy Markdown
Member

@ViktorHofer any objections?

a lot's going on today. I just requested review from dotnet/runtime-infrastructure as well. I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable. Please don't merge the change yet before me and the infra teams has taken a closer look. Thanks for your understanding.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable.

@ViktorHofer let me know if what I proposed in #33292 (comment) makes more sense, thanks.

@directhex

Copy link
Copy Markdown
Contributor

I'm branching off this to try and get the AzDO YAML going. Will rebase as and when.

@directhexdirecthex mentioned this pull request Mar 10, 2020
Comment on lines +1 to +3
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR "configuretools.cmake needs to be included after configureplatforms.cmake")
endif()

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we instead:

Suggested change
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR"configuretools.cmake needs to be included after configureplatforms.cmake")
endif()
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)

and remove three occurrences of include(${CLR_ENG_NATIVE_DIR}/configureplatform.cmake) under :/src/? This way we would only use configuretools.cmake as an entrypoint cmake for eng/native (and rest of the files will become internal detail, which however may get refactored in the future).

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or perhaps for all call sites, we can add an explicit :/eng/native/entrypoint.cmake enlisting:

include(${CMAKE_CURRENT_LIST_DIR}/functions.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configuretools.cmake)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes I think this makes sense. I'd prefer if we do it in a separate PR though :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have addressed it as part of Android fixes #32800.

Comment threadsrc/libraries/Directory.Build.props
Comment threadsrc/libraries/restore/runtime/runtime.depproj Outdated
@steveisok

Copy link
Copy Markdown
Member

@dotnet/runtime-infrastructure Can one or more of you give this a review? I'd like to have this merged today if possible.

Comment threadeng/Subsets.props Outdated

<_subsetCategory Condition="'$(SubsetCategory)' != ''">$(SubsetCategory.ToLowerInvariant())</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == ''">$(DefaultSubsetCategories)</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == '' and '$(TargetOS)' == 'iOS'">libraries-mono</_subsetCategory>

Copy link
Copy Markdown
Member

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 when TargetOS is iOS?

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
Member

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.

@ViktorHoferViktorHoferMar 11, 2020

Copy link
Copy Markdown
Member

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 that SubsetCategory should be inferred from TargetOS. The SubsetCategory can also contain values like mono, or mono-coreclr-libraries-installer therefore I think it's necessary to be intentional here and require the category to be passed in.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ViktorHofer why not? coreclr or installer are not valid subset categories when you have TargetOS=iOS so having a default when nothing is set explicitly makes a lot of sense.

Copy link
Copy Markdown
Member

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 not mono-libraries-installer?

@akoeplingerakoeplingerMar 11, 2020

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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 for TargetOS==iOS instead if that helps making it clearer.

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadeng/native/naming.props Outdated
Comment threadsrc/libraries/Native/build-native.sh Outdated
<watchOS64_32VersionMin>5.1</watchOS64_32VersionMin>
<macOSVersionMin>10.13</macOSVersionMin>

<!-- Version of the OS SDK we target -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious, why are these set to empty?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Some Apple tools embed certain pieces of metadata differently depending on whether you build against /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk or /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk (the former is a symlink to the latter).

On CI we want to build with the explicit version so we pass in the versions installed on the bots. Locally we don't really care about that and hardcoding the version in the file would just mean you need to change it whenever Xcode is updated, so we rely on the symlink there.

I know MSBuild doesn't require you to declare properties if they're empty but I felt it serves as a good documentation that they're used/available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining.

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than a few comments/questions, the infra changes LGTM.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

dotnet-runtime-perf failures are due to #33082.

@akoeplinger
akoeplinger merged commit a54d391 into dotnet:masterMar 11, 2020
@akoeplinger
akoeplinger deleted the add-ios branch March 11, 2020 23:55
@steveisoksteveisok mentioned this pull request Mar 11, 2020
24 tasks
@@ -0,0 +1,272 @@
/*
* Copyright (c) 2000-2008 Apple Inc. All rights reserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we discussed with @richlander and we'll take care of that.

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

discussed various additions to this PR offline with @akoeplinger, like the necessity of the installer subset at a later point for the runtime pack to build. The subset validation as well should be enabled later. LGTM. Thanks!

akoeplinger pushed a commit that referenced this pull request Mar 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto 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.

11 participants

@akoeplinger@marek-safar@ViktorHofer@directhex@steveisok@filipnavara@am11@jkotas@bartonjs@safern@Dotnet-GitSync-Bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add iOS build configurations by akoeplinger · Pull Request #33292 · dotnet/runtime · GitHub
Skip to content

Add iOS build configurations - #33292

Merged
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios
Mar 11, 2020
Merged

Add iOS build configurations#33292
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios

Conversation

@akoeplinger

Copy link
Copy Markdown
Member

This adds support for iOS using the Mono runtime to the build system.

I'm only adding the build integration right now, CI support will be done separately.

Comment threadeng/Subsets.props
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd expect iOS to set CLR_CMAKE_TARGET_DARWIN too. Not sure whether it would make the rest of the diff with if conditions smaller or larger though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes please, at least on cmake side, we are consistent with Darwin naming. It's only the code and RIDs where the (obsolete) osx name is used. 😁

@akoeplingerakoeplingerMar 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This was a conscious decision because CLR_CMAKE_TARGET_DARWIN really means macOS in the rest of the build scripts. We could rename it to CLR_CMAKE_TARGET_OSX?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll do the renaming in a follow-up PR.

# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the minimum target iOS? 9 stopped getting patches 3 years ago (modulo one patch for GPS rollover last year). Even 10 looks to be out of support other than the GPS fix.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The minimum target that Xamarin supports today is iOS 7 for devices and iOS 8 for the simulator so I replicated that here. We're discussing with PM but it looks like we won't be able to meaningfully bump the minimum version, definitely not to 10.

# Conflicts:
#	src/libraries/pkg/Directory.Build.props
#	src/mono/netcore/nuget/Directory.Build.props
It is only used for interacting with OpenSSL which isn't useful on iOS.
@marek-safar

Copy link
Copy Markdown
Contributor

@ViktorHofer any objections?

@ViktorHofer

Copy link
Copy Markdown
Member

I hadn't yet that time to review the PR. Please wait until #33242 is in (should be in ~1 hour) on which I was working the last few days to avoid conflicts. I will now review the PR.

# Conflicts:
#	eng/native/configuretools.cmake
#	src/installer/corehost/CMakeLists.txt
#	src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
@akoeplinger

Copy link
Copy Markdown
MemberAuthor

Sure. I'll need to push another merge anyway due to conflicts so I can wait for that PR to land.

@ViktorHofer
ViktorHofer requested a review from a teamMarch 10, 2020 14:21
@ViktorHofer

Copy link
Copy Markdown
Member

@ViktorHofer any objections?

a lot's going on today. I just requested review from dotnet/runtime-infrastructure as well. I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable. Please don't merge the change yet before me and the infra teams has taken a closer look. Thanks for your understanding.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable.

@ViktorHofer let me know if what I proposed in #33292 (comment) makes more sense, thanks.

@directhex

Copy link
Copy Markdown
Contributor

I'm branching off this to try and get the AzDO YAML going. Will rebase as and when.

@directhexdirecthex mentioned this pull request Mar 10, 2020
Comment on lines +1 to +3
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR "configuretools.cmake needs to be included after configureplatforms.cmake")
endif()

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we instead:

Suggested change
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR"configuretools.cmake needs to be included after configureplatforms.cmake")
endif()
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)

and remove three occurrences of include(${CLR_ENG_NATIVE_DIR}/configureplatform.cmake) under :/src/? This way we would only use configuretools.cmake as an entrypoint cmake for eng/native (and rest of the files will become internal detail, which however may get refactored in the future).

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or perhaps for all call sites, we can add an explicit :/eng/native/entrypoint.cmake enlisting:

include(${CMAKE_CURRENT_LIST_DIR}/functions.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configuretools.cmake)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes I think this makes sense. I'd prefer if we do it in a separate PR though :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have addressed it as part of Android fixes #32800.

Comment threadsrc/libraries/Directory.Build.props
Comment threadsrc/libraries/restore/runtime/runtime.depproj Outdated
@steveisok

Copy link
Copy Markdown
Member

@dotnet/runtime-infrastructure Can one or more of you give this a review? I'd like to have this merged today if possible.

Comment threadeng/Subsets.props Outdated

<_subsetCategory Condition="'$(SubsetCategory)' != ''">$(SubsetCategory.ToLowerInvariant())</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == ''">$(DefaultSubsetCategories)</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == '' and '$(TargetOS)' == 'iOS'">libraries-mono</_subsetCategory>

Copy link
Copy Markdown
Member

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 when TargetOS is iOS?

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
Member

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.

@ViktorHoferViktorHoferMar 11, 2020

Copy link
Copy Markdown
Member

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 that SubsetCategory should be inferred from TargetOS. The SubsetCategory can also contain values like mono, or mono-coreclr-libraries-installer therefore I think it's necessary to be intentional here and require the category to be passed in.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ViktorHofer why not? coreclr or installer are not valid subset categories when you have TargetOS=iOS so having a default when nothing is set explicitly makes a lot of sense.

Copy link
Copy Markdown
Member

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 not mono-libraries-installer?

@akoeplingerakoeplingerMar 11, 2020

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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 for TargetOS==iOS instead if that helps making it clearer.

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadeng/native/naming.props Outdated
Comment threadsrc/libraries/Native/build-native.sh Outdated
<watchOS64_32VersionMin>5.1</watchOS64_32VersionMin>
<macOSVersionMin>10.13</macOSVersionMin>

<!-- Version of the OS SDK we target -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious, why are these set to empty?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Some Apple tools embed certain pieces of metadata differently depending on whether you build against /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk or /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk (the former is a symlink to the latter).

On CI we want to build with the explicit version so we pass in the versions installed on the bots. Locally we don't really care about that and hardcoding the version in the file would just mean you need to change it whenever Xcode is updated, so we rely on the symlink there.

I know MSBuild doesn't require you to declare properties if they're empty but I felt it serves as a good documentation that they're used/available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining.

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than a few comments/questions, the infra changes LGTM.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

dotnet-runtime-perf failures are due to #33082.

@akoeplinger
akoeplinger merged commit a54d391 into dotnet:masterMar 11, 2020
@akoeplinger
akoeplinger deleted the add-ios branch March 11, 2020 23:55
@steveisoksteveisok mentioned this pull request Mar 11, 2020
24 tasks
@@ -0,0 +1,272 @@
/*
* Copyright (c) 2000-2008 Apple Inc. All rights reserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we discussed with @richlander and we'll take care of that.

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

discussed various additions to this PR offline with @akoeplinger, like the necessity of the installer subset at a later point for the runtime pack to build. The subset validation as well should be enabled later. LGTM. Thanks!

akoeplinger pushed a commit that referenced this pull request Mar 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto 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.

11 participants

@akoeplinger@marek-safar@ViktorHofer@directhex@steveisok@filipnavara@am11@jkotas@bartonjs@safern@Dotnet-GitSync-Bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Add iOS build configurations by akoeplinger · Pull Request #33292 · dotnet/runtime · GitHub
Skip to content

Add iOS build configurations - #33292

Merged
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios
Mar 11, 2020
Merged

Add iOS build configurations#33292
akoeplinger merged 14 commits into
dotnet:masterfrom
akoeplinger:add-ios

Conversation

@akoeplinger

Copy link
Copy Markdown
Member

This adds support for iOS using the Mono runtime to the build system.

I'm only adding the build integration right now, CI support will be done separately.

Comment threadeng/Subsets.props
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd expect iOS to set CLR_CMAKE_TARGET_DARWIN too. Not sure whether it would make the rest of the diff with if conditions smaller or larger though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes please, at least on cmake side, we are consistent with Darwin naming. It's only the code and RIDs where the (obsolete) osx name is used. 😁

@akoeplingerakoeplingerMar 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This was a conscious decision because CLR_CMAKE_TARGET_DARWIN really means macOS in the rest of the build scripts. We could rename it to CLR_CMAKE_TARGET_OSX?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll do the renaming in a follow-up PR.

# Manually set results from check_c_source_runs() since it's not possible to actually run it during CMake configure checking
unset(HAVE_SHM_OPEN_THAT_WORKS_WELL_ENOUGH_WITH_MMAP)
unset(HAVE_CLOCK_MONOTONIC) # only exists on iOS 10+
unset(HAVE_CLOCK_REALTIME) # only exists on iOS 10+

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the minimum target iOS? 9 stopped getting patches 3 years ago (modulo one patch for GPS rollover last year). Even 10 looks to be out of support other than the GPS fix.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The minimum target that Xamarin supports today is iOS 7 for devices and iOS 8 for the simulator so I replicated that here. We're discussing with PM but it looks like we won't be able to meaningfully bump the minimum version, definitely not to 10.

# Conflicts:
#	src/libraries/pkg/Directory.Build.props
#	src/mono/netcore/nuget/Directory.Build.props
It is only used for interacting with OpenSSL which isn't useful on iOS.
@marek-safar

Copy link
Copy Markdown
Contributor

@ViktorHofer any objections?

@ViktorHofer

Copy link
Copy Markdown
Member

I hadn't yet that time to review the PR. Please wait until #33242 is in (should be in ~1 hour) on which I was working the last few days to avoid conflicts. I will now review the PR.

# Conflicts:
#	eng/native/configuretools.cmake
#	src/installer/corehost/CMakeLists.txt
#	src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
@akoeplinger

Copy link
Copy Markdown
MemberAuthor

Sure. I'll need to push another merge anyway due to conflicts so I can wait for that PR to land.

@ViktorHofer
ViktorHofer requested a review from a teamMarch 10, 2020 14:21
@ViktorHofer

Copy link
Copy Markdown
Member

@ViktorHofer any objections?

a lot's going on today. I just requested review from dotnet/runtime-infrastructure as well. I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable. Please don't merge the change yet before me and the infra teams has taken a closer look. Thanks for your understanding.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

I peeked into the changes and the RuntimeFlavor one in Subsets.props is questionable.

@ViktorHofer let me know if what I proposed in #33292 (comment) makes more sense, thanks.

@directhex

Copy link
Copy Markdown
Contributor

I'm branching off this to try and get the AzDO YAML going. Will rebase as and when.

@directhexdirecthex mentioned this pull request Mar 10, 2020
Comment on lines +1 to +3
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR "configuretools.cmake needs to be included after configureplatforms.cmake")
endif()

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we instead:

Suggested change
if(NOT CLR_CMAKE_CONFIGURE_PLATFORM_INCLUDED)
message(FATAL_ERROR"configuretools.cmake needs to be included after configureplatforms.cmake")
endif()
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)

and remove three occurrences of include(${CLR_ENG_NATIVE_DIR}/configureplatform.cmake) under :/src/? This way we would only use configuretools.cmake as an entrypoint cmake for eng/native (and rest of the files will become internal detail, which however may get refactored in the future).

@am11am11Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or perhaps for all call sites, we can add an explicit :/eng/native/entrypoint.cmake enlisting:

include(${CMAKE_CURRENT_LIST_DIR}/functions.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configureplatform.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/configuretools.cmake)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes I think this makes sense. I'd prefer if we do it in a separate PR though :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have addressed it as part of Android fixes #32800.

Comment threadsrc/libraries/Directory.Build.props
Comment threadsrc/libraries/restore/runtime/runtime.depproj Outdated
@steveisok

Copy link
Copy Markdown
Member

@dotnet/runtime-infrastructure Can one or more of you give this a review? I'd like to have this merged today if possible.

Comment threadeng/Subsets.props Outdated

<_subsetCategory Condition="'$(SubsetCategory)' != ''">$(SubsetCategory.ToLowerInvariant())</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == ''">$(DefaultSubsetCategories)</_subsetCategory>
<_subsetCategory Condition="'$(SubsetCategory)' == '' and '$(TargetOS)' == 'iOS'">libraries-mono</_subsetCategory>

Copy link
Copy Markdown
Member

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 when TargetOS is iOS?

Copy link
Copy Markdown
MemberAuthor

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

Copy link
Copy Markdown
Member

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.

@ViktorHoferViktorHoferMar 11, 2020

Copy link
Copy Markdown
Member

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 that SubsetCategory should be inferred from TargetOS. The SubsetCategory can also contain values like mono, or mono-coreclr-libraries-installer therefore I think it's necessary to be intentional here and require the category to be passed in.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ViktorHofer why not? coreclr or installer are not valid subset categories when you have TargetOS=iOS so having a default when nothing is set explicitly makes a lot of sense.

Copy link
Copy Markdown
Member

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 not mono-libraries-installer?

@akoeplingerakoeplingerMar 11, 2020

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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 for TargetOS==iOS instead if that helps making it clearer.

Copy link
Copy Markdown
MemberAuthor

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadeng/native/naming.props Outdated
Comment threadsrc/libraries/Native/build-native.sh Outdated
<watchOS64_32VersionMin>5.1</watchOS64_32VersionMin>
<macOSVersionMin>10.13</macOSVersionMin>

<!-- Version of the OS SDK we target -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just curious, why are these set to empty?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Some Apple tools embed certain pieces of metadata differently depending on whether you build against /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk or /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk (the former is a symlink to the latter).

On CI we want to build with the explicit version so we pass in the versions installed on the bots. Locally we don't really care about that and hardcoding the version in the file would just mean you need to change it whenever Xcode is updated, so we rely on the symlink there.

I know MSBuild doesn't require you to declare properties if they're empty but I felt it serves as a good documentation that they're used/available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining.

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than a few comments/questions, the infra changes LGTM.

@akoeplinger

Copy link
Copy Markdown
MemberAuthor

dotnet-runtime-perf failures are due to #33082.

@akoeplinger
akoeplinger merged commit a54d391 into dotnet:masterMar 11, 2020
@akoeplinger
akoeplinger deleted the add-ios branch March 11, 2020 23:55
@steveisoksteveisok mentioned this pull request Mar 11, 2020
24 tasks
@@ -0,0 +1,272 @@
/*
* Copyright (c) 2000-2008 Apple Inc. All rights reserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we discussed with @richlander and we'll take care of that.

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

discussed various additions to this PR offline with @akoeplinger, like the necessity of the installer subset at a later point for the runtime pack to build. The subset validation as well should be enabled later. LGTM. Thanks!

akoeplinger pushed a commit that referenced this pull request Mar 17, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto 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.

11 participants

@akoeplinger@marek-safar@ViktorHofer@directhex@steveisok@filipnavara@am11@jkotas@bartonjs@safern@Dotnet-GitSync-Bot