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

[mono][NativeAOT] System.Globalization.Native build improvements #82393

Merged
merged 24 commits into from
Apr 25, 2023
Merged

[mono][NativeAOT] System.Globalization.Native build improvements #82393

merged 24 commits into from
Apr 25, 2023

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Feb 20, 2023 •
edited
Loading

Fixes #82389

  • Fixes build of System.Globalization.Native for iOS-like, browser, and wasi platforms
  • Removes ICU shim from static builds of Mono runtime and builds it as part of libs.native subset
  • Switches browser/wasi to use the static libraries

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 20, 2023
@ivanpovazan ivanpovazan added the os-ios Apple iOS label Feb 20, 2023
@ghost
Copy link

ghost commented Feb 20, 2023

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #82389

Author: filipnavara
Assignees: -
Labels:

os-ios, community-contribution
Milestone: -

@ghost
Copy link

ghost commented Feb 20, 2023

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

Issue Details

Fixes #82389

Author: filipnavara
Assignees: -
Labels:

os-ios, community-contribution, area-NativeAOT-coreclr
Milestone: -

@filipnavara
Copy link
Member Author

filipnavara commented Feb 20, 2023 •
edited
Loading

The build failures are relevant. I'll try to remove the duplicate build from Mono sources.

(For posterity, took me a while to trace the Mono ICU build all the way to #35790. I think there's no reason to link ICU shim directly into the runtime anymore. This is likely a relict of the same thing being done in CoreCLR in the past.)

@ivanpovazan ivanpovazan changed the title Add System.Globalization.Native builds for iOS-like platforms [NativeAOT] Add System.Globalization.Native builds for iOS-like platforms Feb 20, 2023
@filipnavara

This comment was marked as outdated.

Sign in to view
@build-analysis build-analysis bot mentioned this pull request Feb 20, 2023
Closed
@filipnavara filipnavara reopened this Feb 20, 2023
@build-analysis build-analysis bot mentioned this pull request Feb 21, 2023
Closed
@filipnavara filipnavara force-pushed the icu-ios branch 2 times, most recently from d59baf0 to bc07402 Compare February 22, 2023 08:44
@filipnavara
Copy link
Member Author

The WASI part has overlap with #82444.

@build-analysis build-analysis bot mentioned this pull request Mar 4, 2023
Closed
@filipnavara filipnavara requested a review from akoeplinger March 7, 2023 07:24
@kotlarmilos
Copy link
Member

@filipnavara is there anything for this PR or is it waiting for a review?

@filipnavara
Copy link
Member Author

@filipnavara is there anything for this PR or is it waiting for a review?

Waiting for review.

@kotlarmilos kotlarmilos requested review from am11 and pavelsavara April 11, 2023 09:02
@kotlarmilos
Copy link
Member

/cc: @akoeplinger @MichalStrehovsky @ivanpovazan

ivanpovazan
ivanpovazan approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@ivanpovazan ivanpovazan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Minor note: The title seems outdated as the PR changes now affect mono builds and other platforms as well. I don't have a strong opinion but something like [mono][NativeAOT] System.Globalization.Native build improvements might be more suitable.

Other than that, LGTM.
Thank you very much!

PS I would leave this to @akoeplinger for final approval.

@akoeplinger akoeplinger changed the title [NativeAOT] Add System.Globalization.Native builds for iOS-like platforms [mono][NativeAOT] System.Globalization.Native build improvements Apr 24, 2023
@akoeplinger
Fix NativeAOT build for Apple Mobile
42ae535 
The coreclr CMake "includes" the src/native directory so it doesn't go through build-native.sh.
@akoeplinger
Copy link
Member

Failures are unrelated.

@akoeplinger akoeplinger merged commit 0fc8978 into dotnet:main Apr 25, 2023
@filipnavara filipnavara deleted the icu-ios branch April 25, 2023 18:51
@lewing
Copy link
Member

lewing commented Apr 25, 2023

did anyone check the final post linking binary sizes on this?

@filipnavara
Copy link
Member Author

did anyone check the final post linking binary sizes on this?

Any specific configuration you are concerned about?

For NativeAOT/iOS there's nothing to compare against since it didn't work previously. For everything else it shuffles a bit where/how the globalization shim is built but it doesn't really add or remove any code, or at least not intentionally.

@steveisok
Copy link
Member

did anyone check the final post linking binary sizes on this?

Quick spot check on one of Alex's test official builds show no change. I'll check again when a real one gets through.

This was referenced Apr 25, 2023
Closed
Open
Closed
Closed
@jonathanpeppers jonathanpeppers mentioned this pull request Apr 28, 2023
Merged
jonathanpeppers added a commit to dotnet/android that referenced this pull request May 3, 2023
@dotnet-maestro @jonathanpeppers
Bump to dotnet/installer@0ce891843a 8.0.100-preview.5.23228.7 (#7994)
c332cfc 
Changes: dotnet/installer@f876fb5...0ce8918
Changes: dotnet/runtime@ab2b80d...9a7db55
Changes: dotnet/emsdk@2327f6b...31a4a87
Changes: dotnet/cecil@9a7551f...80d3f38

Updates:

* Microsoft.Dotnet.Sdk.Internal: from 8.0.100-preview.4.23224.14 to 8.0.100-preview.5.23228.7
* Microsoft.NETCore.App.Ref: from 8.0.0-preview.4.23221.1 to 8.0.0-preview.4.23225.14
* Microsoft.NET.Workload.Emscripten.Current.Manifest-8.0.100.Transport: from 8.0.0-preview.4.23218.1 to 8.0.0-preview.4.23219.1
* Microsoft.NET.ILLink.Tasks: from 8.0.0-preview.4.23221.1 to 8.0.0-preview.4.23225.14
* Microsoft.DotNet.Cecil: from 0.11.4-alpha.23178.1 to 0.11.4-alpha.23218.2

Other changes:

A new `libSystem.Globalization.Native.so` file was introduced in the Mono
runtime pack in:

dotnet/runtime#82393

Android doesn't actually need this file.

* Introduce a new private `@(_MonoExcludedLibraries)` item group

* Add exclusion for `libSystem.Globalization.Native`

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
@SingleAccretion SingleAccretion mentioned this pull request May 19, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ivanpovazan ivanpovazan ivanpovazan approved these changes

@akoeplinger akoeplinger akoeplinger approved these changes

@lewing lewing Awaiting requested review from lewing lewing is a code owner

@kg kg Awaiting requested review from kg

@vargaz vargaz Awaiting requested review from vargaz

@lambdageek lambdageek Awaiting requested review from lambdageek

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@marek-safar marek-safar Awaiting requested review from marek-safar

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

@am11 am11 Awaiting requested review from am11

@pavelsavara pavelsavara Awaiting requested review from pavelsavara pavelsavara is a code owner

Assignees
No one assigned
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member os-ios Apple iOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix NativeAOT ICU build integration for Apple mobile platforms
9 participants
@filipnavara @kotlarmilos @akoeplinger @lewing @steveisok @pavelsavara @lambdageek @am11 @ivanpovazan