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

Statically link the C++ runtime to libSkiaSharp.dll for windows platform #136

Closed
wieslawsoltes opened this issue Aug 15, 2016 · 26 comments
Closed

Comments

@wieslawsoltes
Copy link

As followup from issue #109.

I think there are two possible solutions:

  1. Do the static linking for libSkiaSharp.dll by default
  2. Provide separate package with statically linked libSkiaSharp.dll

I vote for option No 1 as solves most of deployment issues, only downside is increased size of dll.

@mattleibow
Copy link
Contributor

mattleibow commented Aug 15, 2016

Typically one would create an installer or the ClickOnce deployment and include the VC++ Runtime as part of the install package.

@migueldeicaza
Copy link
Contributor

Neither one of the original two options look very appealing, we need to digest the above.

Specially considering that @mattleibow 's suggestion is a viable option.

@mattleibow
Copy link
Contributor

mattleibow commented Aug 15, 2016

I found some info:

I think this sums it up nicely:

Dynamic linkage is basically essential if you want to pass CRT objects between binaries. If you allocate something (memory, files, any CRT thing) in a dll/exe that links to one CRT and try to free it (or even use it, files in particular) in a different dll/exe that links to a different CRT then bad things can happen. So keeping an option to dynamically link the CRT is probably important. You don't always need it, but when you do need it you can't really go without it.

@tdenniston
Copy link
Contributor

Looking in to this yesterday (specifically w.r.t. SkiaSharp applications, as in general the situation can be more complicated) I found another option if you don't want to deploy your application via an installer. Your VS license comes with a provision to distribute certain runtime DLLs with your application directly, meaning you don't have to package them via the redistributable installer that everyone is familiar with.

So the fix for issue #109 that doesn't require the redistributable installer is to copy the following two DLLs to the directory with your application .exe file: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\redist\<arch>\Microsoft.VC140.CRT\msvcp140.dll and ...\vcruntime140.dll. These appear to be the only runtime DLLs required by native Skia, at least in my brief testing.

@mattleibow
Copy link
Contributor

@tdenniston Thanks for this. This is definitely another, possibly the best so far, option.

We want to make sure we make the right choice here, because as soon as we do a static link, we cannot go back as we may cause even more issues.

@wieslawsoltes
Copy link
Author

@tdenniston Thanks for info, was looking for this solution myself but did not find this page.

It also covers the Community edition, which is very good:

https://www.visualstudio.com/en-us/support/legal/mt171547

This is newer version:

https://www.visualstudio.com/en-us/downloads/2015-redistribution-vs.aspx

I tested in VM and those 2 files are enough to run SkiaSharp succesfully.

@mattleibow I thinks we can close this issue as the solution provided by @tdenniston is as good as static linking for me. Maybe adding this info to README would help other too.

@prasannavl
Copy link

prasannavl commented Sep 23, 2016

@wieslawsoltes, @tdenniston is correct - but not entirely. Those are the only two non-system dlls that are required in general cases.

Dump of file libSkiaSharp.dll

File Type: DLL

  Image has the following dependencies:

    MSVCP140.dll
    VCRUNTIME140.dll
    api-ms-win-crt-string-l1-1-0.dll
    api-ms-win-crt-math-l1-1-0.dll
    api-ms-win-crt-stdio-l1-1-0.dll
    api-ms-win-crt-runtime-l1-1-0.dll
    api-ms-win-crt-heap-l1-1-0.dll
    api-ms-win-crt-environment-l1-1-0.dll
    api-ms-win-crt-convert-l1-1-0.dll
    api-ms-win-crt-filesystem-l1-1-0.dll
    api-ms-win-crt-time-l1-1-0.dll
    api-ms-win-crt-utility-l1-1-0.dll
    api-ms-win-crt-locale-l1-1-0.dll
    OPENGL32.dll
    KERNEL32.dll
    USER32.dll
    ADVAPI32.dll

The API-* are just the SxS assemblies provided by windows. However, on windows platforms without the Universal CRT, you may also have the need to install https://support.microsoft.com/en-us/kb/2999226.

I'm guessing the reason they worked without it, is that they were likely a part of Windows Update (I may be wrong).

On a side note, here's the Microsoft documentation that provides the full list of CRT deployment options: https://msdn.microsoft.com/en-us/library/ms235299.aspx

@ststeiger
Copy link

ststeiger commented Mar 28, 2017

I disagree with the installer solution.
Particularly, you don't want to require admin rights to "install" (xcopy deploy) an application or its dependencies.
The installation of the VC-runtime requires admin rights.
In order to avoid different version conflicts, you just need to make sure everything is compiled with the same C/C++-runtime. Since .NET Core has to statically links its C-runtime, it's necessary to use that one for SkiaSharp.

Requiring admin rights for the provisioning of an application is stupid.
Not even Google-Chrome does it, and that's an entire browser.

Not requiring an installation is the whole point of .NET-Core self-contained deployment.
Maybe it would be wiser to deploy libSkiaSharp.dll as part of .NET Core itselfs. If you wouldn't need the runtime for each platform in there, you could drastically reduce the file's size. I think that would be the proper way forward. Especially considering that fully-managed solutions like ImageSharp are way too slow (14 seconds to create an image thumbnail vs. 300ms with Skia).

