-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Fix native library resolver to handle cases with extension #20912
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
base: master
Are you sure you want to change the base?
Conversation
@rhubarb-geek-nz Perhaps the PR resolve your issue. Please try download artifact and check with your module.. |
src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs
Outdated
Show resolved
Hide resolved
1b932ed
to
1d14d59
Compare
1d14d59
to
19003e8
Compare
src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs
Outdated
Show resolved
Hide resolved
…text.cs Co-authored-by: Ilya <darpa@yandex.ru>
…text.cs Co-authored-by: Ilya <darpa@yandex.ru>
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
The issue mentioned that
@SteveL-MSFT and @iSazonov The native library resolver was introduced in 2019, so 7.3.10 should be using the same code/logic. Any idea why it works in 7.3.10 but not 7.4.0? Anything changed during 7.4.0? |
That part doesn't make sense to me. I ran my new tests under 7.3.10 and they fail without this change. |
The case where 7.4.0 is not looking in the RID directories is when the script was passed on the pwsh command line so does not enter interactive mode, or likewise the script ran with a hash-bang "#!/usr/bin/env pwsh" If 7.4.0 runs interactively and the script is launched from within PowerShell then it resolves the native libraries correctly |
Does it work on v7.3.10 for these 2 scenarios? Looks like it did work on v7.3.10 for these 2 scenarios, see #20740 (comment) |
} | ||
else if (extension != s_nativeDllExtension) | ||
{ | ||
fullName = Path.Combine(folder, s_nativeDllSubFolder, Path.GetFileNameWithoutExtension(libraryName)) + s_nativeDllExtension; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be controversial. Is it worth changing the extension that is specified by the developer in his code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look #20740 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the problem with the sqlite repro? It seems that the developer accidentally specified .dll but also shipped libraries for other OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no requirement that a shared library does end with dylib, dll or so. merely convention. If you remember on Windows control panel extensions, drivers and OLE controls were dlls but did not have dll as the extension.
I don't think the developer accidentally specified .dll in the DllImport, the examples from Microsoft have "user32.dll" etc in them. As the module is common and only the shared libraries are different, it is consistent to have them all with the same name but just in a different directory according to architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DllImport documentation says that we can use You can provide a full or relative path.
or As a minimum requirement, you must supply the name of the DLL containing the entry point.
and says nothing about extensions. So developers are free to use any file extension for dynamic library.
While I agree that deviating from the usual values looks amazing, there are no standards to call it a bug, especially since these non-standard extensions work in C# applications. If we forcibly change the extension, then obviously these applications will stop working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps if it has an extension, we try loading it as-is, and if it fails, then try the os specific extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest not removing anything from the string that was originally given, trust that the original string was correct, because if it is in published code it must have been tested like that and worked.
The simplest would be...
-
try the exact given name
-
if that does not work try with operating system specific extension appended
No replacing of the extension with something else, only appending.
So if you don't specify an extension you will get the OS one.
If you do, your extension will be honoured first, then it will try with your full name plus OS extension.
And of course it has to go through (a) in the root directory (b) in the directory with just the CPU name (c) in the directory with OS-CPU
So may take 6 attempts to load the DLL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback doesn't make a lot of sense. If a developer specified DllImport("interop.msft")
in the code, then obviously he is referring to the real interop.msft
file on file system and there is no interop.msft.dll
file or interop.dll
file.
(And I don't think DllImport is smart enough to handle multi-dot names. I mean DllImport("interop.msft")
will do not search interop.msft.dll
on Windows and interop.msft.so
on Linux. If he does, it will work for us automatically.)
There is another issue #20740. The PR doesn't fix it #20740 as @rhubarb-geek-nz said. |
yes |
@rhubarb-geek-nz Thanks for the clarification. Then it sounds to me the root cause is not in native library resolver, but somewhere else. @SteveL-MSFT can you please take into account this updated information and re-consider the fix? |
@daxian-dbw I removed the backport label since this doesn't resolve that regression. However, let's have the Engine WG discuss if this new behavior makes sense to take. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the WG discussed this and believe that the 2nd bullet (if the library has an extension mylib.dll and is not Windows, remove the extension) is problematic as there are instance where non-windows systems will have files with .dll as an extension, such as SQLite.Interop.dll
that is pulled in when using the System.Data.SQLite package (see #20740 (comment)). This change would cause that scenario to break.
We do agree with the behavior described in bullet 3. We also believe that if the user provides an extension, we should first attempt to load that specified assembly before trying to be clever by substituting the "proper" extension (see #20740 (comment)).
@SteveL-MSFT I think this PR will address #19277 and added |
PR Summary
Change native resolver to not add the library extension if it is already part of the library name. This was an issue reported internally at Microsoft.
This PR now accounts for 3 scenarios:
mylib.dll
and is on Windows, it's left as-is. This was a bug previously where the code sitll adds.dll
so it tries to loadmylib.dll.dll
.mylib.dll
and is not Windows, the extension is removed and the appropriate extension is usedPR Context
Fix #19277
The addition of the native library resolver via #11032 didn't cover the case where the library name already includes the extension so it adds another extension which then fails to find the native library.
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).