Include System Directory in search dirs for app local ICU

[safern]

Windows ICU depend on VC Redist dlls which are on System32.

cc: @jkotas @tarekgh @jefgen

[ghost]

Tagging subscribers to this area: @tarekgh, @safern, @krwq
Notify danmosemsft if you want to be subscribed.

[jefgen]

Thank you @safern! :)

Windows ICU depend on VC Redist dlls which are on System32.

Just to expand a bit and give some more info:
Public builds of ICU on Windows normally use dynamic linking for the CRT (C Runtime) which is in the VC Redist DLLs.

Since this only allows loading from within the app directory, this means that when .NET attempts to load the icuuc*.dll and it's dependencies, it only looks for the msvc*.dll inside the app directory. Since most apps don't include the VC Redist DLLs, this means that app-local ICU on Windows fails to load the app provided ICU DLL(s).

[ericstj]

We are very careful and intentional to not require VC redist for .netcore components. We don’t want apps to have to include it nor bootstrap the redist in their installers. Would it make sense to change the libs used by ICU to avoid this dependency?

[jkotas]

We are very careful and intentional to not require VC redist for .netcore components.

I do not think it is the case: https://docs.microsoft.com/en-us/dotnet/core/install/windows?tabs=netcore31#additional-deps

[ericstj]

@joktas that's only on down-level OS's. On Win10 we don't require it. In 3.x we had issues related to this with regards to new WPF native components. WPF is special due to the C++/CLI but they also had a dependency in the native components. Pretty much any time we add a new native component to the stack we get this wrong and see a bunch of feedback from customers around this, so it'd be good to consider removing the dependency. I can dig up some pointers.

Even taking .NETCore out of the picture, It's an anti-pattern to have a NuGet package with a pre-requisite on a machine-wide install. If possible that should be avoided.

To be clear, I am fine with this change, but I think the ICU build should consider building in a way similar to the rest of .NETCore native binaries so as to not introduce new dependencies.

[jefgen]

Even taking .NETCore out of the picture, It's an anti-pattern to have a NuGet package with a pre-requisite on a machine-wide install. If possible that should be avoided.

I think that's a really good point.

Would it make sense to change the libs used by ICU to avoid this dependency?

We could certainly consider making this change in MS-ICU, so that any NuGet packages created for it would use static linking for the CRT.

However, I'm not sure how the ICU-TC would feel about making this change in public ICU. Considering it has used dynamic linking to the CRT since as far back as I can tell, it would be a bit of a breaking change perhaps. There's also the security implications of having to rebuild if there's an update to the CRT, etc. that you avoid with a machine wide install.

That said, even if we could make this change in public ICU, it wouldn't be back-ported to older versions, and wouldn't be available until the next release of public ICU (which is approximately this upcoming Oct./Nov. time-frame).

I'll file an issue internally for MS-ICU to consider using the static CRT though, as I think that for a NuGet package it does make sense to try and have it be self-contained.

[safern]

Will merge as we discuss this further.

[ericstj]

Thanks @jefgen. (and thanks @safern for the fix)

[jkotas]

static CRT though

Note that .NET Core does not statically link the whole CRT.

.NET Core dynamically links to UCRT. UCRT is the bulk of the CRT code. UCRT comes with Win8+ by default; and it can be installed on Win7 via the KB above.

.NET Core statically links to VCRuntime only. VCRuntime is relatively small amount of code that is tied to C++ compiler version. VCRuntime does not come with the OS. You have to install the VC redist to get it.