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

[iOS] HybridGlobalization Implement missing cases for GlobalizationNative_GetLocaleInfoString #89296

Merged
merged 17 commits into from
Aug 1, 2023
Merged

[iOS] HybridGlobalization Implement missing cases for GlobalizationNative_GetLocaleInfoString #89296

merged 17 commits into from
Aug 1, 2023

Conversation

mkhamoyan
Copy link
Contributor

@mkhamoyan mkhamoyan commented Jul 21, 2023 •
edited
Loading

Fixes #83514

Contributes to #80689

cc @SamMonoRT

@ghost ghost assigned mkhamoyan Jul 21, 2023
@ghost
Copy link

ghost commented Jul 21, 2023

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

Issue Details

Fixes #83514

Contributes to #80689

Author: mkhamoyan
Assignees: -
Labels:

area-System.Globalization
Milestone: -

@mkhamoyan mkhamoyan added os-ios Apple iOS os-tvos Apple tvOS os-maccatalyst MacCatalyst OS labels Jul 21, 2023
@ghost
Copy link

ghost commented Jul 21, 2023

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

Issue Details

Fixes #83514

Contributes to #80689

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

area-System.Globalization, os-ios, os-tvos, os-maccatalyst
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Jul 21, 2023
Closed
@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Jul 24, 2023
Closed
Open
@mkhamoyan mkhamoyan marked this pull request as ready for review July 24, 2023 12:03
@mkhamoyan mkhamoyan requested a review from ilonatommy July 24, 2023 12:03
@kotlarmilos kotlarmilos mentioned this pull request Jul 24, 2023
Closed
@build-analysis build-analysis bot mentioned this pull request Jul 24, 2023
Closed
@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jul 27, 2023
Closed
@mkhamoyan mkhamoyan removed the NO-REVIEW Experimental/testing PR, do NOT review it label Jul 28, 2023
@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 89296 in repo dotnet/runtime

ilonatommy
ilonatommy approved these changes Jul 28, 2023
View reviewed changes
Copy link
Member

@ilonatommy ilonatommy left a comment

Choose a reason for hiding this comment

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

Other than the failures, it looks good

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan mkhamoyan requested a review from kotlarmilos August 1, 2023 07:05
@mkhamoyan mkhamoyan changed the title [OSX] HybridGlobalization Implement missing cases for GlobalizationNative_GetLocaleInfoString [iOS] HybridGlobalization Implement missing cases for GlobalizationNative_GetLocaleInfoString Aug 1, 2023
kotlarmilos
kotlarmilos reviewed Aug 1, 2023
View reviewed changes
src/tests/build.proj
@@ -427,7 +427,10 @@
<_LinkerFlagsToDrop Include="@(NativeFramework->'-framework %(Identity)')" />
<LinkerArg Remove="@(_LinkerFlagsToDrop)" />
<ExtraAppLinkerArgs Include="@(LinkerArg)" />
<GlobalizationNativeLibs Include="$(MicrosoftNetCoreAppRuntimePackDir)runtimes/$(TargetOS)-$(TargetArchitecture)/native/icudt.dat" />
<GlobalizationNativeLibs Include="$(MicrosoftNetCoreAppRuntimePackDir)runtimes/$(TargetOS)-$(TargetArchitecture)/native/icudt.dat;
Copy link
Member

Choose a reason for hiding this comment

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

Looks good! Can you push your changes to the mirror azdo dotnet/runtime and invoke the iOS perf jobs in the https://dev.azure.com/dnceng/internal/_build/results?buildId=2233228&view=results?

I don't think any additional changes are needed for the sample app, but would be good to confirm that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://dev.azure.com/dnceng/internal/_build/results?buildId=2233344&view=results
When it will finish, we can check and proceed with merging this PR.

@mkhamoyan
Copy link
Contributor Author

Failures are not related.

@mkhamoyan mkhamoyan merged commit 2ac31f3 into dotnet:main Aug 1, 2023
@mkhamoyan mkhamoyan deleted the hybrid_locale_properties branch August 1, 2023 10:40
@ghost ghost locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kotlarmilos kotlarmilos kotlarmilos left review comments

@ilonatommy ilonatommy ilonatommy approved these changes

@steveisok steveisok Awaiting requested review from steveisok

@mdh1418 mdh1418 Awaiting requested review from mdh1418

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

Assignees

@mkhamoyan mkhamoyan

Labels
area-System.Globalization os-ios Apple iOS os-maccatalyst MacCatalyst OS os-tvos Apple tvOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement missing cases for GlobalizationNative_GetLocaleInfoString
4 participants
@mkhamoyan @akoeplinger @kotlarmilos @ilonatommy