[HybridGlobalization] Fix xamarin_macios build after system ICU change #96270

mkhamoyan · 2023-12-22T09:10:45Z

After #93220 xamarin-macios build is failing because of not recognising targets properly.
Issue is that this gives false and it wants to define wrong set of functions.
With this PR adding target definitions also in src/mono/mono/mini/CMakeLists.txt and src/native/libs/System.Globalization.Native/CMakeLists.txt

ghost · 2023-12-22T09:10:49Z

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

After #93220 xamarin-macios build is failing because of not recognising targets properly.
Issue is that this gives false and it wants to define wrong set of functions.
With this PR adding target definitions also in src/mono/mono/mini/CMakeLists.txt and src/native/libs/System.Globalization.Native/CMakeLists.txt

Author:	mkhamoyan
Assignees:	-
Labels:	`area-System.Globalization`, `os-ios`
Milestone:	-

mkhamoyan · 2023-12-22T09:11:51Z

/azp run runtime-maccatalyst

mkhamoyan · 2023-12-22T09:11:58Z

/azp run runtime-ioslikesimulator

azure-pipelines · 2023-12-22T09:11:59Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2023-12-22T09:12:07Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-22T09:12:10Z

/azp run runtime-ioslike

azure-pipelines · 2023-12-22T09:12:19Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-22T11:08:51Z

/azp run runtime-ioslike

azure-pipelines · 2023-12-22T11:09:04Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-22T11:09:04Z

/azp run runtime-maccatalyst

azure-pipelines · 2023-12-22T11:09:13Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-22T11:09:18Z

/azp run runtime-ioslikesimulator

azure-pipelines · 2023-12-22T11:09:27Z

Azure Pipelines successfully started running 1 pipeline(s).

src/mono/mono/mini/CMakeLists.txt

mkhamoyan · 2023-12-22T11:42:31Z

src/native/libs/System.Globalization.Native/CMakeLists.txt

@@ -107,6 +107,13 @@ else()
 endif()

 if (CLR_CMAKE_TARGET_APPLE)
+    if(CLR_CMAKE_TARGET_IOS)


@vargaz Do you think this block is also unnecessary?

That might be needed, those defines are not set in that project. I think the issues is that in some cases these files are compiled as part of the mono runtime, and sometimes they are compiled into a separate lib ?

Yes, when I build runtime locally I see that we compile the globalization code twice:

directly into the mono runtime

from src/native/System.Globalization.Native

That is why I don't see any failure on runtime, but xamarin-macios maybe is taking src/native/System.Globalization.Native compiled into separate lib.

xamarin-macios maybe is taking src/native/System.Globalization.Native compiled into separate lib.

I am not sure how is it possible for xamarin to skip cmake inclusions needed to build this cmake list? These TARGET_XX variables are defined in eng/native/configurecompiler.cmake along with many compiler flags, and configurecompiler is ultimately included by this cmake list. If xamarin is skipping configurecompiler include somehow (which seems unlikely), then fixing it this way won't be enough as it will break something else. We should fix xamarin build instead so it builds the runtime's native lib subset properly as intended; with the entire cmake include chain.

cc @jkoritzinsky, @akoeplinger

xamarin-macios maybe is taking src/native/System.Globalization.Native compiled into separate lib.

I am not sure how is it possible for xamarin to skip cmake inclusions needed to build this cmake list? These TARGET_XX variables are defined in eng/native/configurecompiler.cmake along with many compiler flags, and configurecompiler is ultimately included by this cmake list. If xamarin is skipping configurecompiler include somehow (which seems unlikely), then fixing it this way won't be enough as it will break something else. We should fix xamarin build instead so it builds the runtime's native lib subset properly as intended; with the entire cmake include chain.

cc @jkoritzinsky, @akoeplinger

xamarin/xamarin-macios doesn't build anything from dotnet/runtime, we consume the static/dynamic libraries dotnet/runtime ships. The problem is that those libraries aren't built correctly.

It is still mystery to me why the change in this file is needed. Why are these defines not getting picked up from eng/native/configurecompiler.cmake as @am11 pointed out?

Have we validated that this change is actually fixing the problem?

In xamarin-macios, dotnet/runtime artifacts are consumed in two different ways:

