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

Resolve problem with sqlite3_win32_set_directory for wasm #477

Closed ericsink closed 2 years ago

ericsink commented 2 years ago

See dotnet/aspnetcore#39825 for more info.

The problem is that sqlite3_win32_set_directory() only appears in the native SQLite library when compiled for Windows.

In the case of a typical DllImport, this doesn't cause much trouble, because it results in a failure at runtime.

In AOT scenarios, the situation is worse, since the symbol is missing, so it fails at build time.

In provider_internal, which we assume is iOS-specific, and is therefore an AOT scenario, we have a special flag we use when generating that provider to make sure the actual native function is not referenced, so we don't get a build-time failure.

Wasm is a similar situation.

There are basically two ways we could fix this:

  1. Use something similar to what we do with provider_internal, creating a special build of provider_e_sqlite3 and provider_e_sqlcipher (any others) which do what provider_internal does. This approach would require us to handle RID-specific builds of those providers.
  2. Use something similar to what we do for sqlite3_key and friends, putting instances of those functions in a stubs.c file in the native build (over in the cb repo).
ericsink commented 2 years ago

The unfortunate thing about RID-specific builds of provider_e_sqlite3 is that I recently just changed all the provider builds to use multi-targeting and to no longer need a separate nuspec file. This simplified the tree quite a bit, and it seems like it would be SO SAD to undo all that.

What we would need is to say "build this assembly twice for the same TFM (which multi-targeting can't do), and when creating the nuspec for pack, put one of them in the usual place in lib/TFM, and put the other one in the RID-specific area". There is, AFAICT, no way to express what we want in csproj properties for use with dotnet pack without a separate nuspec.

ericsink commented 2 years ago

@bricelam Over in the other issue, you said:

The RID-specific assembly is more work, but would probably be the better option.

Initially I agreed with that, but now I want to push on it a little bit.

So, er, WHY would it probably be the better option?

I wish to highlight the fact that both approaches accomplish the same result, which is to make sure that sqlite3_win32_set_directory() returns an error code. Either way, that function is a member of ISQLite3Provider. We're just deciding whether we want SQLITE_ERROR to be returned in C# or C.

And right now, doing the work in stubs.c is a heckuva lot simpler.

One big unknown here is the question of whether I will eventually need RID-specific assemblies anyway. If the pain is coming no matter what, there may be less incentive to avoid it now. OTOH, I suppose maybe stalling might pay off if "dotnet pack without nuspec" later gains the ability handle more advanced scenarios.

bricelam commented 2 years ago

The only real advantage I see is that users may want to bring their own custom build of SQLite on Wasm. If you go with the stubs.c approach, they'll just have to know to include that in their custom builds. But I can't imagine that being very common, so it's probably OK to take that approach too.

I also see a world where AoT becomes more and more popular. For example, creating an AoT version of your ASP.NET Core application to deploy and run inside Docker. But that's a bit forward thinking, and we can always solve any problems with it as they arrise.

ericsink commented 2 years ago

Good observations. I'll think about this a bit more.

peerem commented 2 years ago

To get it work pls. add this line to your project:

<EmccExtraLDFlags>-s ERROR_ON_UNDEFINED_SYMBOLS=0</EmccExtraLDFlags>

See also here: https://github.com/ericsink/SQLitePCL.raw/issues/472#issuecomment-1046038711

ericsink commented 2 years ago

To borrow another quote from @bricelam over in the issue where this started:

We could even reverse it and make the one with sqlite3_win32_set_directory specific to windows.

I do think the best way forward is likely to go back to treating Windows as the special case here. I mean, not only is this API call specific to Windows, the most common use case is UWP, which is an even more specialized case.

I recently changed the providers to build with multi-targeting, and I hope to preserve that.

In that transition I also changed to use dotnet pack without a side nuspec file, and I'd like to keep that too. Earlier today I made a commit which demonstrates a way to use an advanced feature of dotnet pack to put an assembly in runtimes/rid/etc/.

For the 4 main DllImport providers (e_sqlite3, e_sqlcipher, sqlite3, sqlcipher,), there are basically two switches we can turn on and off when we generate the C# code for that provider:

  1. Whether to use (funcptrs) C# 9 function pointers or (prenet5) the older way, delegates with UnmanagedFunctionPointerAttribute.
  2. Whether sqlite3_win32_set_directory should (win32dir) try to call the native code or (notwin) just return an error.

