Use SQLitePCL.raw #320

bricelam · 2017-02-10T00:16:54Z

Changes:

Switches NuGet dependency from SQLite to SQLitePCLRaw.bundle_green
Splits into two packages:
- Microsoft.Data.Sqlite--metapackage includes ADO.NET provider and native implementation of SQLite
- Microsoft.Data.Sqlite.Core--just the ADO.NET provider (resolves Allow using an external version of sqlite3.dll #148, fixes Problems using winsqlite3.dll in UWP applications #249)
Removes SqliteEngine
Updates SqliteConnection and SqliteDataReader Handle property return type from IntPtr to the SQLitePCL.raw primitives
Stop normalizing relative paths (handled by SQLitePCL.raw)
Target .NET Standard 1.2 (resolves Support .NET Standard 1.2 (Windows 8.1 and Windows Phone 8.1) #185)
Bump minimum supported SQLite version to 3.7.15
Removes the use of ADONET_DATA_DIR (see Tools: Current Directory is wrong for .NET Core projects dotnet/efcore#7588)
Other minor cleanups including C# 7 usage

Other notes:

If a SQLitePCL.raw bundle is installed, Microsoft.Data.Sqlite.Core with auto-initialize it

Resolves #21, resolves #255

bricelam · 2017-02-10T02:49:48Z

src/Microsoft.Data.Sqlite/Microsoft.Data.Sqlite.csproj

-    <PackageTags>SQLite;Data;ADO.NET</PackageTags>
-    <CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)$(MSBuildProjectName).ruleset</CodeAnalysisRuleSet>
-    <IncludeSymbols>true</IncludeSymbols>
+    <Description>Metapackage that includes Microsoft.Data.Sqlite.Core and SQLitePCLRaw.bundle_green.</Description>


cc @rowanmiller (I wasn't sure what to put here.)

SQLite ADO.NET provider, packaged for most common use cases

Can we keep the description the same? The fact that it is a metapackage is an implementation detail that 99% of people won't care about.

In my view the term "metapackage" is a bit nerdy so I agree that's somewhat of an implementation detail. The most interesting things to put in the description are: 1. What is this thing? 2. Why do I need/want it?

Will keep the same. I debated doing that, but wanted emphasize that one includes SQLite while the other doesn't, but it probably doesn't actually matter.

ErikEJ · 2017-02-10T12:05:18Z

Wonder if the newer required runtime version will cause issue on non MS OSes - I recall a number of issues already with the current required version

bricelam · 2017-02-10T14:21:31Z

@ErikEJ It was a problem before we started distributing SQLite on OSX and Linux. You can still hit issues, but only if you step outside of the default bundle_green experience or are deploying to older iOS devices.

natemcmaster

Most of my comments are questions. I'm not familiar with SQlitePCL, but want to make sure we've done the due diligence to ensure it works in all the scenarios we currently support.

Also, cc @Eilon - this change introduces a new third-party dependency.

natemcmaster · 2017-02-10T17:12:28Z

Microsoft.Data.Sqlite.sln

 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
-		Debug|ARM = Debug|ARM
-		Debug|x64 = Debug|x64


👏 thanks for the cleanup.

natemcmaster · 2017-02-10T17:13:59Z

src/Microsoft.Data.Sqlite.Core/Microsoft.Data.Sqlite.Core.csproj

+    <AssemblyName>Microsoft.Data.Sqlite</AssemblyName>
+    <RootNamespace>$(AssemblyName)</RootNamespace>
+    <PackageId>$(MSBuildProjectName)</PackageId>
+    <Description>SQLite implementation of the System.Data.Common provider model.


I know you removed the Summary tag a cleanup commit, but consider re-adding it. NuGet will issue build warnings for long descriptions like this. In general, I'm in favor of eliminating warnings.

Nothing was handling <Summary>. NuGet doesn't support it... (I was getting the warning even with it.)

Oh, nvm. Weird...was sure NuGet supported that property.

FYI I've opened NuGet/Home#4587 as this long description will always produce a build warning.

natemcmaster · 2017-02-10T17:14:54Z

src/Microsoft.Data.Sqlite.Core/Microsoft.Data.Sqlite.Core.csproj

+      <PrivateAssets>All</PrivateAssets>
+    </PackageReference>
+    <PackageReference Include="System.Data.Common" Version="4.4.0-*" />
+    <PackageReference Include="System.ValueTuple" Version="4.3.0" />


Use 4.4.0-* of this too.

Does it exist? I was having problems when I tried...

Yes, it's on the https://dotnet.myget.org/feed/dotnet-core/package/nuget/System.ValueTuple. I updated our mirror settings to include this package. I manually added it to our feeds to unblock this.

Awesome, thank you. Tuple power! ✊

natemcmaster · 2017-02-10T17:15:14Z

src/Microsoft.Data.Sqlite.Core/Microsoft.Data.Sqlite.Core.csproj

+  </ItemGroup>
+  <ItemGroup>
+    <Compile Update="SqliteCommand.cs">
+      <SubType>Component</SubType>


I think I saw VS automatically add these...but what is it?

It makes a different icon show up in Solution Explorer 😄

It's because they inherit from Component on net451. I figured I'd stop fighting it and just check it in.

It's also a hint that there may be a designer associated with them.

That's annoying. Consider opening and issue on dotnet/roslyn-project-system.

natemcmaster · 2017-02-10T17:16:27Z

src/Microsoft.Data.Sqlite.Core/SqliteCommand.cs

                {
-                    stmts.Enqueue(Tuple.Create(stmt, rc != SQLITE_DONE));
+                    stmts.Enqueue((stmt, hasRows: rc != raw.SQLITE_DONE));


i ❤️ c# 7.0

lol, this feature is a bit hard to grok syntactically.

natemcmaster · 2017-02-10T17:25:41Z

src/Microsoft.Data.Sqlite/Interop/MarshalEx.cs

-{
-    internal static class MarshalEx
-    {
-        public static string PtrToStringUTF8(IntPtr ptr)


General question: does SQLitePCL default to UTF8 encoding of strings?

natemcmaster · 2017-02-10T17:26:31Z

src/Microsoft.Data.Sqlite/Interop/NativeMethods.cs

-                using (var connection = new SqliteConnection("Data Source=:memory:"))
-                {
-                    connection.Open();
-                    connection.ExecuteNonQuery("PRAGMA temp_store_directory = '" + ApplicationDataHelper.TemporaryFolderPath + "';");


This was required for UWP. Handled by SQLitePCL?

natemcmaster · 2017-02-10T17:27:49Z

src/Microsoft.Data.Sqlite/Microsoft.Data.Sqlite.csproj

-    <PackageTags>SQLite;Data;ADO.NET</PackageTags>
-    <CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)$(MSBuildProjectName).ruleset</CodeAnalysisRuleSet>
-    <IncludeSymbols>true</IncludeSymbols>
+    <Description>Metapackage that includes Microsoft.Data.Sqlite.Core and SQLitePCLRaw.bundle_green.</Description>


Can we keep the description the same? The fact that it is a metapackage is an implementation detail that 99% of people won't care about.

natemcmaster · 2017-02-10T17:29:24Z

src/Microsoft.Data.Sqlite/Microsoft.Data.Sqlite.csproj

-  <ItemGroup>
-    <PackageReference Include="SQLite" Version="3.12.3" />
-    <PackageReference Include="StyleCop.Analyzers" Version="1.0.0">
+    <PackageReference Include="SQLitePCLRaw.bundle_green" Version="1.1.2" />


Set PrivateAssets=None. SQLitePCLRaw.bundle_green has some build targets that should be included. Cref NuGet/Home#4521

natemcmaster · 2017-02-10T17:30:37Z

src/Microsoft.Data.Sqlite/Utilities/NativeLibraryLoader.cs

-                foreach (var path in privateBinPath.Split(new [] { Path.PathSeparator }, StringSplitOptions.RemoveEmptyEntries))
-                {
-                    yield return path;
-                    yield return Path.Combine(path, GetArchitecture());


How does SQLitePCL handle AnyCPU desktop apps?

It has some code to try and resolve things at runtime. In sqlite3_pinvoke.cs, look for PRELOAD_FROM_ARCH_DIRECTORY. I can't claim that code is perfect, but it is generally working for people.

Looks like the same way we did: x86 and x64 directories + LoadLibraryEx.

I'm trying to navigate the layers of SQLitePCL packages. I'm sure this handled, but can you point me to which package actually has the native windows x64 vs x86 binaries? What I'm trying to make sure we've addressed this this: dotnet/sdk#847. Microsoft.NET.Sdk will not require .NET Framework apps to specify a runtime identifier. But if the package is using the 'runtimes' folder of NuGet to distribute win-x86 vs win-x64 binaries, we may run into issues.

I think you're looking for SQLitePCLRaw.lib.e_sqlite3.v110_xp

Have you tried this package with VS 2017 and the .NET Core SDK? I suspect you are going to run into this issue: NuGet/Home#4432. Basically, because the targets file adds the library as 'Content', it will end up in nuget packages. You can probably make a simple change to fix this. Set <Pack>false</Pack> on the Content items. You may also want to set CopyToPublishDirectory in addition to output directory.

No, I have not yet tested with VS 2017. Until a couple of days ago when the March release date was announced, I thought I had plenty of time. :-)

Thanks for the tip. I will investigate.

Eilon · 2017-02-10T17:39:33Z

@natemcmaster re: new 3rd party dependency. Yup, already noted and we filed the paperwork with the paperwork people 😄

ericsink · 2017-02-10T19:11:16Z

FYI about bundle_green:

I currently have three bundles:

bundle_e_sqlite3 (always uses my e_sqlite3 builds)
bundle_sqlcipher (uses sqlcipher builds, which I currently get from couchbaselabs/couchbase-lite-libsqlcipher )
bundle_green

Basically, bundle_green means "Use the e_sqlite3 builds everywhere except iOS. On iOS, use the SQLite provided by the system."

In other words, it is identical to bundle_e_sqlite3, except on iOS.

Why is it called "green"? Because it provides the behavior that Frank Krueger aka @praeclarum wanted for sqlite-net-pcl, and it didn't seem right to call it bundle_frank, and bundle_sqlite_net is confusing, and bundle_use_e_sqlite3_everywhere_except_ios is too long, and Frank is from the pacific northwest where there are lots of trees, and trees are mostly green, and I had already spent too much time deciding on the name, so I just settled it and moved on.

Anyway, people like bundle_green, except one issue tends to come up. My e_sqlite3 builds are compiled with SQLite's default setting for thread safety, but the iOS system SQLite is compiled with a different setting. The main difference is that the iOS setting means that a sqlite3 connection handle cannot be shared across threads. Every so often, somebody has trouble because their app is giving different behavior on iOS than on other platforms.

Luckily, the thread safety setting can be changed at runtime, using sqlite3_config(). So, people can eliminate the inconsistency by changing the setting to one way or the other. However, sqlite3_config() needs to be called before any other sqlite function, and this is why those sqlite3_win32_set_directory calls in sqlite3_invoke.cs (see related comments elsewhere on this PR) are problematic. Currently, with SQLitePCL.raw on UWP, to call sqlite3_config, you have to (carefully) do a sqlite3_shutdown and then call sqlite3_config and then call sqlite3_initialize. Kinda messy, but it works, and only UWP has this problem.

Anyway, I tend to use bundle_e_sqlite3 in my own apps. Having consistent compile settings for the SQLite library seems more important to me than making the size of the iOS app smaller by using the system SQLite on that platform.

Anyway, if you stay with bundle_green, you may want to add the necessary sqlite3_config() calls to keep the thread safety behavior consistent across all platforms. OTOH, if you never share a sqlite3 connection handle across threads, those config calls probably are not necessary.

Switching to bundle_e_sqlite3 is an option as well.

Finally, it is worth noting that I would be happy to add (and maintain) another bundle which is customized for your requirements. Suppose you decide you want the same compile options on all platforms (like e_sqlite3), but you want compile options that are different from e_sqlite3. For example, I build e_sqlite3 with support for FTS, RTREE, and json. The builds would be smaller if any of those were removed.

bricelam · 2017-02-10T21:22:52Z

@ericsink TBH, the only reason I picked SQLitePCLRaw.bundle_green is because Microsoft.Azure.Mobile.Client.SQLiteStore uses it, and I didn't want to create an ecosystem where mixing different SQLite technologies in the same project created NuGet package hell. 😈

For better or worse, it appears to have become the de facto bundle for library authors. Our saving grace is that we also let you bring your own bundle by dropping down to the Microsoft.Data.Sqlite.Core package.

Thanks for the heads up about multi-threading on iOS we saw similar issues on Mac before we started re-distributing SQLite there.

ericsink · 2017-02-10T21:31:00Z

@bricelam Makes sense. I believe I talked with them about the tradeoffs as well. I just want folks to be informed.

natemcmaster · 2017-02-13T18:18:51Z

@ericsink what is your release timeline like for updates? I'm hesitant to sign off on this change now if we can't get a fix for ericsink/SQLitePCL.raw#140 (comment) before our next release.

The issue means anyone with VS 2017 and a class library referencing Microsoft.Data.Sqlite will end up with sqlite3.dll in their NuGet package when they call "dotnet pack".

ericsink · 2017-02-13T18:59:22Z

@natemcmaster I've been traveling, but this will be a priority when I get back in the office (tomorrow-ish). I would expect this to be days, or worst case, weeks.

When is your next release? I've had the impression it was still months away. Your concern makes me think I am misunderstanding something.

natemcmaster · 2017-02-13T19:01:33Z

days, or worst case, weeks.

That's what I was hoping for, but wanted to double check. In that case, we will be fine. Our next release will be after that.

natemcmaster

We will probably need to bump to a new version of bundle_green with fixes to make it work better with Microsoft.NET.Sdk. Let's add an issue to track make sure we handle that before we release 2.0.

Otherwise, looks good. Let's get this in so we can get tests going.

One more bookkeeping thing: please update packages.csv in aspnet/Coherence before merging.

Changes: * Switches NuGet dependency from SQLite to SQLitePCLRaw.bundle_green * Splits into two packages: * Microsoft.Data.Sqlite--metapackage includes ADO.NET provider and native implementation of SQLite * Microsoft.Data.Sqlite.Core--just the ADO.NET provider (resolves #148, fixes #249) * Removes SqliteEngine * Updates SqliteConnection and SqliteDataReader Handle property return type from IntPtr to the SQLitePCL.raw primitives * Stop normalizing relative paths (handled by SQLitePCL.raw) * Target .NET Standard 1.2 (resolves #185) * Bump minimum supported SQLite version to 3.7.15 * Other minor cleanups including C# 7 usage Other notes: * If a SQLitePCL.raw bundle is installed, Microsoft.Data.Sqlite.Core with auto-initialize it Resolves #21, resolves #255

dnfclas added the cla-already-signed label Feb 10, 2017

bricelam commented Feb 10, 2017

View reviewed changes

bricelam requested a review from natemcmaster February 10, 2017 05:02

This was referenced Feb 10, 2017

Allow using an external version of sqlite3.dll #148

Closed

Problems using winsqlite3.dll in UWP applications #249

Closed

natemcmaster reviewed Feb 10, 2017

View reviewed changes

ericsink mentioned this pull request Feb 10, 2017

Need <pack>false</pack> ericsink/SQLitePCL.raw#140

Closed

bricelam mentioned this pull request Feb 10, 2017

App_Data no longer used on ASP.NET #321

Closed

natemcmaster approved these changes Feb 13, 2017

View reviewed changes

bricelam merged commit 3e40a9d into aspnet:dev Feb 14, 2017

bricelam deleted the raw branch February 14, 2017 16:16

Use SQLitePCL.raw #320

Use SQLitePCL.raw #320

Uh oh!

Conversation

bricelam commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bricelam Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikEJ commented Feb 10, 2017

Uh oh!

bricelam commented Feb 10, 2017

Uh oh!

natemcmaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bricelam commented Feb 10, 2017 •

edited

Loading

bricelam Feb 10, 2017 •

edited

Loading

bricelam commented Feb 10, 2017 •

edited

Loading