-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add app-local support for ICU #35383
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Forgot to mention. I need to add tests for this, but I'm working on building a test package in runtime-assets repo with ICU in it. |
jkotas
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Show resolved
Hide resolved
jkotas
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Outdated
Show resolved
Hide resolved
jefgen
reviewed
Apr 24, 2020
jkotas
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Show resolved
Hide resolved
stephentoub
reviewed
Apr 24, 2020
stephentoub
reviewed
Apr 24, 2020
src/libraries/Native/Unix/System.Globalization.Native/pal_icushim.c
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Show resolved
Hide resolved
tarekgh
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Apr 24, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Apr 24, 2020
I've addressed all PR feedback except for: #35383 (review) which I plan to address today. Could I please get more reviews in the meantime? |
tarekgh
reviewed
Apr 30, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs
Show resolved
Hide resolved
tarekgh
reviewed
Apr 30, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Show resolved
Hide resolved
jkotas
reviewed
Apr 30, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Apr 30, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Show resolved
Hide resolved
jkotas
reviewed
Apr 30, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
safern
commented
May 2, 2020
All feedback is addressed 😄 |
jkotas
reviewed
May 2, 2020
src/coreclr/src/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Show resolved
Hide resolved
@tarekgh @jkotas @stephentoub does this look good now? |
tarekgh
approved these changes
May 6, 2020
Thanks all for the reviews 😄 |
yay!!!! 🔥 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #826
This adds supports for apps to carry their own copy of ICU.
How this works
Apps need to set a runtime config property:
It can also be via an env var in the form of
DOTNET_SYSTEM_GLOBALIZATION_APPLOCALICU
.<suffix>
: this is optional in the config property, but this follows the public ICU packaging conventions as when building a custom ICU you can customize it to produce the lib names and exported symbol names to contain a suffix. i.e:libicuucmyapp
wheremyapp
is the suffix. This can't be greater than 20 chars in length for the config switch.<version>
: this has to be a valid ICU version, i.e: 67.1. This version will be used to load the binaries and to get the exported symbols.How do we load ICU
To load ICU we use
NativeLibrary.TryLoad
api which does probing in different paths, first it tries to find the library inNATIVE_DLL_SEARCH_DIRECTORIES
property which is created by the dotnet host based on the deps.json file for the app. More explained hereFor self contained apps, the user doesn't really need to do anything special but to make sure the app has ICU side by side in the APP directory, as for self contained apps, the app directory is by default in
NATIVE_DLL_SEARCH_DIRECTORIES
.If you're consuming ICU via a NuGet package, this will work in framework-dependent apps as NuGet will resolve the native assets and include them in the deps.json file and in the output directory for the app under the
runtimes
dir, then we will load it from there.However, the tricky part comes whenever it is a framework-dependent app (no self contained) and ICU is part of the project. The SDK doesn't yet have a feature for "loose" native binaries to land into
deps.json
: dotnet/sdk#11373. However, there is a workaround by adding something like this to the csproj:Note that this will have to be done for all the ICU binaries for the supported runtimes. Also, the
NuGetPackageId
metadata in theRuntimeTargetsCopyLocalItems
item group, needs to match a NuGet package that the project actually references, it can't just be a dummy NuGet package.macOS behavior
MacOS has a different behavior for resolving dependent dynamic libraries from the load commands specified in the
match-o
file than the Linux loader. In the Linux loader, we could just loadlibicudata
first, thenlibicuuc
and lastlibicui18n
in that order to satisfy dependent libraries.However, in MacOS this doesn't work. When building ICU in MacOS, you by default get a dynamic library with these load commands in libicuuc for example:
These commands are just referencing the name of the dependent libraries for the other components of ICU, so the loader will do the search following the
dlopen
conventions. Which involve having these libraries in the system directories or setting theLD_LIBRARY_PATH
env vars, or having ICU at the app level directory.However, there are some directives for the loader, like
@loader_path
which tells the loader to search for that dependency in the same directory as the binary with that load command. So there's 2 ways to achieve this.instal_name_tool -change
Running:
patching ICU to produce the install names with @loader_path
Changing: https://github.com/unicode-org/icu/blob/ef91cc3673d69a5e00407cda94f39fcda3131451/icu4c/source/config/mh-darwin#L32-L37 to:
before running autoconf (./runConfigureICU).
Homebrew does this for all the installed packages: https://github.com/Homebrew/brew/blob/d5ffb96d241e6a19d6ac4563f93210821db23245/Library/Homebrew/extend/os/mac/keg_relocate.rb#L110
https://github.com/Homebrew/brew/blob/ef471e97675b97a87827bda2542e39fc07bfe7b3/Library/Homebrew/os/mac/keg.rb#L24
Also, Firefox patches ICU similar, using the @executable_path directive for the loader:
https://hg.mozilla.org/mozilla-central/file/tip/intl/icu-patches/bug-915735#l20
cc: @ericstj @danmosemsft @tarekgh @jkotas @janvorli @stephentoub
FYI: @jefgen @mjsabby