Skip to content

[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

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

mkhamoyan
Copy link
Contributor

@mkhamoyan mkhamoyan commented Jul 4, 2024

Contributes to #99521

Copy link
Contributor

Tagging subscribers to this area: @directhex, @matouskozak
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

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

@mkhamoyan mkhamoyan added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 4, 2024
@mkhamoyan
Copy link
Contributor Author

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

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@matouskozak matouskozak left a 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?

@mkhamoyan
Copy link
Contributor Author

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.
If you could help with the end-to-end testing with Xamarin/MAUI, that would be greatly appreciated!

@ivanpovazan
Copy link
Member

Shouldn't we remove the references from all places in regard to targeting Apple mobile in general?

From AppleAppBuilder:

public bool HybridGlobalization { get; set; }

bool hybridGlobalization,

From: Dotnet perf pipelines:

- script: make build-appbundle TARGET=ios MONO_ARCH=arm64 MONO_CONFIG=Release AOT=True USE_LLVM=False DEPLOY_AND_RUN=false STRIP_DEBUG_SYMBOLS=false HYBRID_GLOBALIZATION=${{ parameters.hybridGlobalization }}

hybridGlobalization: ${{ parameters.hybridGlobalization }}

etc..

@mkhamoyan
Copy link
Contributor Author

Shouldn't we remove the references from all places in regard to targeting Apple mobile in general?

From AppleAppBuilder:

public bool HybridGlobalization { get; set; }

bool hybridGlobalization,

From: Dotnet perf pipelines:

- script: make build-appbundle TARGET=ios MONO_ARCH=arm64 MONO_CONFIG=Release AOT=True USE_LLVM=False DEPLOY_AND_RUN=false STRIP_DEBUG_SYMBOLS=false HYBRID_GLOBALIZATION=${{ parameters.hybridGlobalization }}

hybridGlobalization: ${{ parameters.hybridGlobalization }}

etc..

Yes, these references should be removed across all relevant files targeting Apple mobile.
However, for this particular PR, the focus is on cleaning up ICU related files.
We can plan a separate effort to address these references in the future.

@ivanpovazan
Copy link
Member

I agree we can split it, but if you choose to split it, then I would suggest dividing this PR into two:

Comment on lines -144 to -146
<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'" />
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the verification!

@mkhamoyan
Copy link
Contributor Author

I agree we can split it, but if you choose to split it, then I would suggest dividing this PR into two:

Reverted changes in sample, testing from this PR.
Created a ticket for the second part #105130.

Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mkhamoyan mkhamoyan merged commit b9e842a into dotnet:main Jul 19, 2024
85 of 88 checks passed
@mkhamoyan mkhamoyan deleted the clean_up branch July 19, 2024 12:13
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-mono area-System.Globalization NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) os-ios Apple iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants