Extract bundled files when IncludeAllContentForSelfExtract is set #42435

mateoatr · 2020-09-18T05:47:36Z

ghost · 2020-09-18T05:47:39Z

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

src/installer/corehost/cli/bundle/manifest.h

agocke · 2020-09-18T08:09:58Z

src/installer/corehost/cli/bundle/extractor.cpp

@@ -204,7 +204,7 @@ void extractor_t::extract_new(reader_t& reader)
    begin();
    for (const file_entry_t& entry : m_manifest.files) 
    {
-        if (entry.needs_extraction())
+        if (m_manifest.is_netcoreapp3_compat_mode() || entry.needs_extraction())


It looks like there are a number of other places in this file that are wrong (locate, et al). I think a safer approach than trying to modify all the use sites is to have file_entry_t::read take an extra force_extract parameter, then pass it through to each file_entry_t constructor and store it in a field. Then we can check the field in needs_extraction and avoid changing all the call sites

Agreed - the problem is everywhere we call entry.needs_extraction, so it would be easier to bake it into the needs_extraction itself. It would feel cleaner if we stored something like extracted on each file entry.

agocke · 2020-09-18T08:10:52Z

...aller/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/NetCoreApp3CompatModeTests.cs

+                .Should()
+                .Pass()
+                .And
+                .HaveStdOutContaining("Hello World");


Any way we could also confirm that the assemblies are actually loaded from the extraction location and not the bundle? Perhaps by printing the result of Assembly.Location or similar?

We should definitely test for this.

I think we always set BUNDLE_PROBE (regardless of 3.x compat mode), so bundle probing takes precedence during assembly loading - I could see some undesired behaviour happening as a result.

With the most recent changes bundle probe should return false for all queries - I checked that the runtime behavior in that case is the same as without bundle probe in the case of assembly resolution. Note that it's not the same in other cases - especially Environment.GetCommandLineArguments[0] should still return the .exe path, not the main .dll path.

I also checked all the other places in runtime where we somehow react to bundle probe, and they all look correct to me.

This also means we can still pass bundle probe to the runtime even in full-extract case as all files will be marked as extracted in that situation (so the probe should return false all the time). This might be benefitial as the runtime will still "know" it's single-file (we don't really use that knowledge today though). Improve the test to actually validate locally built binaries and add validation of TPA and framework assembly loading.

… into netCoreApp3Compat

vitek-karas · 2020-09-21T22:14:19Z

I tested lot of cases around the APIs which behave in a different/interesting way in single-file cases - AFAIK the behavior is as expected now. That is - for 3.1 backcompat, almost all APIs behave as non-single, with a couple of exceptions.

@elinor-fung @agocke could you please take a second look?

elinor-fung

In AssemblyBinder::BindToSystem, if the bundle probe doesn't return a valid location, we expect System.Private.CoreLib.dll to be in the runtime's directory - won't that be problematic for platforms like Linux where the runtime is linked into the host?

almost all APIs behave as non-single, with a couple of exceptions

Can we make sure we have a list somewhere in comments and for doc?

src/installer/corehost/cli/hostpolicy/deps_resolver.h

src/installer/corehost/cli/bundle/manifest.cpp

src/installer/tests/Assets/TestProjects/SingleFileApiTests/Program.cs

...aller/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/NetCoreApp3CompatModeTests.cs

src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp

elinor-fung · 2020-09-22T03:40:03Z

Re: testing - not sure if this has already been done or how simple it would be, but it might be interesting to grab a known consumer of 3.x single-file and try building/running it against 5.0 with this backcompat property. I remember Greenshot being a common reporter of issues with single-file in 3.x

vitek-karas · 2020-09-22T12:14:31Z

@elinor-fung You're correct that this is broken on Linux due to not finding CoreLib. Unfortunately this is a bigger issue:
In runtime there's a notion of "system directory", which currently is always deduced from the location of the runtime module. For non-single-file this points to the path of the coreclr.dll. For single-file this points to the path of the bundle .exe. This works well for all cases except this special case - since we want the system to behave as if the app is loaded from the extracted location fully.

CoreLib's path is derived from the system path.
There are several other places where system path is used - for example that's where we look for DAC, that's where GC looks for plugin GCs, that's where we look for runtime resources, ....

I can see several possible ways to "fix" this:

Introduce a fallback when we look for corelib and look into the path passed by APP_CONEXT_BASE_DIRECTORY runtime property - which will point to the extraction directory. This would fix the CoreLib problem, but the other dependencies on system path would still be broken.
Introduce a new runtime property SYSTEM_PATH - the host would set this only in this special case and we would use that everywhere. This would simulate the old behavior pretty well.
Change how we package single-file in the 3.1 case - basically go back to 3.1 behavior fully - so don't use superhost - bundle coreclr.dll as normal.
- We could use the 3.1 apphost for this (the one which can self-extract) - but it would be a servicing nightmare probably
- We would have to introduce a new binary on linux - superhost can't do this, so we would have to build the singlefilehost just like Windows does on Linux as well - and ship that.

If I could get any solution for free I would pick the last one - go back to exactly how 3.1 did it, but that's very expensive and introduces lot of differences in behavior in 5.0.
The next best solution right now seems to introduce the SYSTEM_PATH property and wire it up internally in the runtime. It's still not ideal, but it's better than hacking around CoreLib alone.

vitek-karas · 2020-09-22T15:00:45Z

Yet another possible solution would be to hardcode CoreLib into the bundle such that the probe always returns true for CoreLib (even if it's extracted onto disk) - the downside is that corelib would be memory-loaded so without Location and so on - less compatible.

vitek-karas · 2020-09-22T15:54:55Z

Candidate fix for the CoreLib issue on Linux - passing system path from host approach:
mateoatr#1

It works well on Linux - also works on Windows (since it will sue the same codepath on Windows as well for consistency).
Please take a look - I didn't want to push it here if we don't think it's the right way to fix the problem.

jkotas · 2020-09-22T16:49:00Z

Change how we package single-file in the 3.1 case - basically go back to 3.1 behavior fully - so don't use superhost - bundle coreclr.dll as normal

This is the most compatible solution. It would be my top pick. Note that it will also address all not-yet-known issues where the true single file is breaking layout assumptions hardcoded in components out there (e.g. debugger?).

The next best solution that I can think of is to extract the CoreLib path from TRUSTED_PLATFORM_ASSEMBLIES. The host send the CoreLib path in TRUSTED_PLATFORM_ASSEMBLIES, but runtime ignores it and tries to find corelib next to coreclr.dll.

elinor-fung · 2020-09-22T17:05:57Z

If I could get any solution for free I would pick the last one - go back to exactly how 3.1 did it

Agreed. But, but yeah - the host build / packaging / sdk changes might be a bit expensive at this point.

The CoreLib from TRUSTED_PLATFORM_ASSEMBLIES solution would only hit the CoreLib problem, but not other dependencies on the system path? Or would we be using that as the system path as well?

jkotas · 2020-09-22T19:25:53Z

I would fix just the CoreLib problem and leave the rest alone (for the 5.0 fix). The system directory concept is overloaded and it would be best to eliminate it.

vitek-karas · 2020-09-23T16:09:26Z

So I prepared a fix which adds a fallback to CoreLib search - if we don't find it in the bundle, nor "next" to the runtime, we will also look for it in TPA and use that path there.

Tested it that it works in the single-file scenario on Linux.

The change is not super clean as doing that would require non-trivial refactorings, so I avoided that. I just did the minimum to avoid too much code duplication. The general idea:

Use the Configuration class to get to the TPA value (this is also not very clean, but it works and effectively does the right thing)
Use the same code we use to parse TPA - we can now end up parsing it twice
Search of CoreLib in the TPA that way

The change is here (didn't push it here to get some feedback on the approach first)
mateoatr#2

@elinor-fung @jank could you please take a quick look if this approach sounds reasonable?

jkotas · 2020-09-23T16:28:32Z

Looks reasonable as release/5.0 fix.

elinor-fung · 2020-09-23T17:22:24Z

That seems reasonable for 5.0

Search for CoreLib in TPA

elinor-fung · 2020-09-25T22:54:01Z

The other thing I found is that if there is no .deps.json file (e.g. GenerateDependencyFile=false), the published single-file app doesn't run - self-contained can't find coreclr on Windows, can't find SPCL on Linux. Runs fine with 3.1 single-file (or IncludeAllContentForSelfExtract=false)

vitek-karas · 2020-09-25T22:59:13Z

Sad face...
I guess we could make that work with a fix which I think Mateo actually implemented at one point - the piece of code you asked about (why do we add CoreLib to TPA at the end, always). We could tweak that to add CoreLib to TPA in the 3.0 compat case (it is currently disabled when CoreLib exists in the bundle, because we expect it to be loaded from the bundle - which is technically wrong).

vitek-karas · 2020-09-25T22:59:48Z

That said - I would be fine not supporting that case if it came down to it - it's such a niche thing ... I think

elinor-fung · 2020-09-25T23:08:00Z

Yeah, I'd also be fine not supporting it - seems like we'd have to special case where we load coreclr from and adding SPCL to the TPA in compat-mode... I think it is pretty niche as well, but we do talk about how loading works in our default dependency probing doc.

The path must point to the extraction folder and not use the "fake" in-bundle path.

vitek-karas · 2020-09-25T23:19:40Z

So the last commit should fix the .deps.json problem (had trouble with local setup, somehow everything got corrupted and I had to rebuild the entire repo again). That leaves only the no .deps.json case - I may look at that as well - since we know about it and the fix seems simple enough - we might as well.

Actually we should be using path separator char in the APP_CONTEXT_DEPS_FILES, but that's something we won't be able to change really (too much chance of breaking something without too much value).

vitek-karas · 2020-09-28T14:43:04Z

I tried the missing .deps.json case and it actually asserts in the host... pretty bad. I will probably look into this a little bit.
On a bright side - it works without .deps.json and without backward compat mode - I don't know why we find the runtime in the host, but after that finding everything else is easy because we probe the bundle every time we need instead of enumerating files in the TPA - so as long as it's in the bundle, it doesn't have to be in .deps.json for runtime to find it.

agocke · 2020-09-28T23:21:44Z

@jkotas @janvorli @davidwrighton Would one of you have some time to do a last review here?

jkotas · 2020-09-29T01:53:51Z

Changes under src/coreclr/src look good to me. I have glanced over the rest and I did not find anything suspect, but I do not understand that part well.

janvorli · 2020-09-29T11:46:44Z

src/coreclr/src/binder/utils.cpp

+            SString sDll(W(".dll"));
+            SString sExe(W(".exe"));
+
+            if (!dllOnly && (outPath.EndsWithCaseInsensitive(sNiDll) ||


Why do we exclude the .ni.dll here? I wonder if that's correct.

The dllOnly is currently only set to true when searching for System.Private.CoreLib (in the added fallback logic), so I believe the handling of it was more targeted to avoiding extra work in that scenario of just looking for .dll and not .ni.dll. For longer term / 6.0, I expect it will be refactored away per #42435 (comment).

agocke · 2020-09-30T17:53:33Z

@janvorli Any more comments?

janvorli · 2020-09-30T23:32:10Z

@agocke no, I don't. Things look reasonable, but I am not much familiar with the code being updated.

agocke · 2020-09-30T23:33:13Z

Understandable, thanks for the input!

@vitek-karas @mateoatr @elinor-fung Ready to merge?

vitek-karas · 2020-09-30T23:41:10Z

I read through it once more - looks good to me.

elinor-fung · 2020-09-30T23:44:46Z

Looks good to me as well.

mateoatr · 2020-09-30T23:47:51Z

agocke · 2020-09-30T23:54:34Z

/backport to release/5.0

github-actions · 2020-09-30T23:55:39Z

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/281381356

github-actions · 2020-09-30T23:57:38Z

@agocke backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Extract bundled files when IncludeAllContentForSelfExtract is set
Applying: Don't pass bundle probe to the runtime for netcoreapp3 compat mode
error: sha1 information is lacking or useless (src/installer/corehost/cli/bundle/file_entry.h).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Don't pass bundle probe to the runtime for netcoreapp3 compat mode
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

…tnet#42435) Backport of dotnet#42435 to release/5.0. (cherry picked from commit 0be66cb)

Extract bundled files when IncludeAllContentForSelfExtract is set

a2feaec

mateoatr added the area-Single-File label Sep 18, 2020

mateoatr requested review from agocke, vitek-karas and elinor-fung September 18, 2020 05:47

agocke reviewed Sep 18, 2020

View reviewed changes

Mateo Torres Ruiz and others added 3 commits September 21, 2020 00:22

Don't pass bundle probe to the runtime for netcoreapp3 compat mode

83a1109

CodeBaseThrows should fail

788e046

mateoatr requested a review from agocke September 21, 2020 20:10

vitek-karas added 2 commits September 21, 2020 15:04

Normalize app base path

459c10e

Merge branch 'netCoreApp3Compat' of https://github.com/mateoatr/runtime…

2917ca5

… into netCoreApp3Compat

vitek-karas approved these changes Sep 21, 2020

View reviewed changes

elinor-fung reviewed Sep 22, 2020

View reviewed changes

vitek-karas requested a review from jkotas September 22, 2020 15:55

vitek-karas mentioned this pull request Sep 23, 2020

Keep deps.json in APP_CONTEXT_DEPS_FILE for .NET Core 3 compat mode #42611

Closed

Search for CoreLib in TPA

42bc99b

Merge pull request #2 from vitek-karas/CoreLibFromTPA

02b975d

Search for CoreLib in TPA

Fix the reported path to .deps.json for back compat mode

7ec7fd3

The path must point to the extraction folder and not use the "fake" in-bundle path.

elinor-fung mentioned this pull request Sep 26, 2020

[Single-File] DllImport fails to find native library #42772

Closed

Fix a test bug on Linux

19ea932

Actually we should be using path separator char in the APP_CONTEXT_DEPS_FILES, but that's something we won't be able to change really (too much chance of breaking something without too much value).

jkotas approved these changes Sep 29, 2020

View reviewed changes

janvorli reviewed Sep 29, 2020

View reviewed changes

agocke merged commit 0be66cb into dotnet:master Sep 30, 2020

agocke pushed a commit to agocke/runtime that referenced this pull request Oct 1, 2020

Extract bundled files when IncludeAllContentForSelfExtract is set (do…

7616c4e

…tnet#42435) Backport of dotnet#42435 to release/5.0. (cherry picked from commit 0be66cb)

This was referenced Oct 1, 2020

[release/5.0] Extract bundled files when IncludeAllContentForSelfExtract is set #42934

Merged

IsolatedStorage doesn't work from Single-File application in 5.0 #42265

Closed

hoyosjs mentioned this pull request Oct 9, 2020

R2R jobs are failing with Fatal error. Internal CLR error. (0x80131506) #43198

Closed

This was referenced Oct 9, 2020

[5.0.100-rc.2.20464.10] Single File Publish issue on Greenshot App after retarget .NET5 #42304

Closed

[5.0.100-rc.2.20471.7] Single File for OpenMU/HandyControl/TuringTrader target 5.0 app, tests failed #42665

Closed

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

Extract bundled files when IncludeAllContentForSelfExtract is set #42435

Extract bundled files when IncludeAllContentForSelfExtract is set #42435

Conversation

mateoatr commented Sep 18, 2020

ghost commented Sep 18, 2020

agocke Sep 18, 2020

Choose a reason for hiding this comment

vitek-karas Sep 18, 2020

Choose a reason for hiding this comment

agocke Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

elinor-fung Sep 18, 2020

Choose a reason for hiding this comment

vitek-karas Sep 21, 2020

Choose a reason for hiding this comment

vitek-karas commented Sep 21, 2020

elinor-fung left a comment • edited Loading

Choose a reason for hiding this comment

elinor-fung commented Sep 22, 2020

vitek-karas commented Sep 22, 2020

vitek-karas commented Sep 22, 2020

vitek-karas commented Sep 22, 2020

jkotas commented Sep 22, 2020 • edited Loading

elinor-fung commented Sep 22, 2020

jkotas commented Sep 22, 2020

vitek-karas commented Sep 23, 2020

jkotas commented Sep 23, 2020

elinor-fung commented Sep 23, 2020

elinor-fung commented Sep 25, 2020

vitek-karas commented Sep 25, 2020

vitek-karas commented Sep 25, 2020

elinor-fung commented Sep 25, 2020

vitek-karas commented Sep 25, 2020

vitek-karas commented Sep 28, 2020

agocke commented Sep 28, 2020

jkotas commented Sep 29, 2020

janvorli Sep 29, 2020

Choose a reason for hiding this comment

elinor-fung Sep 29, 2020

Choose a reason for hiding this comment

agocke commented Sep 30, 2020

janvorli commented Sep 30, 2020

agocke commented Sep 30, 2020

vitek-karas commented Sep 30, 2020

elinor-fung commented Sep 30, 2020

mateoatr commented Sep 30, 2020

agocke commented Sep 30, 2020

github-actions bot commented Sep 30, 2020

github-actions bot commented Sep 30, 2020

agocke Sep 18, 2020 •

edited

Loading

elinor-fung left a comment •

edited

Loading

jkotas commented Sep 22, 2020 •

edited

Loading