FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add native dll resolver by iSazonov · Pull Request #11032 · PowerShell/PowerShell · GitHub

/ PowerShell Public

Add native dll resolver - #11032

Merged
Dongbo Wang (daxian-dbw) merged 15 commits into
PowerShell:masterfrom
iSazonov:add-native-dll-resolver
Dec 9, 2019
ConversationCommits (15)ChecksFiles changed
Merged

Add native dll resolver#11032
Dongbo Wang (daxian-dbw) merged 15 commits into
PowerShell:masterfrom
iSazonov:add-native-dll-resolver

Conversation

Ilya (iSazonov) commented Nov 11, 2019 •
edited by daxian-dbw
Loading

Copy link
Copy Markdown
Collaborator

PR Summary

Resolve #9488

Add a native dll resolver to discover and load native dlls based on OS platform and architecture.

PR Context

Releated #3091

If a managed dll has native dependencies the handler will try to find these native dlls.
1. Gets the managed.dll location (folder)
2. Based on OS name and architecture name builds subfolder name where it is expected the native dll resides:
3. Loads the native dll

    ///     managed.dll folder
    ///                     |
    ///                     |--- 'win-x64' subfolder
    ///                     |       |--- native.dll
    ///                     |
    ///                     |--- 'win-x86' subfolder
    ///                     |       |--- native.dll
    ///                     |
    ///                     |--- 'win-arm' subfolder
    ///                     |       |--- native.dll
    ///                     |
    ///                     |--- 'win-arm64' subfolder
    ///                     |       |--- native.dll
    ///                     |
    ///                     |--- 'linux-x64' subfolder
    ///                     |       |--- native.so
    ///                     |
    ///                     |--- 'linux-x86' subfolder
    ///                     |       |--- native.so
    ///                     |
    ///                     |--- 'linux-arm' subfolder
    ///                     |       |--- native.so
    ///                     |
    ///                     |--- 'linux-arm64' subfolder
    ///                     |       |--- native.so
    ///                     |
    ///                     |--- 'osx-x64' subfolder
    ///                     |       |--- native.dylib

PR Checklist

Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 11, 2019
Ilya (iSazonov) added this to the 7.0.0-preview.6 milestone Nov 11, 2019
Ilya (iSazonov) added the Documentation Needed in this repo Documentation is needed in this repo label Nov 11, 2019

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 11, 2019

Copy link
Copy Markdown
Collaborator

This looks super promising, wow! Nice work! 😍

