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

[release/6.0-rc1] Fix loading app local ICU #57992

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 24, 2021

Backport of #57971 to release/6.0-rc1

Fixes #57942

/cc @tarekgh

Customer Impact

Customer reported that when the app opt-in using ICU app-local (feature which uses ICU NuGet package instead of using the system installed ICU libraries) the app crashes.

This regression happened because we have started using a new two ICU APIs and we loaded the pointers of these APIs from a specific ICU library libicui18n. This work with the installed system ICU libraries but didn't work with the ICU comes from the NuGet. Users using ICU app-local with this issue will not be able to start their app.
The fix is just to use the library name libicuuc instead. That will make it work in all scenarios when using the ICU system libraries or when using ICU app-local.

Testing

I have tested this manually with all scenarios when using ICU system libraries and when using ICU app-local. I tested this on Windows and Linux. Also, I have run the whole regression tests with the fix without any problem.

Risk

is very low. we are not changing any real code more than pointing at correct library and we have tested the fix in different environments with different settings.

@ghost
Copy link

ghost commented Aug 24, 2021

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

Issue Details

Backport of #57971 to release/6.0-rc1

/cc @tarekgh

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@tarekgh tarekgh self-assigned this Aug 24, 2021
@tarekgh tarekgh added this to the 6.0.0 milestone Aug 24, 2021
@tarekgh tarekgh added the Servicing-consider Issue for next servicing release review label Aug 24, 2021
@tarekgh
Copy link
Member

tarekgh commented Aug 24, 2021

CC @danmoseley @ericstj.

I am asking to port it to RC 1 branch. Should I manually port it to RC 2 too?

@danmoseley
Copy link
Member

@tarekgh no if it goes into RC1 then it will flow to RC2 automatically.

if we wanted to protect against future bugs like this using automation -- presumably we would need a test project to restore the nuget ICU package? is that a reasonable thing to do?

@danmoseley
Copy link
Member

wasm failure is xunit .. #11063

@tarekgh
Copy link
Member

tarekgh commented Aug 24, 2021

@danmoseley

if we wanted to protect against future bugs like this using automation -- presumably we would need a test project to restore the nuget ICU package? is that a reasonable thing to do?

Yes, we already have the issue #35930 tracking this work. @safern is already planning to get it early in 7.0.

@SteveMCarroll SteveMCarroll added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 24, 2021
@danmoseley
Copy link
Member

None of the staging failures are relevant (although, I can't root cause the tvOS crashes, not so many are crashing that it would look like a widespread failure in basic API, as if ICU wouldn't load)

@ViktorHofer
Copy link
Member

runtime (Build Browser wasm Release AllSubsets_Mono_RuntimeTests) failure is #11063

@ViktorHofer ViktorHofer merged commit faf0c8f into release/6.0-rc1 Aug 24, 2021
@ViktorHofer ViktorHofer deleted the backport/pr-57971-to-release/6.0-rc1 branch August 24, 2021 17:46
@ghost ghost locked as resolved and limited conversation to collaborators Sep 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants