Fix native library resolver to handle cases with extension by SteveL-MSFT · Pull Request #20912 · PowerShell/PowerShell · GitHub

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:

If the library has an extension 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 load mylib.dll.dll.
If the library has an extension mylib.dll and is not Windows, the extension is removed and the appropriate extension is used
If the library doesn't have an extension, the appropriate one for the OS is added (existing behavior).

PR 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

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
Breaking changes
- None
- OR
- Experimental feature(s) needed
  - Experimental feature name(s):
User-facing changes
- Not Applicable
- OR
- Documentation needed
  - Issue filed:
Testing - New and feature
- N/A or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
Tooling
- I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
- OR
- I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
  - Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
    (which runs in a different PS Host).
    - Issue filed:
  - Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
    - Issue filed:
  - Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
    - Issue filed:
  - Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
    - Issue filed:

rhubarb-geek-nz Perhaps the PR resolve your issue. Please try download artifact and check with your module..

…text.cs Co-authored-by: Ilya <darpa@yandex.ru>

This PR has 59 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Small
Size       : +57 -2
Percentile : 23.6%

Total files changed: 2

Change summary by file extension:
.cs : +8 -1
.ps1 : +49 -1

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

The issue mentioned that

7.3.10 finds and loads these shared libraries.
7.4.0 does not look for them on Linux under the RID directory

Steve Lee (@SteveL-MSFT) and Ilya (@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?

The issue mentioned that

7.3.10 finds and loads these shared libraries.
7.4.0 does not look for them on Linux under the RID directory

Steve Lee (@SteveL-MSFT) and Ilya (@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

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"

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)

This could be controversial. Is it worth changing the extension that is specified by the developer in his code?

Please look #20740 (comment)

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 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.

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.

Perhaps if it has an extension, we try loading it as-is, and if it fails, then try the os specific extension?

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.

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.)

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"

Does it work on v7.3.10 for these 2 scenarios?

There is another issue #20740. The PR doesn't fix it #20740 as rhubarb-geek-nz said.

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"

Does it work on v7.3.10 for these 2 scenarios?

yes

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"

Does it work on v7.3.10 for these 2 scenarios?

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. Steve Lee (@SteveL-MSFT) can you please take into account this updated information and re-consider the fix?

Dongbo Wang (@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.

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)).

Steve Lee (@SteveL-MSFT) I think this PR will address #19277 and added Fix #19277 in the PR description. Please take a look at the issue and feel free to remove it if I'm wrong.

Fix native library resolver to handle cases with extension

3b5fc2a

Steve Lee (SteveL-MSFT) requested a review from Ilya (iSazonov) December 13, 2023 06:36

Steve Lee (SteveL-MSFT) assigned Ilya (iSazonov) Dec 13, 2023

pull-request-quantifier-deprecated Bot added the Extra Small label Dec 13, 2023

Steve Lee (SteveL-MSFT) added Backport-7.4.x-Consider and removed Extra Small labels Dec 13, 2023

Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 13, 2023

Ilya (iSazonov) approved these changes Dec 13, 2023

View reviewed changes

Ilya (iSazonov) requested changes Dec 13, 2023

View reviewed changes

Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated

microsoft-github-policy-service Bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 13, 2023

Ilya (iSazonov) mentioned this pull request Dec 13, 2023

7.4.0 Linux native shared libraries in module not loaded using the RID directory #20740

Closed

5 tasks

microsoft-github-policy-service Bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 13, 2023

Steve Lee (SteveL-MSFT) force-pushed the native-resolver branch from 1b932ed to 1d14d59 Compare December 13, 2023 14:59

change code to remove invalid extension first

19003e8

Steve Lee (SteveL-MSFT) force-pushed the native-resolver branch from 1d14d59 to 19003e8 Compare December 13, 2023 15:08

update tests

4756d45

pull-request-quantifier-deprecated Bot added the Small label Dec 13, 2023

address codefactor

1adc65c

Ilya (iSazonov) reviewed Dec 13, 2023

View reviewed changes

Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated

Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated

Steve Lee (SteveL-MSFT) and others added 2 commits December 13, 2023 09:43

Update src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadCon…

99172aa

…text.cs Co-authored-by: Ilya <darpa@yandex.ru>

Update src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadCon…

5d04af6

…text.cs Co-authored-by: Ilya <darpa@yandex.ru>

Ilya (iSazonov) reviewed Dec 13, 2023

View reviewed changes

Steve Lee (SteveL-MSFT) added WG-Engine core PowerShell engine, interpreter, and runtime WG-NeedsReview Needs a review by the labeled Working Group and removed Backport-7.4.x-Consider labels Dec 13, 2023

microsoft-github-policy-service Bot added the Review - Needed The PR is being reviewed label Dec 21, 2023

microsoft-github-policy-service Bot removed the Review - Needed The PR is being reviewed label Mar 15, 2024

James Truher (JamesWTruher) suggested changes Mar 18, 2024

View reviewed changes

microsoft-github-policy-service Bot added the Review - Needed The PR is being reviewed label Mar 26, 2024

Keith Hill (rkeithhill) added WG-Reviewed A Working Group has reviewed this and made a recommendation and removed WG-NeedsReview Needs a review by the labeled Working Group labels Jul 7, 2024

+                          }
+                          else if (extension != s_nativeDllExtension)
+                          {
+                              fullName = Path.Combine(folder, s_nativeDllSubFolder, Path.GetFileNameWithoutExtension(libraryName)) + s_nativeDllExtension;

Fix native library resolver to handle cases with extension by SteveL-MSFT · Pull Request #20912 · PowerShell/PowerShell · GitHub

Uh oh!

Fix native library resolver to handle cases with extension - #20912

Fix native library resolver to handle cases with extension#20912Steve Lee (SteveL-MSFT) wants to merge 6 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom SteveL-MSFT:native-resolverSteveL-MSFT/PowerShell:native-resolverCopy head branch name to clipboard

Conversation

Steve Lee (SteveL-MSFT) commented Dec 13, 2023 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Ilya (iSazonov) commented Dec 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pull-request-quantifier-deprecated Bot commented Dec 13, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

Dongbo Wang (daxian-dbw) commented Dec 13, 2023

Uh oh!

Steve Lee (SteveL-MSFT) commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhubarb-geek-nz commented Dec 13, 2023

Uh oh!

Dongbo Wang (daxian-dbw) commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

rhubarb-geek-nz Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

rhubarb-geek-nz Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Dec 13, 2023

Uh oh!

rhubarb-geek-nz commented Dec 13, 2023

Uh oh!

Dongbo Wang (daxian-dbw) commented Dec 13, 2023

Uh oh!

Steve Lee (SteveL-MSFT) commented Dec 13, 2023

Uh oh!

James Truher (JamesWTruher) left a comment • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) commented Mar 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix native library resolver to handle cases with extension#20912
Steve Lee (SteveL-MSFT) wants to merge 6 commits into
PowerShell:masterfrom
SteveL-MSFT:native-resolver

Steve Lee (SteveL-MSFT) commented Dec 13, 2023 •
edited by daxian-dbw
Loading

Steve Lee (SteveL-MSFT) commented Dec 13, 2023 •
edited
Loading

Dongbo Wang (daxian-dbw) commented Dec 13, 2023 •
edited
Loading

Ilya (iSazonov) Dec 13, 2023 •
edited
Loading

rhubarb-geek-nz Dec 13, 2023 •
edited
Loading

Ilya (iSazonov) Dec 14, 2023 •
edited
Loading

James Truher (JamesWTruher) left a comment •
edited by daxian-dbw
Loading