Google has this very useful guiding principle: If it takes longer than 4 seconds, it's a bug.
You can apply that to deployment as well.
I also apply that directive to SQL and reports, and pretty much everything else.

@toptensoftware
Copy link
Contributor

I'd like to add my vote to making a statically linked version available, although first of all... does anyone know if a statically linked version actually works?

Assuming it does, here's my (possibly unique) rationale:

  1. My installer (typical Windows .exe installer built with InnoSetup) installs both the x86 and x64 versions of my app and is currently about 10mb. To include the MSVC redistributables for both platforms would bloat that to over 36mb.

  2. My app loads lots of different third party plugins and I've had stability problems with shared runtimes before. Admittedly that was a long time ago and I'm not sure why, but switching to statically linked runtime has helps reduce those problems. (basically I think some of these plugins are very poorly written so anything I can do to isolate my app is good).

  3. I'm not keen on installing just the two CRT dlls - sounds dangerous if plugins loaded by my app are dependant on those dlls they'll be expecting the full runtime in the system folder and opens the possibility of mismatches.

  4. On this point: "Dynamic linkage is basically essential if you want to pass CRT objects between binaries.". Does libSkiaSharp ever do this?

So I'm wondering what's the downside to including the statically linked versions in a separate folder in the nuget package (besides additional size)? This doesn't impact current users who want the shared runtime versions, but for anyone trying to package a static version, the correctly built versions of the dlls are there ready to go. It also means as new releases of SkiaSharp are made, we (those who want the statically linked version) don't need to make a special one-off build each time.

In other words, I think there are legitimate cases for wanting statically linked versions and having to build them ourselves is kinda painful.

@toptensoftware
Copy link
Contributor

To answer my own question, I've just built a statically linked version and it seems to work just fine. The size difference is about 300k:

x64 6,254,592 => 6,567,424
x86 4,857,856 => 5,134,848

Anyone else interested in this, there's a fork here with a new build target:

> .\bootstrapper.ps1 -Target externals-winstatic

@danroot
Copy link

danroot commented Jun 21, 2018

One scenario I'm facing this on, where an installer will not work, is deployment to Azure App Services.

@ststeiger
Copy link

ststeiger commented Jun 22, 2018

Is Skia still relevant anyway ?
ImageSharp has become a lot faster, and basically, the only thing Skia can do that imagesharp can't is webp and tiff.
The time might be better spent adding webp and/or tiff to imagesharp.

@mattleibow
Copy link
Contributor

mattleibow commented Jun 22, 2018

@ststeiger SkiaSharp can do far more that ImageSharp and also has the added support of doing everything on the GPU. SkiaSharp uses skia, which is the same engine that powers Android and Chrome.

Of course, if you like ImageSharp, then feel free to use that.

@mattleibow
Copy link
Contributor

@danroot Thanks for pointing out the case with Azure, I will investigate this and see what we can do.

@danroot
Copy link

danroot commented Jun 22, 2018

@mattleibow I may have misunderstood this issue. I landed here from #109 with a similar error in an Azure App Service. In my case though, I was eventually able to get it working in Azure App Services by using this in the csproj PropertyGroup section:

<ShouldIncludeNativeSkiaSharp>True</ShouldIncludeNativeSkiaSharp>
<PreferredNativeSkiaSharp>x86</PreferredNativeSkiaSharp>

And publishing the ASP.NET Core 2.1 app as "Self Contained".

When I do this, the right DLL lands in Azure App Service, and I can run locally from my dev VM. I have the app service set (intentionally) to 32-bit, the projects set to "Any CPU", and it seems to be working both in Azure and on dev machine. I had manually copied msvcp140,dll and vcruntime140.dll to the bin folder, but just removed those and SkiaSharp still works. So I'm thinking Azure App Services has the C++ runtimes already and you just need to be sure the right libSkiaSharp.dll lands as well. It's still a little odd to me. I'm having a hard time imagining a scenario I wouldn't want the native dll and would rather it all "just work" even if it means a few more bytes in the package. But I'm sure there's reasons I just don't understand, and this solves my immediate issue..

@mattleibow
Copy link
Contributor

@danroot Ah. I see. You are most probably working on a x64 machine? What is happening is that AnyCPU is checking your current build machine and detecting x64, so it copies the x64 libSkiaSharp.dll. You deploy to a x86 machine, but there is no way for the IDE to know this.

To work around this, you can add the <PreferredNativeSkiaSharp> element, as you did. This probably will break your local debug, so you can set the variable as part of the CI that builds this: /p:PreferredNativeSkiaSharp=x86. If you build AnyCPU, there should be a x86 and a x64 folder in the output directory that has the native files. As part of the deploy step, you can copy the correct one into the parent folder.

@ststeiger
Copy link

ststeiger commented Jun 25, 2018

@mattleibow

Of course, if you like ImageSharp, then feel free to use that.