For some scenarios a single libmonosgen-2.0.dylib is consumed: this file is built from src/mono/mono/mini/CMakeLists.txt and it includes a copy of System.Native.Globalization. This version of the globalization code is built by these rules: https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/CMakeLists.txt#L59-L111. and is consumed here:

runtime/src/mono/mono/mini/CMakeLists.txt

Lines 429 to 431 in 806c365

if(HAVE_ICU_SHIM)

target_link_libraries(monosgen-shared PRIVATE icu_shim_objects)

endif()

In this case the cmake defines that are floating around are coming from how the mono runtime is configured (which does not yet use configurecompiler.cmake and configureplatform.cmake).

For some scenarios xamarin-macios instead consumes a collection of static libraries: libmono-sgen-2.0.a, libSystem.Globalization.Native.a, libmono-component-debugger.a etc. In this case libSystem.Globalizaiton.Native.a is built from src/native/libs/CMakeLists.txt which includes configureplatform.cmake, but does not include the plethora of Mono definitions.

On the other hand, the C source files comprising src/native/libs/System.Globalization.Native depend on defines like TARGET_MACCATALYST that are part of the mono build, but weren't yet part of the build for the static libSystem.Globalization.Native.a.

By the way @mkhamoyan there's similar logic in https://github.com/dotnet/runtime/blob/main/src/native/libs/System.Security.Cryptography.Native.Apple/CMakeLists.txt#L60-L70. Maybe it would make more sense to move it up to src/libs/native/CMakeLists.txt or to some common .cmake fragment, instead?

Also - I'm not sure why we link the icu shim directly into libmonosgen-2.0.dylib - I'm pretty sure we don't do that with crypto, but we can apparently still ensure that the symbols are available at runtime so that all the pinvokes can find them.

the C source files comprising src/native/libs/System.Globalization.Native depend on defines like TARGET_MACCATALYST that are part of the mono build, but weren't yet part of the build for the static libSystem.Globalization.Native.a.

I do not think that this is correct. These defines are defined in configurecompiler.cmake too. For example, TARGET_MACCATALYST is defined at https://github.com/dotnet/runtime/blob/main/eng/native/configurecompiler.cmake#L697 .

I expect that we are going to find that this fix is not addressing the xamarin-macios build break.

I do not think that this is correct. These defines are defined in configurecompiler.cmake too.

I expect that we are going to find that this fix is not addressing the xamarin-macios build break.

Yea, I agree now, I don't see how this change would fix anything.

Yeah, I don't think it addresses it either. @mkhamoyan pointed this out to me earlier today in that the xamarin build may need to be modified because only a subset of the pinvokes will be active since we are shipping with hybrid globalization always on. We plan on adding the ability to turn it off and use our ICU in a later change.

Reverting this PR #96658

mkhamoyan · 2023-12-22T13:03:27Z

/azp run runtime-maccatalyst

azure-pipelines · 2023-12-22T13:03:36Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-22T13:03:38Z

/azp run runtime-ioslikesimulator

azure-pipelines · 2023-12-22T13:03:46Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-22T13:03:52Z

/azp run runtime-ioslike

azure-pipelines · 2023-12-22T13:04:00Z

Azure Pipelines successfully started running 1 pipeline(s).

src/native/libs/System.Globalization.Native/CMakeLists.txt

directhex · 2024-01-04T21:08:29Z

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

azure-pipelines · 2024-01-04T21:08:44Z

Azure Pipelines successfully started running 3 pipeline(s).

directhex · 2024-01-05T14:41:47Z

There are a lot of test failures, but I think this PR is innocent.

iOS and tvOS failures are dotnet/xharness#1126 & dotnet/dnceng#1749

Mac Catalyst failures are PNSE in System.Security.Cryptography.X509Certificates.Tests.CertTests.PublicPrivateKey_IndependentLifetimes_DSA() which is unrelated to this PR

browser-wasm failure is assertion issues in System.Runtime.InteropServices.JavaScript.Tests.WebWorkerTest.ManagedDelay_ContinueWith which, again, seems unrelated to these changes.

directhex · 2024-01-05T14:43:09Z

I'm gonna hit merge, @rolfbjarne seems happy enough and I don't see how any of the failing tests are related.

…CU change (#96270)" This reverts commit ab8c3b8.

…CU change (#96270)" (#96658) This reverts commit ab8c3b8.

…CU change (dotnet#96270)" (dotnet#96658) This reverts commit ab8c3b8.

Fix xamarin_macios build after system ICU change

0daa80b

mkhamoyan added area-System.Globalization os-ios Apple iOS labels Dec 22, 2023

ghost assigned mkhamoyan Dec 22, 2023

mkhamoyan requested review from steveisok, mdh1418 and rolfbjarne December 22, 2023 09:13

mkhamoyan marked this pull request as ready for review December 22, 2023 09:13

mkhamoyan requested review from vargaz, lambdageek and SamMonoRT as code owners December 22, 2023 09:13

mkhamoyan mentioned this pull request Dec 22, 2023

Numerous GlobalizationNative P/Invokes in System.Private.CoreLib.dll don't have a corresponding native function anymore #96251

Closed

rolfbjarne approved these changes Dec 22, 2023

View reviewed changes

Minor fix

a5f62a6

vargaz reviewed Dec 22, 2023

View reviewed changes

src/mono/mono/mini/CMakeLists.txt Outdated Show resolved Hide resolved

mkhamoyan commented Dec 22, 2023

View reviewed changes

Revert from mono/mini/CMakeLists.txt

39bc3ea

jkotas reviewed Dec 22, 2023

View reviewed changes

src/native/libs/System.Globalization.Native/CMakeLists.txt Show resolved Hide resolved

marek-safar added this to the 9.0.0 milestone Jan 4, 2024

Merge remote-tracking branch 'origin/main' into fix_xamarin_macios_build

0672fa9

This was referenced Jan 5, 2024

NRE in iOS.Device.Aot.Test when starting up xharness #96403

Closed

[wasm] Failing test System.Runtime.InteropServices.JavaScript.Tests.WebWorkerTest.ManagedDelay_ContinueWith #96493

Closed

[6.0] Android test failure: Failed to install mobile app #83422

Open

directhex merged commit ab8c3b8 into dotnet:main Jan 5, 2024
197 of 212 checks passed

mkhamoyan added a commit that referenced this pull request Jan 9, 2024

Revert "[HybridGlobalization] Fix xamarin_macios build after system I…

47a6dfe

…CU change (#96270)" This reverts commit ab8c3b8.

mkhamoyan mentioned this pull request Jan 9, 2024

Revert "[HybridGlobalization] Fix xamarin_macios build after system ICU change" #96658

Merged

jkotas pushed a commit that referenced this pull request Jan 9, 2024

Revert "[HybridGlobalization] Fix xamarin_macios build after system I…

57bcccd

…CU change (#96270)" (#96658) This reverts commit ab8c3b8.

tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024

Revert "[HybridGlobalization] Fix xamarin_macios build after system I…

cc27ed2

…CU change (dotnet#96270)" (dotnet#96658) This reverts commit ab8c3b8.

github-actions bot locked and limited conversation to collaborators Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HybridGlobalization] Fix xamarin_macios build after system ICU change #96270

[HybridGlobalization] Fix xamarin_macios build after system ICU change #96270

mkhamoyan commented Dec 22, 2023

ghost commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan Dec 22, 2023

vargaz Dec 22, 2023

mkhamoyan Dec 22, 2023

am11 Dec 23, 2023 •

edited

Loading

rolfbjarne Jan 2, 2024

lambdageek Jan 8, 2024

jkotas Jan 8, 2024

lambdageek Jan 8, 2024

steveisok Jan 8, 2024

mkhamoyan Jan 9, 2024

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

directhex commented Jan 4, 2024

azure-pipelines bot commented Jan 4, 2024

directhex commented Jan 5, 2024

directhex commented Jan 5, 2024

	if(HAVE_ICU_SHIM)
	target_link_libraries(monosgen-shared PRIVATE icu_shim_objects)
	endif()

[HybridGlobalization] Fix xamarin_macios build after system ICU change #96270

[HybridGlobalization] Fix xamarin_macios build after system ICU change #96270

Conversation

mkhamoyan commented Dec 22, 2023

ghost commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 Dec 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

mkhamoyan commented Dec 22, 2023

azure-pipelines bot commented Dec 22, 2023

directhex commented Jan 4, 2024

azure-pipelines bot commented Jan 4, 2024

directhex commented Jan 5, 2024

directhex commented Jan 5, 2024

am11 Dec 23, 2023 •

edited

Loading