I have one suggestion just for compatibility; prior to 6.2 (or 6.1? Don't recall exactly) pwsh was able to search the location the library was in for native DLLs (though not subfolders). As part of one of the changes in 6.2, that was removed, and this seems like a sensible place to add it.

Can we have it also check the same folder as the managed library so that anything that was using the old behaviour works again as well?

Ilya (iSazonov) commented Nov 11, 2019

Copy link
Copy Markdown
Collaborator Author

Can we have it also check the same folder as the managed library so that anything that was using the old behaviour works again as well?

Yes, we can. I think first we need to have a standard (RFC?) how we search, order is important. Also we should consider security compliance.

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

/cc Paul Higinbotham (@PaulHigin) Steve Lee (@SteveL-MSFT)

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 11, 2019

Copy link
Copy Markdown
Collaborator

Ilya (@iSazonov) we may want to look into whether there's a way to pull the acceptable RIDs from dotnet / if this is available from a .NET API rather than having a hardcoded set here. 🙂

Ilya (iSazonov) commented Nov 11, 2019 •
edited by unfurl-links Bot
Loading

Copy link
Copy Markdown
Collaborator Author

Rain Sallow (/u/ta11ow) (@vexx32) .Net Core has not API to expose RIDs.
We could support all RIDs from https://docs.microsoft.com/en-us/dotnet/core/rid-catalog
but it will very complicate the resolver.
I hope the proposed solution covers most of use cases. Notice, that key here is that we calculate path based on managed assembly location: if the assembly is right assembly for OS, arch, runtime then we expect that native assembly also is right assembly and is in right place. I hope other scenarios is edge cases that we could delegate third-party project developers who could use DllImportResolver callbacks directly.

Learn about the Runtime IDentifier (RID) and how RIDs are used in .NET Core.

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Ilya (@iSazonov) yeah I wondered. The reason I ask is that some RIDs include OS version as well as architecture, so we might be missing (for example) a folder with win7-x64 quite easily. Perhaps we could for the moment solve that problem with use of wildcards?

Steve Lee (SteveL-MSFT) commented Nov 11, 2019

Copy link
Copy Markdown
Member

It makes sense to follow the .NET RID hierarchy so that it's consistent and predictable. It doesn't have to be an exact match, but I can image build scripts that create the native folder under the RID passed to msbuild. So for macOS, you're just as likely to see osx-x64 folder.

Ilya (iSazonov) commented Nov 11, 2019

Copy link
Copy Markdown
Collaborator Author

Will replace "osx" with "osx-x64".

Ilya (iSazonov) commented Nov 11, 2019

Copy link
Copy Markdown
Collaborator Author

The reason I ask is that some RIDs include OS version as well as architecture, so we might be missing (for example) a folder with win7-x64 quite easily. Perhaps we could for the moment solve that problem with use of wildcards?

It is just a case I name as "edge". Very unlikely to be needed. I believe any developer prefer to create portable solutions.
Also Core does not have a reliable way to determine OS/kernel version.
So I suggest only use portable RIDs. At least until we get feedback.

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

🤔 along with this, would it perhaps be feasible to expose a public API to allow module authors to add specific paths to search when working in those edge cases?

(maybe that's a future discussion 😆)

Ilya (iSazonov) commented Nov 12, 2019

Copy link
Copy Markdown
Collaborator Author

to expose a public API

The API is already in .Net Core. See DllImportResolver callbacks reference above. I guess it is possible to use even script delegate for the callback.

This was referenced Nov 14, 2019
Closed
Closed
Closed
Dongbo Wang (daxian-dbw) reviewed Nov 21, 2019
View reviewed changes

Dongbo Wang (daxian-dbw) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you please add one test?
I guess you can fake a native library file (renaming a text file to native.dll), then the resolution should succeed but loading the native library will fail with a BadImageFormatException -- An attempt was made to load a program with an incorrect format. (0x8007000B).

Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated
Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated
Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated
Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated
Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated
Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated
Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated
Comment thread src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated
Ilya (iSazonov) mentioned this pull request Nov 30, 2019
Closed

Travis Plunk (TravisEz13) commented Dec 3, 2019

Copy link
Copy Markdown
Member

Ilya (@iSazonov) Please don't use 7.0-consider for PRs... I'll try to update the descriptions to say which milestones are for issues and which are for PRs.

Ilya (iSazonov) removed this from the 7.0-Consider milestone Dec 4, 2019
Dongbo Wang (daxian-dbw) approved these changes Dec 6, 2019
View reviewed changes

Dongbo Wang (daxian-dbw) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Dongbo Wang (daxian-dbw) added this to the rc.1-consider milestone Dec 6, 2019
Ilya (iSazonov) mentioned this pull request Dec 6, 2019
Closed
Ilya (iSazonov) removed the Documentation Needed in this repo Documentation is needed in this repo label Dec 6, 2019

Ilya (iSazonov) commented Dec 6, 2019

Copy link
Copy Markdown
Collaborator Author

Added a reference to new doc issue.

Dongbo Wang (daxian-dbw) merged commit 95c472a into PowerShell:master Dec 9, 2019
Ilya (iSazonov) deleted the add-native-dll-resolver branch December 10, 2019 04:39
Travis Plunk (TravisEz13) pushed a commit that referenced this pull request Dec 11, 2019
Add native binary resolver (#11032)
5df5ac8
Joey Aiello (joeyaiello) mentioned this pull request Dec 12, 2019
Closed
8 tasks

Deleted user (ghost) commented Dec 16, 2019

Copy link
Copy Markdown

🎉v7.0.0-rc.1 has been released which incorporates this pull request.:tada:

Handy links:

ghost mentioned this pull request Dec 16, 2019
Closed
Closed
Ilya (iSazonov) mentioned this pull request Jan 30, 2020
Closed
Steve Lee (SteveL-MSFT) mentioned this pull request Mar 14, 2020
Closed
Closed
refactorsaurusrex mentioned this pull request May 26, 2020
Closed
Steve Lee (SteveL-MSFT) mentioned this pull request Oct 7, 2021
Closed
Travis Plunk (TravisEz13) mentioned this pull request Oct 12, 2022
Merged
22 tasks
Steve Lee (SteveL-MSFT) mentioned this pull request Dec 13, 2023
Open
22 tasks
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
Add native binary resolver (PowerShell#11032)
bd35bfd
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan
+2 more reviewers
PoshChan-Bot (PoshChan) PoshChan left review comments
Rain Sallow (/u/ta11ow) (vexx32) vexx32 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

Dongbo Wang (daxian-dbw)

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.0.0-rc.1

Development

Successfully merging this pull request may close these issues.

Loading Native Libraries is broken

6 participants

Back | FazBrowse Home | New Git URL