Actually I don't.
I tried to like ImageSharp, but I still can't say I do.
Constantly changing everything, breaking everything, making it "hard" to NOT use a nuget package, and slow (albeit the speed is now more-or-less acceptable for my purposes), constant problem reading fonts.
What is nice is that ImageSharp uses SIMD and doesn't require a native library - but that's about it.
Sadly, the native library is kindof show-stopping when you want an easy deployment on Linux.
On the upside, Skia is a hell of a lot faster.
Don't know either if all the insecure code in ImageSharp is as secure as it should be.

@CoenraadS
Copy link

CoenraadS commented Sep 21, 2018

I also encountered this.

I have .Net Framework 4.7 Azure Function (x64 on Azure), depending on a Net Standard 2.0 class library using SkiaSharp.

I platform for all projects to x64 and in the class library I used

<ShouldIncludeNativeSkiaSharp>True</ShouldIncludeNativeSkiaSharp>
<PreferredNativeSkiaSharp>x64</PreferredNativeSkiaSharp>

But still this error occurs. I can see the dll in /bin/runtimes/win7-64/native but I think it should be in the /bin folder

@ststeiger
Copy link

ststeiger commented Sep 21, 2018

Or you can stream it ouf - write to

System.IO.Path.GetDirectoryName(
typeof(SomeClassInSkiaSharpAssembly).Assembly.Location
);

depending on System.IntPtr.Size * 8 == 64
on App_Start/Init.

anyway, that works for each operating system.
Just stream-out the right dll from embedded resources for the right operating system.
Containing dlls for 3 OSes and 4 platforms (x86, x64, arm-32, arm64) adds to executable size, however.

@CoenraadS
Copy link

I solved it using:

  <ItemGroup>
      <ContentWithTargetPath Include="$(OutDir)bin\runtimes\win7-x64\native\libSkiaSharp.dll">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <TargetPath>bin\libSkiaSharp.dll</TargetPath>
    </ContentWithTargetPath>
  </ItemGroup>

@mattleibow
Copy link
Contributor

After seeing what is happening with Windows Docker containers, we may need to revisit the static linking idea. To be honest, I never linked statically and don't know what the side effects are.

Right now, I have this single binary that works on Win 7+ and possible accross various server variants. If this affects loading of other assemblies, will this affect the loading process of the ANGLE binaries? What happens if we statically link to the VC2017 binaries and we run on Windows 7?

I need to do some more testing and determine if a static linking has side effects that will affect us. It may just be something that does not affect us in the least, in that case, there is no reason not to.

@kekekeks
Copy link
Contributor

kekekeks commented Oct 7, 2018

If this affects loading of other assemblies, will this affect the loading process of the ANGLE binaries

It only affects exported/imported C++ objects and memory allocated via CRT allocator (new/malloc). C++ objects would be incompatible and free/delete in one library won't be able to release memory allocated in another with new/malloc.

SkiaSharp and ANGLE aren't doing any of these things. They have plain C API (no C++ objects) and provide custom deallocators for all "object" types. So it's completely safe to use any CRT linking options: static, dynamic, they can even use different versions of C runtime.

VC2017 compiler and linker are compatible with Win7.

@mattleibow
Copy link
Contributor

@kekekeks I tried a test build with a statically linked Windows binary, and something is not quite right. The raster rendering appears to work just fine, but SkiaSharp chokes when trying to load the OpenGL interface.

It throws an AccessViolationException on this line:

https://github.com/mono/skia/blob/9ec6cb29f41014890f61ed1329fc934bfd9167ae/src/gpu/gl/win/GrGLMakeNativeInterface_win.cpp#L101

Here is my PR: #654 Maybe I am doing something wrong? I just ran the tests and I created a minimal sample using a SKGLControl on the desktop, and it consistently died.

@toptensoftware, did you make use of the GPU features?

Also, things might have changed - I no longer have a hard dependency on opengl32.dll as I now load it dynamically with LoadLibraryA and then each entry with GetProcAddress.

@mattleibow
Copy link
Contributor

mattleibow commented Oct 19, 2018

I may need to retest this one... I think something broke and now all GPU fails ;) Ran a clean, if this fails then a clean checkout may be necessary... Or maybe I broke my machine :(

EDIT: yup, everything is broken. Going full reset and nuking the entire repo 💥 Bwahahaaaa! Take that you piece of silicon!

mattleibow added a commit that referenced this issue Oct 29, 2018
 - dotnet publish improvements #659
 - statically link vcredist #136
 - clang O3 optimization bug #647
@mattleibow
Copy link
Contributor

OK. I have fixed the static link errors - it had to do with me writing bad C++ interop code. It appears to be working and most probably will read the next release. Yay! No more vc_redist installs!

As of this comment, the new package is undergoing testing and can be downloaded here if you are not part of the EAP: https://jenkins.mono-project.com/view/SkiaSharp/job/SkiaSharp-Pipeline/job/feature%252Fstatic-win32/12/Azure

mattleibow added a commit that referenced this issue Oct 29, 2018
* Now building a static Windows binary. #136
* Add support for building a "feature" version
@mattleibow
Copy link
Contributor

That's it folks! The next version will be statically linked. We hope to have a public preview in a few days.

@mattleibow mattleibow added this to the v1.68.0 milestone Nov 6, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants