Skip to content
This repository has been archived by the owner on Nov 1, 2018. It is now read-only.

Allow using an external version of sqlite3.dll #148

Closed
rowanmiller opened this issue Oct 5, 2015 · 55 comments
Closed

Allow using an external version of sqlite3.dll #148

rowanmiller opened this issue Oct 5, 2015 · 55 comments

Comments

@rowanmiller
Copy link
Contributor

We ship a version in the NuGet package, but we should provide a way to use a version that is obtained from elsewhere.

@Eilon
Copy link
Member

Eilon commented Oct 7, 2015

There might be some NuGet 3 features that could help with this, such as using Runtime IDs (rids). Should discuss with some NuGet experts because this might be super easy.

@natemcmaster
Copy link
Contributor

Related: we can start falling back to C:\Windows\System32\winsqlite3.dll (which ships in Win10 installations) if we are unable to load a nuget sqlite3.dll

@Eilon
Copy link
Member

Eilon commented Oct 7, 2015

Is that copy of SQLite meant for public consumption, or is it a private copy for Windows?

@natemcmaster
Copy link
Contributor

AFAIK yes. On my machine, it is SQLite 3.8.8.3, x64. It works with all of the tests in this project.

@natemcmaster
Copy link
Contributor

(as in yes, meant for public consumption)

@Eilon
Copy link
Member

Eilon commented Oct 7, 2015

Do you know if there's any official-looking statement of this? Or if not, can we contact the owners? I want to make sure if we allow it that there's some level of guarantee around it.

@natemcmaster
Copy link
Contributor

I couldn't find anything. I supposed its existence implied it's available for general use. I haven't tested against many versions of Windows, which is why I suggested only as a fallback. The fallback may not be necessary if we can get a good SQLite nuget package.

@davidfowl
Copy link
Member

I think we should just change this package to use DllImport. The runtime should set the system up in such a way that it will get the right one (architecture wise). For csproj based projects that copy to output would need to preserve the folder structure in the bin folder so that this works. CoreCLR based runtimes can read the project.lock.json and do the right thing.

Assuming that part was figured out, the user could just update to the latest SQLite package (assuming there is one) to get the latest bits. DllImport will also search the default search paths so I'm not sure you'd need to do anything else.

@bricelam
Copy link
Contributor

bricelam commented Oct 7, 2015

@davidfowl This issue is more about the user creating a custom build of SQLite, not just updating to the latest official release.

Also yes, we're already just using [DllImport]. We just happen to put the native bits in the same package at the moment for convenience.

@davidfowl
Copy link
Member

Custom build of SQLite.... ok 😄

@bricelam
Copy link
Contributor

bricelam commented Oct 7, 2015

lol, yeah... It's a common and "supported" scenario; see Compilation Options For SQLite for all the buttons and knobs.

@Eilon
Copy link
Member

Eilon commented Oct 7, 2015

@natemcmaster I definitely wouldn't recommend assuming that existence implies availability for general use. It has to be a documented feature to be available for general use. If we know what component drops it there we can contact that team and find out their usage policy.

@rowanmiller
Copy link
Contributor Author

@Eilon the Windows team has been working to have a local copy of SQLite available for use (and they have been asking us to ensure EF can make use of it). @natemcmaster not sure if you have confirmed that is the one they want us to use? My understanding was there would be an API we call to find the assembly to load.

@natemcmaster
Copy link
Contributor

@Eilon internal email powers FTW.
@rowanmiller I wasn't aware of plans other than I heard (in a rumor type way) that a Windows team was making a NuGet package for sqlite3 native binaries.

@divega
Copy link

divega commented May 19, 2016

With the work we are doing in #237 we will depend on a package containing sqlite3 native libraries appropriate for a wider set of platforms which should improve the default experience of using Microsoft.Data.Sqlite and Microsoft.EntityFrameworkCore.Sqlite.

However this issue remains unaddressed.

One possible way to address it would be to create "NoNative" variants of our packages. Possibly the "default" packages could become metapackages that depend on both the "NoNative" and the native distribution package.

We probably would need to address for RTM. As far as I understand we could refactor our packages in this way in the future without it being a breaking change.

@bricelam
Copy link
Contributor

bricelam commented Jun 2, 2016

I'm starting to think that Microsoft.Data.Sqlite shouldn't depend on sqlite. Instead, we can throw a friendly error at runtime pointing users to the sqlite package as one possible way of acquiring the library. We could even try to loading winsqlite3.dll on UWP if sqlite3.dll wasn't found.

@bricelam
Copy link
Contributor

bricelam commented Jun 2, 2016

The ASP.NET templates that use SQLite could even include a reference to the package by default.

@bricelam
Copy link
Contributor

bricelam commented Jun 2, 2016

BTW, falling back to winsqlite3 would make UWP like every other platform where, if you don't include an app-local version of sqlite3, it will just use the system's. We could even consider removing SqliteEngine.UseWinSqlite3().

@ali-hk
Copy link

ali-hk commented Jun 2, 2016

Just wanted to clarify some things about winsqlite:

  • It is officially sanctioned and intended to be used by UWPs as a means of reducing app package size. The following article outlines this: https://msdn.microsoft.com/en-us/windows/uwp/data-access/sqlite-databases
  • There is a major caveat to using winsqlite, which is the fact that it only exists in Windows 10 (10586) and above. This means Windows 10 RTM (10240) does NOT have winsqlite (you can confirm this by peeking in the system32 directory). As a result of this, it is not safe to fallback on winsqlite automatically as suggested by @bricelam. Using winsqlite should be an explicit opt-in by the developer.
  • Also, removing the option to opt-out of winsqlite precludes developers from using a custom, perhaps newer, build of sqlite if they so choose.

In my opinion @divega's approach is probably the best.

@bricelam
Copy link
Contributor

bricelam commented Jun 2, 2016

To be clear, we'd always try for sqlite3 first but if we can't find it, we'd fallback to winsqlite3 (and if we can't find that, we'd throw). Hence, to opt out of using winsqlite3, include a copy of sqlite3 with your app.

@bricelam
Copy link
Contributor

bricelam commented Jun 2, 2016

(But I understand your point that falling back may hide the fact that some users you deploy to may not actually have it.)

@divega
Copy link

divega commented Jul 29, 2016

That should in general work, although I think @natemcmaster is right that we would need something slightly different from what we had in the diagram in #148 (comment): the "sqlite3.dll" specific implementation of ISqlite3 would need to live somwhere. It seems the best place is Microsoft.Data.Sqlite, so Microsoft.EntityFrameworkCore.Sqlite will have to depend on Microsoft.Data.Sqlite instead of depending on the SQLite package directly (so maybe we just got an arrow wrong 😄).

FWIW, in conversations with the .NET Native folks it was also brough up that we could go low level with LoadPackagedLibrary() and GetProcAddress() instead of [DllImport(...)] to avoid the native compiler from hardcoding the native assembly import. Not sure if that could lead to a workable solution though.

@divega
Copy link

divega commented Oct 12, 2016

@bricelam are we still planning to do anything about this for 1.1.0-preview1 or are we going to rely on the switch to SQLitePCL.raw to take care of it at a later point?

@bricelam
Copy link
Contributor

The current plan is to switch to SQLitePCL.raw in 1.2.0 where they've already solved this problem.

@bricelam bricelam modified the milestones: 1.2.0, 1.1.0-preview1 Oct 12, 2016
@bricelam
Copy link
Contributor

We may need to adopt a similar packaging strategy as discussed above. SQLitePCL.raw has a ton of packaging variations, but I think we can simplify it to two:

  • SQLitePCLRaw.core--No native library included. The user picks sqlite3, sqlcipher, winsqlite3, etc.
  • SQLitePCLRaw.bundle_green--Just works™ everywhere

One thing I don't like about SQLitePCL.raw is that the user has to add something to their app's startup:

// For specific providers...
SQLitePCL.raw.SetProvider(new SQLitePCL.SQLite3Provider_winsqlite3());

// For bundles...
SQLitePCL.Batteries.Init();

We need to investigate if we can call this automatically or perhaps provider sugar APIs over...

@ericsink
Copy link

If you want to spare the user the Init call:

You should be able to just dep on bundle_whatever and call Batteries.Init()
in a static constructor. That's what most other libs are doing.

If your lib is PCL-ish, the bundle also needs to get added to the platform
project so the proper version of Batteries.Init() gets used. This is
usually done by telling the dev to add your lib to both their PCL-ish lib
and their platform project, which you made already need them to do for
other reasons.

I hope this makes sense. I'm happy to help figure out the right way to go
on this.

E

On Wed, Oct 12, 2016 at 11:45 AM, Brice Lambson [email protected]
wrote:

We may need to adopt a similar packaging strategy as discussed above
#148 (comment).
SQLitePCL.raw has a ton of packaging variations, but I think we can
simplify it to two:

  • SQLitePCLRaw.core--No native library included. The user picks
    sqlite3, sqlcipher, winsqlite3, etc.
  • SQLitePCLRaw.bundle_green--Just works™ everywhere

One thing I don't like about SQLitePCL.raw is that the user has to add
something to their app's startup:

// For specific providers...
SQLitePCL.raw.SetProvider(new SQLitePCL.SQLite3Provider_winsqlite3());
// For bundles...
SQLitePCL.Batteries.Init();

We need to investigate if we can call this automatically or perhaps
provider sugar APIs over...


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#148 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADsojYlUUIrj9VR8ejR51dQz9ORSBmdrks5qzQ6NgaJpZM4GJHtT
.

@bricelam
Copy link
Contributor

If our lib calls Batteries.Init(), but the user app doesn't use a bundle (because they pull in the version of our package that just depends on SQLitePCLRaw.core) will it throw or no-op?

I think our main goal is to make the default experience just work without having to initialize anything before opening a connection. If they have to do something special to use sqlcipher or winsqilte3, that's fine.

@bricelam
Copy link
Contributor

(Or would we not be able call Batteries.Init() if we don't reference a bundle?)

@ericsink
Copy link

Batteries.Init() is only present in the bundles, so you would have to dependon/reference it.

@ericsink
Copy link

I think our main goal is to make the default experience just work without having to initialize anything before opening a connection.

Then you probably want to depend on bundle_whatever and call Batteries.Init(). As long as your pkg is added to a platform project, this should work.

And I'm assuming that if you do this, you don't actually a separate version of the pkg that depends on just SQLitePCLRaw.core? Is that intended to support cases where somebody wants to swap out one bundle type for another?

@bricelam
Copy link
Contributor

bricelam commented Oct 12, 2016

Is that intended to support cases where somebody wants to swap out one bundle type for another?

Yes, eg. they want to use winsqlite3 or sqlcipher

@ericsink
Copy link

The problem with multiple versions of your package is that it forces other library packages to decide which one to take a dependency on.

And this is more or less how SQLitePCL.raw ended up in its current situation. I used to have separate packages for regular-sqlite and system-sqlite and sqlcipher and so on. But then library X would use SQLitePCL.raw, and it would have to choose just one of them, and then the users of X would ask me how to swap for a different one.

In your case, if I understand correctly, this problem would happen immediately. Which Microsoft.Data.Sqlite nupkg does EF want to use? Is it going to have two flavors of the package too?

FWIW, in part to create yet another option for dealing with these kinds of problems, I added Batteries_V2.Init(). It works the same way as Batteries.Init(), except that it lives in a different assembly name.

The assembly name for regular Batteries.Init() is different for every bundle pkg.

But Batteries_V2.Init() always lives in an assembly with the same name, regardless of the bundle.

This allows substituting one bundle type for another. The full explanation of how to do this can be less-than-intuitive, so I'm not saying this is an ideal solution. But it's another possibility. I have some users who really, really hate initialization calls.

BTW if I had realized earlier, I would have done the name of the batteries assembly this way from the beginning. But changing the name of that assembly would be a breaking change, so I just added a second version of the Init call. There should be no downside for someone to use the V2 version, whether they want the ability to substitute bundles across types or not.

@bricelam
Copy link
Contributor

bricelam commented Jan 11, 2017

I have some users who really, really hate initialization calls.

I think we're one of 'em. 😄

I suspect the best way to call Batteries_V2.Init() is using reflection? This would allow users to continue using a provider directly if they wanted...

Assembly batteriesAssembly = null;
try
{
    batteriesAssembly = Assembly.Load(
        new AssemblyName(
            "SQLitePCLRaw.batteries_v2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=8226ea5df37bcae9"));
}
catch
{
}

var batteriesType = batteriesAssembly?.GetType("SQLitePCL.Batteries_V2");
var initMethod = batteriesType?.GetTypeInfo().GetMethod("Init");

initMethod?.Invoke(null, null);

I think we'll continue with the plan of having metapackages for a good default experience.

image

Any libraries building on top of Microsoft.Data.Sqlite can choose what strategy they'd like to use: one version that depends directly on Microsoft.Data.Sqlite.NoNative and the app needs to bring a bundle/provider; or two versions of their package.

@ericsink
Copy link

Re: Assembly.Load() -- IIRC, this is one of those things that doesn't work well on iOS. And since UWP is also AOT, I would wonder if it might not work well there either?

@ericsink
Copy link

Re: the two-package solution:

Over the last few months, it looks like using bait-and-switch and calling Batteries_V2.Init() from a portable assembly is working out pretty well for other projects. The benefit of that solution is that you spare your users the init call, but you can stay with one package instead of two.

That said, I'm just trying to inform, not argue. :-)

@bricelam
Copy link
Contributor

Reflection will work on .NET Native, but we'll certainly have to test it on Xamarin.iOS.

Personally, I'm all for a one-package, auto-initializing solution if we can get it to display a nice message during build or run-time when a bundle isn't installed. Something like: A SQLitePCL.raw bundle isn't installed. Please install one (e.g. using the SQLitePCLRaw.bundle_green NuGet package) and try again.

But we'll re-discuss as a team.

@ericsink
Copy link

Minor quibble, just in case it matters: I wasn't broadly saying that reflection doesn't work on Xamarin.iOS. It does. I was specifically saying that I'm not sure Assembly.Load() works in all cases on Xamarin.iOS.

@ericsink
Copy link

"if we can get it to display a nice message"

I am happy to improve the error message and/or provide a way for packages to customize it.

@bricelam
Copy link
Contributor

After #320, using SQLCipher is as easy as:

Install-Package Microsoft.Data.Sqlite.Core
Install-Package SQLitePCLRaw.bundle_sqlcipher

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

8 participants