Skip to content

Add native dll resolver #11032

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.

Sign up for GitHub

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

Jump to bottom
Merged
daxian-dbw merged 15 commits into from
Dec 9, 2019
Merged

Add native dll resolver #11032

daxian-dbw merged 15 commits into from
Dec 9, 2019

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Nov 11, 2019 •
edited by daxian-dbw
Loading

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

@iSazonov 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
@iSazonov iSazonov added this to the 7.0.0-preview.6 milestone Nov 11, 2019
@iSazonov iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Nov 11, 2019
@vexx32
Copy link
Collaborator

vexx32 commented Nov 11, 2019

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?

@iSazonov
Copy link
Collaborator Author

iSazonov commented Nov 11, 2019

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.

@vexx32
Copy link
Collaborator

vexx32 commented Nov 11, 2019 •
edited
Loading

/cc @PaulHigin @SteveL-MSFT

@vexx32
Copy link
Collaborator

vexx32 commented Nov 11, 2019

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

@iSazonov
Copy link
Collaborator Author

iSazonov commented Nov 11, 2019 •
edited by unfurl-links bot
Loading

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

@vexx32
Copy link
Collaborator

vexx32 commented Nov 11, 2019 •
edited
Loading

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

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Nov 11, 2019

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.

@iSazonov
Copy link
Collaborator Author

iSazonov commented Nov 11, 2019

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

@iSazonov
Copy link
Collaborator Author

iSazonov commented Nov 11, 2019

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.

@vexx32
Copy link
Collaborator

vexx32 commented Nov 11, 2019 •
edited
Loading

🤔 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 😆)

@iSazonov
Copy link
Collaborator Author

iSazonov commented Nov 12, 2019

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
@vexx32 vexx32 mentioned this pull request Nov 21, 2019
Closed
daxian-dbw
daxian-dbw reviewed Nov 21, 2019
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

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

@daxian-dbw daxian-dbw added this to the 7.0-Consider milestone Nov 27, 2019
@iSazonov iSazonov mentioned this pull request Nov 30, 2019
Closed
@TravisEz13
Copy link
Member

TravisEz13 commented Dec 3, 2019

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

@iSazonov iSazonov removed this from the 7.0-Consider milestone Dec 4, 2019
daxian-dbw
daxian-dbw approved these changes Dec 6, 2019
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@daxian-dbw daxian-dbw added this to the rc.1-consider milestone Dec 6, 2019
@iSazonov iSazonov mentioned this pull request Dec 6, 2019
Closed
@iSazonov iSazonov removed the Documentation Needed in this repo Documentation is needed in this repo label Dec 6, 2019
@iSazonov
Copy link
Collaborator Author

iSazonov commented Dec 6, 2019

Added a reference to new doc issue.

@daxian-dbw daxian-dbw merged commit 95c472a into PowerShell:master Dec 9, 2019
@iSazonov iSazonov deleted the add-native-dll-resolver branch December 10, 2019 04:39
@TravisEz13 TravisEz13 modified the milestones: rc.1-approved, 7.0.0-rc.1 Dec 11, 2019
TravisEz13 pushed a commit that referenced this pull request Dec 11, 2019
@iSazonov @TravisEz13
Add native binary resolver (#11032)
5df5ac8
@joeyaiello joeyaiello mentioned this pull request Dec 12, 2019
Closed
8 tasks
@ghost
Copy link

ghost commented Dec 16, 2019

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

Handy links:

@ghost ghost mentioned this pull request Dec 16, 2019
Closed
@vexx32 vexx32 mentioned this pull request Jan 20, 2020
Closed
@iSazonov iSazonov mentioned this pull request Jan 30, 2020
Closed
@SteveL-MSFT SteveL-MSFT mentioned this pull request Mar 14, 2020
Closed
@mattleibow mattleibow mentioned this pull request Apr 25, 2020
Closed
@refactorsaurusrex refactorsaurusrex mentioned this pull request May 26, 2020
Closed
@SteveL-MSFT SteveL-MSFT mentioned this pull request Oct 7, 2021
Closed
@TravisEz13 TravisEz13 mentioned this pull request Oct 12, 2022
Merged
22 tasks
@SteveL-MSFT SteveL-MSFT mentioned this pull request Dec 13, 2023
Open
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

+2 more reviewers

@PoshChan PoshChan PoshChan left review comments

@vexx32 vexx32 vexx32 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw 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

@iSazonov @vexx32 @SteveL-MSFT @daxian-dbw @TravisEz13 @PoshChan