danipen / TextMateSharp

A port of tm4e to bring TextMate grammars to dotnet ecosystem
MIT License
87 stars 15 forks source link

Include packages for x86/x64 onigwrap.dll, and dynamic binding between them #51

Closed dmitry-medvedev closed 1 year ago

dmitry-medvedev commented 1 year ago

We use TextMateSharp in our projects; on our tests, projects compiled with AnyCPU platform either do not copy onigwrap.dll to the bin folder (in case of packages.config), or copy x86 version of onigrwrap.dll (in case of sdk-style projects/package references). On the x64 platform, the project cannot load this dll and fails to run.

Please consider providing additional packages like TextMateShar.onigwrap.dll.Native.win-x64/TextMateShar.onigwrap.dll.Native.win-x86,

and implementing run-time binding to the correct version of onigwrap.dll depending on the platform.

Please take a look at ClearScript project which does it this way: https://www.nuget.org/packages/Microsoft.ClearScript

(different platforms are handled inside V8SplitProxyNative.Generated.cs)

danipen commented 1 year ago

@dmitry-medvedev are you planning a PR for this or do you want me to take a look (not sure if I will able to take a look soon since I am quite busy now)?

dmitry-medvedev commented 1 year ago

We will need this to work quite soon, as our release depends on it. We will do local changes first ourselves, and then submit a pull request.

danipen commented 1 year ago

Ok then. Thanks.

danipen commented 1 year ago

Not sure I understand the proposed solution. Are you proposing separating different nuget packages for each platform? In that case, how would you need to reference each nuget reference in the .csproj based on the current platform?

Note that, right now, the nuget package packs every version of the native libraries, and it places it on the right path, so the runtime is able to load them based on the platform:

An existing issue is that, in debug mode, using Visual Studio, only the x86 version is copied to the output dir, and probably, using x64 won't work (in my case it works fine for the Demo project but it fails for the unit tests).

But I never saw it failing when using nuget packages.

danipen commented 1 year ago

So probably, finding a better way of copying the native deps into the bin folder (bin\Debug\netstandard2.0) may fix the issue.

dmitry-medvedev commented 1 year ago

Yes, separate NuGet packages for every platform looks like an overkill. We will try the solution you proposed.

danipen commented 1 year ago

@dmitry-medvedev I have some time to have a look. Could you please confirm if #52 resolves the problem you were facing?

dmitry-medvedev commented 1 year ago

Many thanks for that!

We will test changes in #52 and will let you know ASAP. Changes to the theme names work great for us.

Dmitry

danipen commented 1 year ago

Awesome. Thanks for the contribution BTW! 😉

dmitry-medvedev commented 1 year ago

We've tested the new interop code with different platforms, and it works perfectly. Thank you!

danipen commented 1 year ago

A new version of TextMateSharp is available in NuGet including the PR that fixes this issue: https://www.nuget.org/packages/TextMateSharp/1.0.55

dmitry-medvedev commented 1 year ago

Thanks, we're now using it, and it works great. I will let you know once we release the software which uses TextMate package.

danipen commented 1 year ago

BTW and out of curiosity ... are you using AvaloniaEdit for the editor or are you using a different implementation?

dmitry-medvedev commented 1 year ago

No, we're not using Avalonia, it's our own implementation.

dmitry-medvedev commented 1 year ago

Hi Daniel,

I thought you might be interested. We've released a new version of AlterNET Studio, with a new TextMate-based parser as one of the major features: https://www.alternetsoft.com/news/alternet-studio-9-0-released

There are a couple of things that we had to work around when using the TextMateSharp package. Maybe that's something you may address in future releases:

  1. TextMateSharp depends on System.Text.Json of version 7.0.0.2, while other libraries that we use, such as Microsoft Code Analysis, use version 7.0.0.0. I wonder if there is a specific reason why you're using that specific version or if it can be downgraded to 7.0.0.0.

  2. We found that for .net applications using .Net Framework 4.6.2, x86 and x64 onigwrap libraries are not copied in the bin folder. On our tests, either the x64 or x86 bit version is copied. We solved this issue by adding onigwrap libraries to our own package that uses TextMateSharp with the following build target:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <ItemGroup Condition="'$(TargetFramework)' == '' Or '$(TargetFramework.TrimEnd(`0123456789`))' == 'net'">
    <None Include="$(MSBuildThisFileDirectory)..\runtimes\win-x64\native\onigwrap-x64.dll">
      <Link>onigwrap-x64.dll</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <Visible>false</Visible>
    </None>
    <None Include="$(MSBuildThisFileDirectory)..\runtimes\win-x86\native\onigwrap-x86.dll">
      <Link>onigwrap-x86.dll</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <Visible>false</Visible>
    </None>
  </ItemGroup>
</Project>

Kind regards, Dmitry

danipen commented 1 year ago

I wonder if there is a specific reason why you're using that specific version or if it can be downgraded to 7.0.0.0.

No prob about downgrading. I was just using latest.

danipen commented 1 year ago

We found that for .net applications using .Net Framework 4.6.2, x86 and x64 onigwrap libraries are not copied in the bin folder. On our tests, either the x64 or x86 bit version is copied.

Not sure about this one. The nugget package should deploy the native deps automatically...

Are you consuming TexmateSharp from the nuget package?

dmitry-medvedev commented 1 year ago

WindowsFormsApp7.zip

I'm attaching a sample project created with .NET Framework referencing TextMateSharp and TextMateSharp.Grammars packages. On our tests it does not copy onigwrap DLLs to the bin folder.

danipen commented 1 year ago

In this case you're not consuming TexmateSharp as a nuget package so probably the deps are not copied due to this reason.

If you want to add a PR with the needed changes to copy deps when referencing the project directly for .net framework it's ok for me.

danipen commented 1 year ago

No prob about downgrading. I was just using latest.

Same for this one. Add a PR if you want and I'll be happy to merge it 😊. Thanks!