Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
Jan 5, 2024
Merged

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0daa80b
Fix xamarin_macios build after system ICU change
mkhamoyan Dec 22, 2023
a5f62a6
Minor fix
mkhamoyan Dec 22, 2023
39bc3ea
Revert from mono/mini/CMakeLists.txt
mkhamoyan Dec 22, 2023
0672fa9
Merge remote-tracking branch 'origin/main' into fix_xamarin_macios_build
directhex Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/native/libs/System.Globalization.Native/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,13 @@ else()
endif()

if (CLR_CMAKE_TARGET_APPLE)
if(CLR_CMAKE_TARGET_IOS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vargaz Do you think this block is also unnecessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. directly into the mono runtime
  2. 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.

Copy link
Member

@am11 am11 Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting this PR #96658

add_definitions(-DTARGET_IOS)
elseif(CLR_CMAKE_TARGET_TVOS)
add_definitions(-DTARGET_TVOS)
elseif(CLR_CMAKE_TARGET_MACCATALYST)
add_definitions(-DTARGET_MACCATALYST)
endif()
directhex marked this conversation as resolved.
Show resolved Hide resolved
set(NATIVEGLOBALIZATION_SOURCES_OBJC
pal_locale.m
pal_collation.m
Expand Down
Loading