-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[iOS][non-icu] Clean up ICU related files from runtime pack #104443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Tagging subscribers to this area: @directhex, @matouskozak |
Tagging subscribers to this area: @dotnet/area-system-globalization |
/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Do you know if we should do some e2e testing with Xamarin/MAUI before merging this PR?
For xamarin-macios, I have already done the cleanup, see in this PR. |
Shouldn't we remove the references from all places in regard to targeting Apple mobile in general? From AppleAppBuilder:
runtime/src/tasks/AppleAppBuilder/Xcode.cs Line 186 in 2b3088b
From: Dotnet perf pipelines:
etc.. |
Yes, these references should be removed across all relevant files targeting Apple mobile. |
I agree we can split it, but if you choose to split it, then I would suggest dividing this PR into two:
|
<NetCoreAppNativeLibrary Include="icudata" Condition="'$(_IsiOSLikePlatform)' == 'true' and '$(InvariantGlobalization)' != 'true' and '$(HybridGlobalization)' != 'true'" /> | ||
<NetCoreAppNativeLibrary Include="icui18n" Condition="'$(_IsiOSLikePlatform)' == 'true' and '$(InvariantGlobalization)' != 'true' and '$(HybridGlobalization)' != 'true'" /> | ||
<NetCoreAppNativeLibrary Include="icuuc" Condition="'$(_IsiOSLikePlatform)' == 'true' and '$(InvariantGlobalization)' != 'true' and '$(HybridGlobalization)' != 'true'" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We verified that these changes allow NativeAOT-iOS LibraryMode to work without setting the <HybridGlobalization>true</HybridGlobalization>
in the csproj file.
The verification was done by manually updating the $HOME/.nuget/packages/microsoft.dotnet.ilcompiler/9.0.0-preview.6.24327.7/build/Microsoft.NETCore.Native.Unix.targets
file and rebuilding a NativeAOT classlib project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the verification!
Reverted changes in sample, testing from this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Contributes to #99521