Improve LoadExtension to work correctly with dotnet run and lib* named libs#35617
Merged
roji merged 2 commits intodotnet:mainfrom Mar 3, 2025
Merged
Improve LoadExtension to work correctly with dotnet run and lib* named libs#35617roji merged 2 commits intodotnet:mainfrom
roji merged 2 commits intodotnet:mainfrom
Conversation
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. |
cincuranet
reviewed
Feb 21, 2025
cincuranet
reviewed
Feb 21, 2025
| return Array.Empty<string>(); | ||
| } | ||
|
|
||
| return searchDirs!.Split([ Path.PathSeparator ], StringSplitOptions.RemoveEmptyEntries); |
Contributor
There was a problem hiding this comment.
Suggested change
| return searchDirs!.Split([ Path.PathSeparator ], StringSplitOptions.RemoveEmptyEntries); | |
| return searchDirs.Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries); |
Member
Author
There was a problem hiding this comment.
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'
cincuranet
reviewed
Feb 21, 2025
Contributor
cincuranet
left a comment
There was a problem hiding this comment.
Some minor code style changes, but otherwise LGTM.
AndriySvyryd
approved these changes
Feb 24, 2025
Member
Author
|
Please hold off with merging until @roji tries out this change on OSX |
Member
|
Confirmed that this works on OSX, merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently if you package an SQLite extension in the nuget package by placing binaries in the
runtimes/<rid>/nativethe extension will work fine when you dodotnet run -r <rid>but when using justdotnet runit won't pick up the dll because they end up in different directory than an app. Currently host uses propertyNATIVE_DLL_SEARCH_DIRECTORIESto 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
libprefixes 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 thelibprefix).Might also fix this but I haven't tested android: #24094