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

Expected icu*.dlls are not loaded at runtime #20

Closed
conniey opened this issue Dec 8, 2016 · 4 comments
Closed

Expected icu*.dlls are not loaded at runtime #20

conniey opened this issue Dec 8, 2016 · 4 comments
Labels

Comments

@conniey
Copy link
Contributor

conniey commented Dec 8, 2016

Description

The code for dynamically loading the icu libraries at runtime is causing some unexpected behaviour.

  • If you have a newer set of icu assemblies in your PATH because of another program, it ends up loading those assemblies rather than the installed ones from the package Icu4c.Win.Full.Lib.54.1.3-beta3. Repro below.
  • Running tests outside of VS results in failures because it is unable to load icu libraries. (ie, running D:\git\tools\nunit3-console.exe D:\git\icu.net\output\Debug\icu.net.tests.dll when from a directory that is not where icu.net.tests.dll is located.)
  • Makes it really difficult to support .NET Core loading of packages. Packages are no longer continuously downloaded and stored in the repository (under packages folder). It is stored in a NuGet cache (ie. %USERPROFILE%\.nuget\packages). As a result, when running on .NET Core, the dependencies are resolved in their cache folder and not actually copied to the program's binary output folder. The .NET runtime knows how resolve these runtime dependencies when you use [DllImport] but since we are calling a Windows function LoadLibrary, it bypasses that logic.

Repro Steps:

  1. Copy the test case below into a test file in icu.net.tests.dll
  2. Have a newer version of the icu dlls in a directory that is in your PATH.
    • In my case, I have MiKTeX installed on my machine. It comes with icu version 57.1 installed.
  3. Run the test below.
public class TestClass
{
    [Test]
    public void TestForCorrectVersion()
    {
        var version = Wrapper.IcuVersion;
        Assert.AreEqual("54.1", version);
    }   
}

Expected Behaviour

The test passes because the Icu4c.Win.Full.Lib contains v54.1 libraries.

Actual Behaviour

Test fails because it loads C:\Program Files (x86)\MiKTeX 2.9\miktex\bin\icuuc57.dll and outputs 57.1.

Proposed Solution

One solution is to revert the dynamic loading with [DllImport] and manage the native library names with an approach that LibGit2Sharp uses. They have a setup that allows for the NativeDllName.Name to be generated at build time so it is not hard-coded. Consequently, their NativeMethods looks like the one below:

internal static class NativeMethods
{
        private const string libgit2 = NativeDllName.Name;

        [DllImport(libgit2)]
        internal static extern unsafe GitError* giterr_last();
}

They have two packages similar to icu.net called LibGit2Sharp.NativeBinaries and LibGit2Sharp.

LibGit2Sharp.NativeBinaries contains:

The LibGit2Sharp project has a dependency on the LibGit2Sharp.NativeBinaries package. At build time, it has a couple of .targets that run to generate the correct native library name.

  • NativeDllName.targets: Takes the embedded resource libgit2_filename.txt and calls their custom build task to generate the NativeDllName class.
  • GenerateNativeDllNameTask: A MSbuild task that reads libgit2_filename and outputs the NativeDllName class.

Considerations

Two nuget packages of icu.net would have to be made. One for the minimal ICU build and other for the full ICU build because the dependencies are linked to the project.

@ermshiperete
Copy link
Member

The first two bullet points will be fixed by #21.

Thanks for mentioning the third one. That's something we'll have to remember and fix when we have a .NET Core version.

@ermshiperete
Copy link
Member

Capture the third bullet point in #23.

@conniey
Copy link
Contributor Author

conniey commented Dec 13, 2016

@ermshiperete I pulled the latest from master and am still experiencing the same issue where I expect 56.1 to be loaded, but it is loading version 57.1 when running that test.

This is because the dynamic loading starts from version 60 downwards to 56. So it will check for the existence of icu 57 on my computer before the version my assembly has packaged.

ermshiperete added a commit that referenced this issue Dec 15, 2016
If there are unmanaged ICU libraries in the same directory
as icu.net.dll we will now use those even if we find a higher versioned
ICU elsewhere on the path.

This fixes most of #20.
@ermshiperete
Copy link
Member

@conniey With #21 now merged it should work iff the 56.1 binaries are in the same directory as icu.net.dll. The NativeMethodsTests test exactly that scenario. Can you try again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants