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

NativeAOT/trimming compatibility for Microsoft.Data.Sqlite #29725

Closed
roji opened this issue Dec 1, 2022 · 14 comments · Fixed by dotnet/runtime#79963 or #30728
Closed

NativeAOT/trimming compatibility for Microsoft.Data.Sqlite #29725

roji opened this issue Dec 1, 2022 · 14 comments · Fixed by dotnet/runtime#79963 or #30728
Labels
area-adonet-sqlite area-aot closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@roji
Copy link
Member

roji commented Dec 1, 2022

Raised by @DamianEdwards:

I've tried Sqlite in a console app and it produces a trimming warning but works, but fails when NativeAOTed :

image

@ajcvickers ajcvickers changed the title NativeAOT/trimming compatibility for Microsoft.Data.SqlClient NativeAOT/trimming compatibility for Microsoft.Data.Sqlite Dec 1, 2022
@bricelam
Copy link
Contributor

bricelam commented Dec 1, 2022

Super surprised this is being trimmed out. Was this with the latest version?

static SqliteConnection()
{
Type.GetType("SQLitePCL.Batteries_V2, SQLitePCLRaw.batteries_v2")
?.GetRuntimeMethod("Init", Type.EmptyTypes)
?.Invoke(null, null);

@DamianEdwards
Copy link
Member

@roji roji mentioned this issue Dec 4, 2022
38 tasks
@ajcvickers ajcvickers added this to the 8.0.0 milestone Dec 5, 2022
kant2002 added a commit to kant2002/runtime that referenced this issue Dec 25, 2022
Simplified type name parsing was breaking if full name or assembly name has underscode ('_') in it. That breaks referencing `SQLitePCL.Batteries_V2, SQLitePCLRaw.batteries_v2` type inside `Microsoft.Data.Sqlite`

Fixes dotnet/efcore#29725
@kant2002
Copy link

I think that's issue on NativeAOT side with parsing of the typenames in the Type.GetType() calls

@roji roji removed this from the 8.0.0 milestone Dec 26, 2022
@roji roji reopened this Dec 26, 2022
@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Dec 26, 2022
@DamianEdwards
Copy link
Member

Confirmed I can compile it for native AOT and successfully run it now! I am still getting a trim warning from Microsoft.Data.Sqlite though:

dotnet restore .\src\TrimmedTodo.Console.Sqlite\TrimmedTodo.Console.Sqlite.csproj -r win10-x64 -p:Configuration=Release
  Determining projects to restore...
  All projects are up-to-date for restore.
dotnet clean .\src\TrimmedTodo.Console.Sqlite\TrimmedTodo.Console.Sqlite.csproj -c Debug -r win10-x64 -v q --nologo -o  .artifacts\TrimmedTodo.Console.Sqlite
dotnet clean .\src\TrimmedTodo.Console.Sqlite\TrimmedTodo.Console.Sqlite.csproj -c Release -r win10-x64 -v q --nologo -o  .artifacts\TrimmedTodo.Console.Sqlite
Publishing TrimmedTodo.Console.Sqlite: dotnet publish -r win10-x64 --self-contained
  Determining projects to restore...
  All projects are up-to-date for restore.
C:\Program Files\dotnet\sdk\8.0.100-alpha.1.23076.1\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInfer
ence.targets(287,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-po
licy [C:\src\GitHub\DamianEdwards\TrimmedTodo\src\TrimmedTodo.Console.Sqlite\TrimmedTodo.Console.Sqlite.csproj]
  TrimmedTodo.Console.Sqlite -> C:\src\GitHub\DamianEdwards\TrimmedTodo\src\TrimmedTodo.Console.Sqlite\bin\Release\net8
  .0\win10-x64\TrimmedTodo.Console.Sqlite.dll
  Generating native code
C:\Users\damia\.nuget\packages\microsoft.data.sqlite.core\8.0.0-alpha.1.23076.2\lib\net6.0\Microsoft.Data.Sqlite.dll :
warning IL2104: Assembly 'Microsoft.Data.Sqlite' produced trim warnings. For more information see https://aka.ms/dotnet
-illink/libraries [C:\src\GitHub\DamianEdwards\TrimmedTodo\src\TrimmedTodo.Console.Sqlite\TrimmedTodo.Console.Sqlite.cs
proj]
  TrimmedTodo.Console.Sqlite -> C:\src\GitHub\DamianEdwards\TrimmedTodo\.artifacts\TrimmedTodo.Console.Sqlite\

App was published as a single 4.41 MB file (TrimmedTodo.Console.Sqlite.exe)

App last access time: 01/26/2023 15:02:54

Running TrimmedTodo.Console.Sqlite

Ensuring database exists and is up to date at connection string 'Data Source=todos.db;Cache=Shared'

There are currently no todos!

Added todo 1
Added todo 2
Added todo 3

Id Title
----------------------
1  Do the groceries
2  Give the dog a bath
3  Wash the car

Todo 'Wash the car' completed!

Id Title
----------------------
1  Do the groceries
2  Give the dog a bath
3  Wash the car

Deleted all 3 todos!

@tkapin
Copy link
Member

tkapin commented Feb 1, 2023

@DamianEdwards - I think this has been accidentally closed by GH. I was syncing my runtime fork and the abovementioned commit contains "Fixes <this_issue>" in the description, which seems to have caused this issue to get closed. Thanks for catching it and reopening the issue.

@DamianEdwards
Copy link
Member

@tkapin yep no problem, I figured it was something like that 😄

@DamianEdwards
Copy link
Member

Any ETA for fixing the remaining warnings? Here is what I get right now with preview.3 bits (source project):

ILC : Trim analysis warning IL2093: Microsoft.Data.Sqlite.SqliteDataReader.GetFieldType(Int32): 'DynamicallyAccessedMemberTypes' in
'DynamicallyAccessedMembersAttribute' on the return value of method 'Microsoft.Data.Sqlite.SqliteDataReader.GetFieldType(Int32)' don
't match overridden return value of method 'System.Data.Common.DbDataReader.GetFieldType(Int32)'. All overridden members must have t
he same 'DynamicallyAccessedMembersAttribute' usage. [C:\src\GitHub\DamianEdwards\Nanorm\samples\Nanorm.Samples.Console.Todos.Sqlite
\Nanorm.Samples.Console.Todos.Sqlite.csproj::TargetFramework=net8.0]

ILC : Trim analysis warning IL2072: Microsoft.Data.Sqlite.SqliteConnection..cctor(): 'name' argument does not satisfy 'DynamicallyAc
cessedMemberTypes.PublicProperties' in call to 'System.Reflection.RuntimeReflectionExtensions.GetRuntimeProperty(Type,String)'. The
return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same
 requirements as those declared on the target location it is assigned to. [C:\src\GitHub\DamianEdwards\Nanorm\samples\Nanorm.Samples
.Console.Todos.Sqlite\Nanorm.Samples.Console.Todos.Sqlite.csproj::TargetFramework=net8.0]

ILC : Trim analysis warning IL2072: Microsoft.Data.Sqlite.SqliteConnection..cctor(): 'name' argument does not satisfy 'DynamicallyAc
cessedMemberTypes.PublicProperties' in call to 'System.Reflection.RuntimeReflectionExtensions.GetRuntimeProperty(Type,String)'. The
return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same
 requirements as those declared on the target location it is assigned to. [C:\src\GitHub\DamianEdwards\Nanorm\samples\Nanorm.Samples
.Console.Todos.Sqlite\Nanorm.Samples.Console.Todos.Sqlite.csproj::TargetFramework=net8.0]

ILC : Trim analysis warning IL2072: Microsoft.Data.Sqlite.SqliteConnection..cctor(): 'name' argument does not satisfy 'DynamicallyAc
cessedMemberTypes.PublicProperties' in call to 'System.Reflection.RuntimeReflectionExtensions.GetRuntimeProperty(Type,String)'. The
return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same
 requirements as those declared on the target location it is assigned to. [C:\src\GitHub\DamianEdwards\Nanorm\samples\Nanorm.Samples
.Console.Todos.Sqlite\Nanorm.Samples.Console.Todos.Sqlite.csproj::TargetFramework=net8.0]

ILC : Trim analysis warning IL2072: Microsoft.Data.Sqlite.SqliteConnection..cctor(): 'name' argument does not satisfy 'DynamicallyAc
cessedMemberTypes.PublicProperties' in call to 'System.Reflection.RuntimeReflectionExtensions.GetRuntimeProperty(Type,String)'. The
return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same
 requirements as those declared on the target location it is assigned to. [C:\src\GitHub\DamianEdwards\Nanorm\samples\Nanorm.Samples
.Console.Todos.Sqlite\Nanorm.Samples.Console.Todos.Sqlite.csproj::TargetFramework=net8.0]

@ajcvickers ajcvickers removed this from the 8.0.0 milestone Apr 17, 2023
@ajcvickers
Copy link
Contributor

The problematic code is below. The calls to GetType and the subsequent calls to GetRuntimeMethod and GetRuntimeProperty are being flagged because the types and/or members may not be available. However, I believe this code is only needed if the application has not explicitly called Batteries_V2.Init(), which is reasonable for an trimmed application to do. The problem is how to annotate/change the code to support this? That is, to build without warnings, but throw at runtime if Batteries_V2.Init() has not been called? Any thoughts? @bricelam @roji @DamianEdwards

static SqliteConnection()
{
    Type.GetType("SQLitePCL.Batteries_V2, SQLitePCLRaw.batteries_v2")
        ?.GetRuntimeMethod("Init", Type.EmptyTypes)
        ?.Invoke(null, null);

    try
    {
        var currentAppData = Type.GetType("Windows.Storage.ApplicationData, Windows, ContentType=WindowsRuntime")
            ?? Type.GetType("Windows.Storage.ApplicationData, Microsoft.Windows.SDK.NET")
            ?.GetRuntimeProperty("Current")?.GetValue(null);

        var localFolder = currentAppData?.GetType()
            .GetRuntimeProperty("LocalFolder")?.GetValue(currentAppData);
        var localFolderPath = (string?)localFolder?.GetType()
            .GetRuntimeProperty("Path")?.GetValue(localFolder);
        if (localFolderPath != null)
        {
            var rc = sqlite3_win32_set_directory(SQLITE_WIN32_DATA_DIRECTORY_TYPE, localFolderPath);
            Debug.Assert(rc == SQLITE_OK);
        }

        var tempFolder = currentAppData?.GetType()
            .GetRuntimeProperty("TemporaryFolder")?.GetValue(currentAppData);
        var tempFolderPath = (string?)tempFolder?.GetType()
            .GetRuntimeProperty("Path")?.GetValue(tempFolder);
        if (tempFolderPath != null)
        {
            var rc = sqlite3_win32_set_directory(SQLITE_WIN32_TEMP_DIRECTORY_TYPE, tempFolderPath);
            Debug.Assert(rc == SQLITE_OK);
        }
    }
    catch
    {
        // Ignore "The process has no package identity."
    }
}

@roji
Copy link
Member Author

roji commented Apr 18, 2023

For Windows.Storage.ApplicationData, is this just for getting the AppData directory on Windows? If so, can this simply be replaced with Environment.SpecialFolder.ApplicationData?

For SQLitePCL.Batteries_V2, maybe [DynamicDependency] is a good approach here? If I understand correctly, this code calls Init() on that type if it's already somehow there (i.e. the user referenced the nuget), if so, that should work I think...

@kant2002
Copy link

private bool manualInitialization = false;
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2063",
        Justification = "for trimming scenarions we recommend run SQLitePCL.Batteries_V2.Init() manually.")]
