Single-File: Run from Bundle #36052

swaroop-sridhar · 2020-05-07T22:32:12Z

This change implements:

Runtime changes necessary to load assemblies directly from the bundle:
- Design notes about Load from Bundle
- Most of these changes are directly from [Single-Exe] Run from bundle coreclr#26504 and [single-exe] Run from bundle on Windows coreclr#26904
Hostpolicy change to not add bundled assemblies to TPA list:
- Design notes about Dependency Resolution
- TBD (separately) items: Fix for hammer servicing Single-File: Servicing locations should take precedence over bundle contents #36031

Fixes #32822

ghost · 2020-05-07T22:32:14Z

Tagging subscribers to this area: @swaroop-sridhar
Notify danmosemsft if you want to be subscribed.

swaroop-sridhar · 2020-05-07T22:40:09Z

CC: @jkotas

jkotas · 2020-05-08T01:00:07Z

src/coreclr/src/binder/assemblybinder.cpp

+            // If found, the assembly is loaded from the bundle.  
+            if (Bundle::AppIsBundle())
+            {
+                SString candidates[] = { W(".dll"), W(".ni.dll") };


Do we still use the .ni.dll suffix for anything?

If we do decide to keep it it should use the same order as in BindAssemblyByProbingPaths which is that .ni.dll is preferred over .dll. (or rather BindAssemblyByProbingPaths is first called with useNativeImages=true).

@davidwrighton mentioned here that .ni.dll is used for managed C++ R2R. Has this changed?

Well, I think its still probably the way we should make it work. I'm not sure if it works in practice today though. I know it doesn't work in crossgen2.

jkotas · 2020-05-08T01:08:51Z

src/coreclr/src/binder/assemblybinder.cpp

-        StackSString sCoreLib;
+        StackSString sCoreLib(sCoreLibDir);
+        StackSString coreLibName(CoreLibName_IL_W);
+        sCoreLib.Append(coreLibName);


Is this fake path going to be actually used for anything in the bundle case? Would it be better to not build it and pass it in at all when we find the payload in the bundle?

vitek-karas

Aside from the tracing just minor stuff.

vitek-karas · 2020-05-11T18:10:12Z

src/coreclr/src/binder/assemblybinder.cpp

-        sCoreLib.Append(CoreLibName_IL_W);
+        // System.Private.CoreLib.dll is expected to be found at one of the following locations:
+        //   * Non-single-file app: In systemDirectory, beside coreclr.dll
+        //   * Framework-dependent single-file app: In system directory, beside coreclr.dll


Suggested change

// * Framework-dependent single-file app: In system directory, beside coreclr.dll

// * Framework-dependent single-file app: In systemDirectory, beside coreclr.dll

vitek-karas · 2020-05-11T18:27:28Z

src/coreclr/src/binder/assemblybinder.cpp

+            // If found, the assembly is loaded from the bundle.  
+            if (Bundle::AppIsBundle())
+            {
+                SString candidates[] = { W(".dll"), W(".ni.dll") };


If we do decide to keep it it should use the same order as in BindAssemblyByProbingPaths which is that .ni.dll is preferred over .dll. (or rather BindAssemblyByProbingPaths is first called with useNativeImages=true).

vitek-karas · 2020-05-11T19:33:44Z

src/coreclr/src/binder/assemblybinder.cpp

+                        &pTPAAssembly,
+                        NULL,  // szMDAssemblyPath
+                        Bundle::ProbeAppBundle(assemblyFileName, /* pathIsBundleRelative */ true));
+


We should trace the attempt, just like below whenever we probe we also trace the attempt.

vitek-karas · 2020-05-11T19:34:21Z

src/coreclr/src/binder/assemblybinder.cpp

+                        // Any other error is fatal
+                        IF_FAIL_GO(hr);
+
+                        if (TestCandidateRefMatchesDef(pRequestedAssemblyName, pTPAAssembly->GetAssemblyName(), true /*tpaListAssembly*/))


And similarly here, if this fails we should trace that - look below for an example.

vitek-karas · 2020-05-11T21:12:43Z

src/coreclr/src/vm/appdomain.cpp

@@ -1775,27 +1775,27 @@ void SystemDomain::Init()
    m_pSystemAssembly = NULL;

    DWORD size = 0;
-
+    AppDomain* pAppDomain = ::GetAppDomain();


Is this necessary? I don't see it used anywhere.

vitek-karas · 2020-05-11T21:14:57Z

src/coreclr/src/vm/appdomain.hpp

@@ -29,6 +29,7 @@
 #include "gchandleutilities.h"
 #include "../binder/inc/applicationcontext.hpp"
 #include "rejit.h"
+#include "bundle.h"


Given that there are no changes in appdomain.cpp do we need the include here?

vitek-karas · 2020-05-11T21:29:08Z

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

@@ -281,10 +281,13 @@ void deps_resolver_t::setup_additional_probes(const std::vector<pal::string_t>&
 *   -- When servicing directories are looked up, look up only if the deps file marks the entry as serviceable.
 *   -- When a deps json based probe is performed, the deps entry's package name and version must match.
 *   -- When looking into a published dir, for rid specific assets lookup rid split folders; for non-rid assets lookup the layout dir.
+ * The path to the resolved file is returned in candidate out parameter
+ * If the candidate is embedded within the single-file bundle (rather than an actual file on disk), found_in_bundle will be set to true.


Suggested change

* If the candidate is embedded within the single-file bundle (rather than an actual file on disk), found_in_bundle will be set to true.

* If the candidate is embedded within the single-file bundle (rather than an actual file on disk), loaded_from_bundle will be set to true.

janvorli · 2020-05-11T22:01:44Z

src/coreclr/src/vm/peimage.cpp

-        {
-            bIsFlatLayoutSuitable = FALSE;
-        }
+        bIsFlatLayoutSuitable = IsInBundle() || !bIsMappedLayoutSuitable;


This doesn't look right. We are completely discarding the previously computed value of the bIsFlatLayoutSuitable.

swaroop-sridhar · 2020-05-13T16:15:35Z

Implemented few of the suggested changes, remaining to come shortly.

This change implements: * Runtime changes necessary to load assemblies directly from the bundle: * Design notes about [Load from Bundle](https://github.com/dotnet/designs/blob/master/accepted/2020/single-file/design.md#peimage-loader) * Most of these changes are directly from dotnet/coreclr#26504 and dotnet/coreclr#26904 * Hostpolicy change to not add bundled assemblies to TPA list: * Design notes about [Dependency Resolution](https://github.com/dotnet/designs/blob/master/accepted/2020/single-file/design.md#dependency-resolution) * TBD (separately) items: Fix for hammer servicing dotnet#36031 Fixes dotnet#32822

swaroop-sridhar · 2020-05-22T09:21:34Z

@jkotas, @vitek-karas @janvorli Thanks for the comments. I've fixed these issues now.

* Fix Assembly.ni.dll and Assembly.dll path usage. * Remove unused full-path computation for corelib when loading from bundle. * Add tracing when loading from bundle. * Add bundle-probing for satellite assemblies. * Adapt the BundleRename test to the .net5 scenario where assemblies are loaded from bundle.

swaroop-sridhar added the area-Single-File label May 7, 2020

swaroop-sridhar added this to the 5.0 milestone May 7, 2020

swaroop-sridhar requested a review from vitek-karas May 7, 2020 22:32

swaroop-sridhar requested review from janvorli and VSadov May 7, 2020 22:32

jkotas reviewed May 8, 2020

View reviewed changes

vitek-karas approved these changes May 11, 2020

View reviewed changes

janvorli reviewed May 11, 2020

View reviewed changes

swaroop-sridhar mentioned this pull request May 16, 2020

Support Single-File Apps in .NET 5 #36590

Closed

30 tasks

swaroop-sridhar added 2 commits May 18, 2020 00:53

Address some code review feedback

10732fe

swaroop-sridhar force-pushed the Run branch 2 times, most recently from 1b46420 to 7cc1384 Compare May 22, 2020 09:19

swaroop-sridhar force-pushed the Run branch 2 times, most recently from 31bb483 to 368b189 Compare May 25, 2020 22:16

swaroop-sridhar force-pushed the Run branch from 368b189 to c08d909 Compare May 26, 2020 15:28

swaroop-sridhar merged commit f9d3e6f into dotnet:master May 26, 2020

swaroop-sridhar deleted the Run branch May 26, 2020 21:10

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Single-File: Run from Bundle #36052

Single-File: Run from Bundle #36052

swaroop-sridhar commented May 7, 2020

ghost commented May 7, 2020

swaroop-sridhar commented May 7, 2020

jkotas May 8, 2020

vitek-karas May 11, 2020

swaroop-sridhar May 13, 2020

davidwrighton May 19, 2020

jkotas May 8, 2020 •

edited

Loading

vitek-karas left a comment

vitek-karas May 11, 2020

vitek-karas May 11, 2020

vitek-karas May 11, 2020

vitek-karas May 11, 2020

vitek-karas May 11, 2020

vitek-karas May 11, 2020

vitek-karas May 11, 2020

janvorli May 11, 2020

swaroop-sridhar commented May 13, 2020

swaroop-sridhar commented May 22, 2020

	// * Framework-dependent single-file app: In system directory, beside coreclr.dll
	// * Framework-dependent single-file app: In systemDirectory, beside coreclr.dll

	* If the candidate is embedded within the single-file bundle (rather than an actual file on disk), found_in_bundle will be set to true.
	* If the candidate is embedded within the single-file bundle (rather than an actual file on disk), loaded_from_bundle will be set to true.

Single-File: Run from Bundle #36052

Single-File: Run from Bundle #36052

Conversation

swaroop-sridhar commented May 7, 2020

ghost commented May 7, 2020

swaroop-sridhar commented May 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas May 8, 2020 • edited Loading

Choose a reason for hiding this comment

vitek-karas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swaroop-sridhar commented May 13, 2020

swaroop-sridhar commented May 22, 2020

jkotas May 8, 2020 •

edited

Loading