Skip to content
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

Add single-file app's dir to NATIVE_DLL_SEARCH_DIRECTORIES #42876

Merged
merged 6 commits into from
Oct 2, 2020

Conversation

mateoatr
Copy link
Contributor

Fixes #42772. I took the easy path here - whenever the app is single-file, add the exe's dir to NATIVE_DLL_SEARCH_DIRECTORIES. Additionally, if the user sets -p:IncludeNativeLibrariesForSelfExtract=true, add the extraction path to the search directories.

@ghost
Copy link

ghost commented Sep 29, 2020

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

@vitek-karas
Copy link
Member

I think the logic should be:

  • If this is a single-file - add the bundle's directory to the native search paths
  • If we extracted anything - add the extraction directory to the native search paths

This should specifically cover the 3.1 backward compat mode (I know it works today on Windows, but it won't work on Linux ).

We obviously need tests for this - we can start with validating the content of the NATIVE_DLL_SEARCH_DIRECTORIES as that is easy enough to do in the tests.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Tests?

src/installer/corehost/cli/bundle/header.h Outdated Show resolved Hide resolved
@elinor-fung
Copy link
Member

This should specifically cover the 3.1 backward compat mode (I know it works today on Windows, but it won't work on Linux ).

What is broken with 3.1 compat mode on Linux for this? It should (with the fix for actually extracting) be able to search for native libraries based on the managed assembly's path?

@vitek-karas
Copy link
Member

You're right that it will basically work - but as you pointed out - the design called for setting the extraction path as one of the native search directories - which this should fix. There are still potential scenarios (plugins) where not setting it will break the app (mostly on Linux), but I agree that they're not that common.

@mateoatr
Copy link
Contributor Author

I removed needs_native_libraries_extraction from the manifest flags and changed the logic so that whenever the extraction dir exists, we add it to the native search paths.
For testing I'm just validating that we put the bundle's dir and the extraction dir in NATIVE_DLL_SEARCH_DIRECTORIES.

public void AppContext_Native_Search_Dirs_Contains_Bundle_And_Extraction_Dirs()
{
var fixture = sharedTestState.TestFixture.Copy();
Bundler bundler = BundleHelper.BundleApp(fixture, out string singleFile, BundleOptions.BundleNativeBinaries);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to make sure the bundle actually has something to extract so that it creates/adds the extraction directory? By default, we won't have anything to extract on Linux, right?

@agocke
Copy link
Member

agocke commented Oct 1, 2020

@mateoatr I don't think these changes are strictly necessary, they're more test hygiene suggestions. If you think it would be faster to deal with these separately, that's fine. We can merge this change, and take the modifications in another PR.

@mateoatr
Copy link
Contributor Author

mateoatr commented Oct 1, 2020

@agocke I had some issues testing locally, but I just pushed your suggestions. I think we can merge after all tests run in the CI.

@agocke
Copy link
Member

agocke commented Oct 1, 2020

Yeah, I've been trying to work on stuff locally and mostly got stuff to work by building everything, then runnign the apphost test by doing ~/runtime/dotnet.sh build then ~/runtime/dotnet.sh build -t:test

.And.HaveStdOutContaining("ExecutingAssembly.Location: " + extractionDir) // Should point to the app's dll
.And.HaveStdOutContaining("AppContext.BaseDirectory: " + extractionDir) // Should point to the extraction directory
// In extraction mode, we should have both dirs
.And.HaveStdOutMatching($"NATIVE_DLL_SEARCH_DIRECTORIES: .*{extractionDir}.*{bundleDir}");
Copy link
Member

Choose a reason for hiding this comment

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

Huh, I assume the Matching is turning this into a regex, meaning that the \s now need to be escaped on Windows?

@agocke
Copy link
Member

agocke commented Oct 2, 2020

Moving us back to the previous commit, since that passed tests, so we can get this in today

@agocke agocke merged commit 0403018 into dotnet:master Oct 2, 2020
agocke pushed a commit to agocke/runtime that referenced this pull request Oct 2, 2020
)

Add two directories to NATIVE_DLL_SEARCH_DIRECTORIES to single-file bundles:

1. The bundle exe directory
2. If the bundle extracts any files, the extraction directory

Fixes dotnet#42772

(cherry picked from commit 0403018)
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Single-File] DllImport fails to find native library
4 participants