static SqliteConnection()
{
   bool initialized = false;
   try
   {
        initialized = SQLitePCL.raw.Provider != null;
   }
   catch {}

   if (manualInitialization && !initialized) {
      throw new InvalidOperationException("For timming application you should manually perform Sqlite initializtion. Call SQLitePCL.Batteries_V2.Init(). Go to https://ask.ms/sqlite-trimming");
   }

   RuntimeInitialization();
}

static void RuntimeInitialization()
{
    Type.GetType("SQLitePCL.Batteries_V2, SQLitePCLRaw.batteries_v2")
        ?.GetRuntimeMethod("Init", Type.EmptyTypes)
        ?.Invoke(null, null);

    PrepareWinRTApps();
}
public static void PrepareWinRTApps()
{
    try
    {
        var currentAppData = Type.GetType("Windows.Storage.ApplicationData, Windows, ContentType=WindowsRuntime")
            ?? Type.GetType("Windows.Storage.ApplicationData, Microsoft.Windows.SDK.NET")
            ?.GetRuntimeProperty("Current")?.GetValue(null);

        var localFolder = currentAppData?.GetType()
            .GetRuntimeProperty("LocalFolder")?.GetValue(currentAppData);
        var localFolderPath = (string?)localFolder?.GetType()
            .GetRuntimeProperty("Path")?.GetValue(localFolder);
        if (localFolderPath != null)
        {
            var rc = sqlite3_win32_set_directory(SQLITE_WIN32_DATA_DIRECTORY_TYPE, localFolderPath);
            Debug.Assert(rc == SQLITE_OK);
        }

        var tempFolder = currentAppData?.GetType()
            .GetRuntimeProperty("TemporaryFolder")?.GetValue(currentAppData);
        var tempFolderPath = (string?)tempFolder?.GetType()
            .GetRuntimeProperty("Path")?.GetValue(tempFolder);
        if (tempFolderPath != null)
        {
            var rc = sqlite3_win32_set_directory(SQLITE_WIN32_TEMP_DIRECTORY_TYPE, tempFolderPath);
            Debug.Assert(rc == SQLITE_OK);
        }
    }
    catch
    {
        // Ignore "The process has no package identity."
    }
}

And then provide ILLink.Substitutions.xml embedded in assembly.

<linker>
  <assembly fullname="Microsoft.Data.Sqlite">
    <type fullname="Microsoft.Data.Sqlite.SqliteConnection">
      <field name="manualInitialization" value="true" initialize="true" />
    </type>
  </assembly>
</linker>

Missing part about Windows.Storage.ApplicationData also should be manually initialized by developer by calling to SqliteConnection.PrepareWinRTApps() all these manual steps can be explained in some document. That's not ideal, but

  • no warnings
  • experience more or less tolerable (at least developer have guidance)

what do you think ?

@roji
Copy link
Member Author

roji commented Apr 18, 2023

@kant2002 if this can be made to actually work with [DynamicDependency] that would be better no?

@kant2002
Copy link

@roji obviously, that would be perfect solution. I just worry about that if somebody would not use batteries_v2 nuget at all and manually setup provider. That way I suspect(nevery use that combination) that dependency resolution may fail. ILC for example do not like types which cannot be resolved statically and throws.

maybe better just try it out and check and not speculate about it.

@ajcvickers
Copy link
Contributor

Ignore my comments above. Was totally not understanding the issues. PR coming soon.

@ajcvickers ajcvickers assigned ajcvickers and unassigned bricelam Apr 18, 2023
@ajcvickers ajcvickers added this to the 8.0.0-preview4 milestone Apr 18, 2023
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Apr 18, 2023
ajcvickers added a commit that referenced this issue Apr 18, 2023
Fixes #29725

There were two types of issue:

- In SqliteConnection, the linker was unable to determine the type returned by the `GetType` call. Fixed by explicitly specifying the type.
- In SqliteDataReader, `GetFieldType` on the base has been annotated with `DynamicallyAccessedMembers`, which needed to be flowed through to called methods.

Tested with code from [Nanorm](https://github.com/DamianEdwards/Nanorm/tree/main/samples/Nanorm.Samples.Console.Todos.Sqlite)

After fixes:

```
PS C:\local\code\TrimTest\TrimTest> dotnet publish -f net8.0 -r win10-x64 --self-contained -v m --nologo -o pub
  Determining projects to restore...
  Restored C:\local\code\TrimTest\TrimTest\TrimTest.csproj (in 283 ms).
  Restored C:\local\code\TrimTest\Nanorm.Sqlite\Nanorm.Sqlite.csproj (in 283 ms).
  Restored C:\local\code\TrimTest\Nanorm\Nanorm.csproj (in 283 ms).
C:\Program Files\dotnet\sdk\8.0.100-preview.3.23178.7\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(287,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [C:\local\code\TrimTest\TrimTest\TrimTest.csproj]
  Nanorm -> C:\local\code\TrimTest\Nanorm\bin\Release\net8.0\Nanorm.dll
  Nanorm.Sqlite -> C:\local\code\TrimTest\Nanorm.Sqlite\bin\Release\net8.0\Nanorm.Sqlite.dll
  TrimTest -> C:\local\code\TrimTest\TrimTest\bin\Release\net8.0\win10-x64\TrimTest.dll
  Generating native code
  TrimTest -> C:\local\code\TrimTest\TrimTest\pub\
```
@MichalStrehovsky
Copy link
Member

The problematic code is below

This can be fixed by just replacing the call to object.GetType (which is where the static analyzer is getting confused) with a Type.GetType with the string of the expected type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-adonet-sqlite area-aot closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
7 participants