Skip to content

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Dec 13, 2023

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

@iSazonov
Copy link
Collaborator

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

@iSazonov 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
@microsoft-github-policy-service 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
@microsoft-github-policy-service 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
SteveL-MSFT and others added 2 commits December 13, 2023 09:43
…text.cs

Co-authored-by: Ilya <darpa@yandex.ru>
…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.

@daxian-dbw
Copy link
Member

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

@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?

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Dec 13, 2023

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

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

@rhubarb-geek-nz
Copy link

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

@daxian-dbw
Copy link
Member

daxian-dbw commented Dec 13, 2023

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)

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

@iSazonov iSazonov Dec 13, 2023

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please look #20740 (comment)

Copy link
Member Author

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.

Copy link

@rhubarb-geek-nz rhubarb-geek-nz Dec 13, 2023

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.

Copy link
Collaborator

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.

Copy link
Member Author

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?

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

  1. try the exact given name

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

Copy link
Collaborator

@iSazonov iSazonov Dec 14, 2023

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

@iSazonov
Copy link
Collaborator

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.

@rhubarb-geek-nz
Copy link

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

@daxian-dbw
Copy link
Member

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

@SteveL-MSFT 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
@SteveL-MSFT
Copy link
Member Author

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

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Dec 21, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Mar 15, 2024
Copy link
Collaborator

@JamesWTruher JamesWTruher left a 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)).

@daxian-dbw
Copy link
Member

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

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Mar 26, 2024
@rkeithhill 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Review - Needed The PR is being reviewed Small WG-Engine core PowerShell engine, interpreter, and runtime WG-Reviewed A Working Group has reviewed this and made a recommendation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Dependency on Native Libraries" adds extra file extensions
6 participants