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.

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

Globalization support in .NET 5 #102

Merged
merged 21 commits into from
May 5, 2020

Conversation

steveisok
Copy link
Member

No description provided.

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Added some little comments. LGTM otherwise.

@danmoseley
Copy link
Member

cc @ericstj


A default build of ICU normally results in over 16 MB of data, and a substantial amount of object code.

The packages have to be split to implementation and data part and ideally the data parts will be also sliced in the way that the developers can select only relevant data for their app if they for example don’t target Chinese market.
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to statically link the ICU and the System.Globalization shim together, so that we only carry implementation for the the ICU APIs that the shim needs?

Copy link
Member

Choose a reason for hiding this comment

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

Would that break app local scenarios? This way if people want to upgrade to a newer ICU, they couldn't because we carry the implementations that we built against, no?

Copy link
Member

Choose a reason for hiding this comment

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

This paragraph is proposing to have our own custom build of ICU. There would not be a newer version unless we build it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. I thought you were proposing that for all scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what the implications are, if any, behind that. I'm sure it's something we could determine during when implementing.

@danmoseley
Copy link
Member

@steveisok what remains to call this final - do you need more signoffs, are there more edits to make? If it's final, we can get started.

@steveisok
Copy link
Member Author

I addressed the feedback and I think it's ready to go.

@steveisok
Copy link
Member Author

@marek-safar There are a few comments that you are better equipped to address. Other than that, I think I've incorporated the feedback.

@steveisok
Copy link
Member Author

@danmosemsft @stephentoub Can I get your approval?

@danmoseley
Copy link
Member

If @safern / @stephentoub / @tarekgh are signed off that is good enough for me.

@danmoseley
Copy link
Member

Any signoffs required other than @stephentoub ? Any mono folks?

@spouliot
Copy link

spouliot commented May 5, 2020

TBH it still bugs me that we do not have much data about the size impact for (mobile) apps where the native bits (and data) will need to added to the application bundles: iOS, tvOS and watchOS.

It's also not clear for macOS versus the Mac App Store (which is a target for Xamarin.Mac) mostly because it never was an issue (XM never linked to libicu). If it is considered a private framework on macOS (like other Apple platforms) then the current "use system bits" won't work for Xamarin.Mac.

@danmoseley
Copy link
Member

My inclination I think would be to capture anything remaining as unknowns/TBD and merge this as it's otherwise in a good state.

steveisok and others added 15 commits May 5, 2020 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.