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

Improve LoadExtension to work correctly with dotnet run and lib* named libs #35617

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

krwq
Copy link
Member

@krwq krwq commented Feb 11, 2025

Currently if you package an SQLite extension in the nuget package by placing binaries in the runtimes/<rid>/native the extension will work fine when you do dotnet run -r <rid> but when using just dotnet run it won't pick up the dll because they end up in different directory than an app. Currently host uses property NATIVE_DLL_SEARCH_DIRECTORIES to provide all paths where native dlls should be searched for but SQLite doesn't know about its existence. In normal scenario of just loading library this will happen automatically but since SQLite doesn't use that logic we need to account for it so this scenario works correctly.

I've also added searching for libraries with lib prefixes because I noticed #24094 - please let me know if we don't want this (SQLite will only make sure correct extension is used for all OSes but it won't take care of the lib prefix).

Might also fix this but I haven't tested android: #24094

@krwq krwq requested a review from a team as a code owner February 11, 2025 14:18
@krwq krwq changed the title Improve LoadExtension to work correctly with dotnet run and lib* packages Improve LoadExtension to work correctly with dotnet run and lib* named libs Feb 11, 2025
@SamMonoRT
Copy link
Member

@krwq - changes look straightforward, but I would like a review by @AndriySvyryd and @cincuranet once they are back from vacation to understand the entire workflow.

}

return searchDirs!.Split([ Path.PathSeparator ], StringSplitOptions.RemoveEmptyEntries);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return searchDirs!.Split([ Path.PathSeparator ], StringSplitOptions.RemoveEmptyEntries);
return searchDirs.Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot change this because we're building against netstandard 2.0 where I'm getting:

  • (for lack of !): error CS8602: Dereference of a possibly null reference.
  • (using (char, StringSplitOptions) overload): error CS1503: Argument 2: cannot convert from 'System.StringSplitOptions' to 'char'

Copy link
Contributor

@cincuranet cincuranet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor code style changes, but otherwise LGTM.

@AndriySvyryd AndriySvyryd requested a review from Copilot February 25, 2025 18:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

@krwq
Copy link
Member Author

krwq commented Feb 26, 2025

Please hold off with merging until @roji tries out this change on OSX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants