ericsink / SQLitePCL.raw

A Portable Class Library (PCL) for low-level (raw) access to SQLite
Apache License 2.0
512 stars 106 forks source link

"Library e_sqlite3 not found" on net472, but works on netcoreapp3.1 #389

Closed AArnott closed 2 years ago

AArnott commented 3 years ago

Given the most trivial netstandard2.0 library that consumes SQLitePCLRaw.bundle_green 2.0.4, that is then brought in by a unit test project that targets both netcoreapp3.1 and net472, we see the net472 one uniquely fails with this error:

Library e_sqlite3 not found

Yet looking at the bin directory for each target, we see both net472 and netcoreapp3.1 has e_sqlite3 in the directory alongside the test assembly.

What do I need to do so that net472 can find the native binary?

Minimal repro: SqliteTest.zip

ericsink commented 3 years ago

Unit test projects with .NET framework are always troublesome.

If you're talking about xunit, try adding an xunit.runner.json file that contains:

{
   "shadowCopy": false
}
AArnott commented 3 years ago

That's a good idea, but that didn't solve the problem. I checked the debugger to confirm the managed dll was loaded from its original location and that looked good. Since you mentioned thinking it was test project related, I added a console app to my solution. It has exactly the same problem.

Here is my updated solution: SqliteTest.zip

ericsink commented 3 years ago

Every release of SQLitePCLRaw is tested on net472 as part of the build process.

For example, here is one of the test programs I use. It should work for you as well:

csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net472</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="sqlitepclraw.bundle_e_sqlite3" Version="2.0.4" />
    <PackageReference Include="sqlitepclraw.ugly" Version="2.0.4" />
  </ItemGroup>

</Project>

cs:

using System;
using SQLitePCL.Ugly;

namespace smoke
{
    class Program
    {
        static void Main(string[] args)
        {
            SQLitePCL.Batteries_V2.Init();
            using (var db = ugly.open(":memory:"))
            {
                var s = db.query_scalar<string>("SELECT sqlite_version()");
                Console.WriteLine("{0}", s);
            }
        }
    }
}

I took a glance at your zip file. I'm not 100% sure what is different, but it might be the need to add the SQLitePCLRaw bundle package to the top-level console app instead of to the library.

AArnott commented 3 years ago

Ok, I see what's going on. Your sqlitepclraw.lib.e_sqlite3 package includes an msbuild import file that copies the e_sqlite3.dll file in several architectures a few directory levels deep into the output directory. So a top-level package reference in the console project or (preferably) a PrivateAssets="none" attribute added to the netstandard2.0 projects existing PackageReference fixes the problem, for both the console and test projects.

Now here's a potential opportunity to improve the packages though, because nuget is supposed to solve this for us, and this is why in my test and console projects I had these two extra properties:

    <RuntimeIdentifiers>win-x86;win-x64</RuntimeIdentifiers>
    <RuntimeIdentifier>win-x86</RuntimeIdentifier>

This allows my project to target either of two architectures, and because your nuget package correctly follows the runtimes folder pattern, the build would automatically place the appropriate native dll based on the architecture I am targeting at the time.

But the odd thing (which led to me filing this bug) was that although the native binary was placed, your managed code couldn't find it. Do you have managed code that explicitly looks for the binary in that runtimes folder structure that your SQLitePCLRaw.lib.e_sqlite3.targets file copies them into? If so, would you also consider finding the dll in the same folder as the managed dll, since that is how .NET Framework, .NET Core, and nuget/msbuild are designed to work by default?

ericsink commented 3 years ago

"since that is how .NET Framework, .NET Core, and nuget/msbuild are designed to work by default"

Are they? Where is that explained? That doesn't seem to be the way that .NET Core is designed to work by default. In fact, the current approach is an attempt to have .NET Framework emulate the way I understood things to work for .NET Core.

I have also had use cases where people needed both native libs to be present so they could switch back and forth between 32 and 64 bit builds. But that is a vague recollection from quite some time ago. This may not be currently relevant.

Bottom line: Your explanation sounds good, but this code has been in place for quite a few years, and I have learned to be very cautious of making an improvement which actually turns out to be a regression, especially in this particular area.

AArnott commented 3 years ago

Sure! Here are some docs for your review:

This one talks about the nuget runtimes folder pattern which you are already conforming to. But one point to glean from it is this paragraph:

Please note, NuGet always picks these compile or runtime assets from one folder so if there are some compatible assets from /ref then /lib will be ignored to add compile-time assemblies. Similarly, if there are some compatible assets from /runtimes then also /lib will be ignored for runtime.

Note that it talks about how NuGet picks files from one runtimes folder. You can see this in action by building the projects in my sample .zip.

Then if you look at dotnet build -r|--runtime parameter, you'll see how the build of even an "Any CPU" project can be built with either x86 or x64 binaries. Folks building "any cpu" libraries will not need to specify a runtime architecture, and will not deploy your native binary. But an application (or test) project that references that library (by project or package reference) will need to choose an architecture (as they always must unless they are a .NET Core FDD in which case they have a runtime config file which also tells .NET where to find all native binaries.

You'll also notice that .NET Framework and .NET Core automatically load native binaries from p/invoke signatures, and that Windows itself (via LoadLibrary) will automatically find the native binary when it is directly in the app directory and/or side-by-side the requesting assembly. And the runtime deps json file created for .NET Core apps further even allows finding the right native binary in other locations.

All this is to say Microsoft has up and down the runtime and build tools stack endeavored to make it easy. Your specialized library loading code defeats this. Downsides include:

  1. Extra msbuild imports are required, so in the simplest case of writing a library that consumes yours, the app that uses it will be broken till the lib author adds the missing attribute or the app adds an extra explicit dependency on yours.
  2. Larger payload for an app that gets 3 copies of the native binary when each app only needs one.
  3. Your msbuild import deploys native binaries based on the OS used to build the app instead of the OS that the app actually targets. This is probably the most serious defect. MSBuild and dotnet build are designed to allow targeting any OS from any OS.

I have also had use cases where people needed both native libs to be present so they could switch back and forth between 32 and 64 bit builds.

TBH, that actually works well for me too, since in fact I am building an (all dependencies included) extension for an app that may load my extension in either a 32-bit or 64-bit process. So I like that your library allows packing and loading multiple architectures. I just do not think that it should come at a cost of breaking the more common and Microsoft build chain-supported scenario.

At minimum, I think you should allow your native loader to find the native binary next to the managed assembly. That would un-break the intended scenario. Next, if you modify your .targets file to only copy the native binary around under the condition '$(RuntimeIdentifier)' == '' then you can continue supporting your existing scenarios but will avoid all the downsides listed above when folks are trying to use the Microsoft-offered happy path.

Thoughts?

ericsink commented 3 years ago

(Mostly content-free update)

So far I haven't actually taken the time to think hard about this issue. That targets file (and pretty much anything else that applies to .NET Framework but not .NET Core) is an area of the code I try to avoid thinking about.

That said, here is what has been going through my brain:

Since I don't like thinking about this targets file, I usually rely on advice from people at Microsoft to tell me if I got it right.

The current approach was designed with lots of input from people at Microsoft who seemed awfully smart. When that conversation ended, those people seemed happy with the result, so I haven't touched the targets file since then.

Now you have arrived, and you are also a person at Microsoft who seems awfully smart, and you're not happy.

Spock in Star Trek IV might wonder if this is a good time for a colorful metaphor.

That conversation with the previous round of awfully smart Microsofties was a long time ago. Maybe something changed?

It looks like I'm going to have to turn my brain on and actually think about this issue. But that's not happening until 2021. ;-)

AArnott commented 3 years ago

Maybe something changed?

I'd say that's likely, depending on how long ago we're talking about here. I can also ask a few MS folks to chime in here as well who are more direct area owners of the build toolset and runtimes so they can weigh in on my suggestions. Maybe some that I am thinking of are even among the set that helped you before.

Happy new year! 🥂

0xced commented 3 years ago

I also have experienced this dreaded Library e_sqlite3 not found exception in several occasions. Eric merged #364 recently which will at least ease diagnosing this error since all the searched paths will be included in the exception message.

I'm not sure if this applies to your scenario but I have figured out a way to never get this error by embedding the e_sqlite3.dll file into an executable with the help of Costura that I have documented on Stack Overflow: How to package a single-file executable using the SQLite EF Core Database Provider on .NET Framework?

That's a lot of work on the developer side but you can be 100% sure that the library will always load properly.

But all of this doesn't help with the issue at hand which is about not having ton of work to do. I'm not sure what's the best/correct way to fix this issue, but I'll just remind you of #252 which might be related.

ericsink commented 3 years ago

I finally got back to digging into this issue. This involved re-learning some of my own code which I hadn't looked at in a while.

The suggestion above from @AArnott was to "allow your native loader to find the native binary next to the managed assembly" and add a condition to the targets file.

But I started wondering what benefit my native loader is actually providing. See #419

So, as part of my exploration, I have implemented a more heavy-handed approach wherein (for .NET Framework) I eliminate my native loader (going back to dllimport) and remove the targets file. After that, adding the RuntimeIdentifiers and RuntimeIdentifier props gets my test suite running again.

ericsink commented 3 years ago

pre-release 2.0.5-pre20210521085756 has been pushed up to nuget, including the change described in #419 to default to use dllimport on .NET Framework.

AArnott commented 3 years ago

2.0.5-pre20210521085756 worked great.

ericsink commented 2 years ago

Fixed in 2.0.5

SteveDunn commented 2 years ago

Unit test projects with .NET framework are always troublesome.

If you're talking about xunit, try adding an xunit.runner.json file that contains:

{
   "shadowCopy": false
}

This worked for me. I was targetting net461 and various netcoreapp and net5/6

VincentLucc commented 1 year ago

Same issue here with 4.7.2. I fixed it by manually adding "e_sqlite3.dll" to the project and set to always copy this file. Copied from: packages\SQLitePCLRaw.lib.e_sqlite3.2.1.4\runtimes\win-x86\native\e_sqlite3.dll