(funcptrs) requires net5.0 or higher.

(win32dir) is only useful on Windows. On non-Windows, non-AOT platforms, it has apparently been harmless, simply returning an error at runtime if something happens to call it. But it actually causes problems on any non-Windows platform that uses AOT, because the missing function will cause a linker failure.

As part of the recent changes, I am generating two versions for each of those 4 providers:

  1. A version called "most", which has (prenet5) and (win32dir), and which is built for the TFM netstandard2.0
  2. A version call "funcptrs", which has (funcptrs) and (win32dir), and which is built for the TFM net6.0

In other words, almost all providers include (win32dir). The only provider currently compiled for (notwin) is the __Internal provider, since it is for iOS which is AOT.

So the approach I am investigating is to change all these providers to (notwin), and then add back special cases with (win32dir) where needed.

Hopefully while keeping multi-targeting and dotnet pack without side nuspec, and hopefully not mixing up UWP and other kinds of Windows like I did in 2.0.7 (see discussion in #465).

Details yet to be figured out.

ericsink commented 2 years ago

It is worth noting that the winsqlite3 provider is separate, and it is generated with support for sqlite3_win32_set_directory.

ericsink commented 2 years ago

New theory:

In which cases do we actually need to target netstandard2.0?

UWP currently needs it, as we don't multi-target uap10.0, because targeting that in an SDK-style project is not officially supported. This would suggest that we need netstandard2.0 to include (win32dir). What problems would that cause?

.NET Framework support also currently uses netstandard2.0, although it wouldn't have to do so, as we could explicitly target net461 or net472. But it also should be harmless to include (win32dir) for those cases. .NET Framework is always Windows, except when it's actually something like mono, but those cases do not typically use AOT.

Classic pre-net6.0 Xamarin currently would match netstandard2.0, but we could multi-target to avoid that and give those cases a custom provider. That won't matter for e_sqlite3, which is not supported on iOS, but does it matter for the sqlite3 provider? Probably not, since even though the provider is AOT-compiled, that cases uses dynamic linking to the system SQLite, so it won't fail static linking like the __internal AOT case.

peerem commented 2 years ago

Now it works without the "-s ERROR_ON_UNDEFINED_SYMBOLS=0" switch, thank you. One final warning remains:

Warning : Found a native function (sqlite3_config) with varargs in e_sqlcipher. Calling such functions is not supported, and will fail at runtime. Managed DllImports: 
    System.Int32 sqlite3_config_none(System.Int32) (in [SQLitePCLRaw.provider.e_sqlcipher] SQLitePCL.SQLite3Provider_e_sqlcipher+NativeMethods)
    System.Int32 sqlite3_config_int(System.Int32, System.Int32) (in [SQLitePCLRaw.provider.e_sqlcipher] SQLitePCL.SQLite3Provider_e_sqlcipher+NativeMethods)
    System.Int32 sqlite3_config_int_arm64cc(System.Int32, System.IntPtr, System.IntPtr, System.IntPtr, System.IntPtr, System.IntPtr, System.IntPtr, System.IntPtr, System.Int32) (in [SQLitePCLRaw.provider.e_sqlcipher] SQLitePCL.SQLite3Provider_e_sqlcipher+NativeMethods)
    System.Int32 sqlite3_config_log(System.Int32, System.IntPtr, SQLitePCL.hook_handle) (in [SQLitePCLRaw.provider.e_sqlcipher] SQLitePCL.SQLite3Provider_e_sqlcipher+NativeMethods)
    System.Int32 sqlite3_config_log_arm64cc(System.Int32, System.IntPtr, System.IntPtr, System.IntPtr, System.IntPtr, System.IntPtr, System.IntPtr, System.IntPtr, System.IntPtr, SQLitePCL.hook_handle) (in [SQLitePCLRaw.provider.e_sqlcipher] SQLitePCL.SQLite3Provider_e_sqlcipher+NativeMethods)
ericsink commented 2 years ago

The fix for this is in 2.1.0, which has now been released.