Implement Bundler version 2 #33413

swaroop-sridhar · 2020-03-10T08:47:59Z

Implement support for single-file version 2 layout as described in https://github.com/dotnet/designs/blob/master/accepted/2020/single-file/bundler.md

The supporting changes are:

Implement new HostModel interfaces to communicate additional information from the SDK to the HostModel
- The SDK changes will be done in an upcoming PR.
- Some depricated APIs are maintained so that SDK build doesn't break in the meantime]
Handle various bundling options described in https://github.com/dotnet/designs/blob/master/accepted/2020/single-file/design.md#optional-settings
- This requires ability to recognize native binaries for various architectures.
- Added ability to minimaly parse ELF binaries. PE/MachO support already exists.
- Refactored out PE processing from BinaryUtils, so that PE-ELF-MachO parsers have similar abstractions.
Create bundles with the appropriate layout (v1 for netcoreapp3.0, v2 for net5)
Consume the new layout from the host bundle process/extraction code.
Test cases

Implement support for single-file version 2 layout as described in https://github.com/dotnet/designs/blob/master/accepted/2020/single-file/bundler.md The supporting changes are: * Implement new HostModel interfaces to communicate additional information from the SDK to the HostModel * The SDK changes will be done in an upcoming PR. * Some depricated APIs are maintained so that SDK build doesn't break in the meantime] * Handle various bundling options described in https://github.com/dotnet/designs/blob/master/accepted/2020/single-file/design.md#optional-settings * This requires ability to recognize native binaries for various architectures. * Added ability to minimaly parse ELF binaries. PE/MachO support already exists. * Refactored out PE processing from BinaryUtils, so that PE-ELF-MachO parsers have similar abstractions. * Create bundles with the appropriate layout (`v1` for `netcoreapp3.0`, `v2` for `net5`) * Consume the new layout from the host bundle process/extraction code. * Test cases

Some of the AppHost rewriter files imported from the SDK repo had the license banner in a differnt format. Make them consistent with rest of the files in the installer partition.

swaroop-sridhar · 2020-03-10T08:54:44Z

CC: @wli3 @dsplaisted

vitek-karas

General concern: The bundler "blindly" uses file extensions and probing to determine the behavior for a file. There doesn't seem to be a way to override this behavior.
Is there a way (combined with SDK and such) to do things like:

Include a select native library as a "data file" - so that it's bundled but not extracted
Include file with .pdb extension as a "data file" - bundled but not extracted - when combined with "don't bundle symbols" setting

I'm not sure we need to support these scenarios, but I think we should be explicit about whatever behavior we chose. Maybe the guidance for these cases would be to embed such files as managed resources instead?

vitek-karas · 2020-03-10T11:05:19Z

src/installer/managed/Microsoft.NET.HostModel/Bundle/BundleOptions.cs

+    /// BundleOptions: Optional settings for configuring the type of files
+    ///                included in the single file bundle.


Suggested change

/// BundleOptions: Optional settings for configuring the type of files

/// included in the single file bundle.

/// Optional settings for configuring the type of files

/// included in the single file bundle.

src/installer/managed/Microsoft.NET.HostModel/Bundle/BundleOptions.cs

src/installer/managed/Microsoft.NET.HostModel/Bundle/Bundler.cs

src/installer/managed/Microsoft.NET.HostModel/Bundle/Manifest.cs

src/installer/managed/Microsoft.NET.HostModel/Bundle/TargetInfo.cs

...icrosoft.NET.HostModel.Tests/Microsoft.NET.HostModel.Bundle.Tests/BundlerConsistencyTests.cs

vitek-karas · 2020-03-10T11:28:51Z

src/installer/managed/Microsoft.NET.HostModel/Bundle/TargetInfo.cs

+                    DefaultOptions = BundleOptions.BundleAllContent;
+                    break;
+
+                case "net5":


Not sure how the discussion around .NET 5 TFMs ended up (if it already happened), but I vaguely remember that there were mote than one .NET 5 TFM. Will the SDK make sure to always pass "net5" for all such TFMs, or should we support all the other .NET 5 TFMS here?

Spec and discussion here: dotnet/designs#92

Yes, there are multiple TFMs, it is better to use the pre-parsed identifiers and versions.

I wrote down the framework strings to make it evident that the bundle versions are tied to TFMs. I'll change it to accept the version number only (3, 3.1, 5). This way, the SDK can efficiently pass the TargetFrameworkVersionWithoutV as input.

Nit: ">= 5" for the last case. :-) I'm sure there is going to be some next version at some point and we want it to default to the new code path.

vitek-karas · 2020-03-10T11:45:31Z

src/installer/corehost/cli/apphost/bundle/header.h

+    {
+        none = 0,
+        needs_extraction = 1,
+        netcoreapp3_compat_mode = 2


To be honest I don't like the compat mode flag. Can't we deduce it from the missing V2 header instead? I find it really weird that we would have the new bundle, but try to mimic the old behavior.
Basically - I would expact behavior like this:

3.* SDK will always use 3.* apphost and also V1 header only

5.* SDK building netcoreapp3.* app will use 3.* apphost and V1 header only

5.* SDK building net5 app will always use 5.* apphost and V2 header

The presence of the flag means that there will be a combination of 5.* apphost used on netcoreapp3.* - I find that wrong.

I agree that apphosts will always match the TFM, exactly as you've noted above, regardless of the SDK.

The netcoreapp3_compat_mode flag is set on a .net5 app, which chooses to build single-file apps in .netcore3.x compat mode, through the IncludeAllContentInSingleFile setting.

This means:

All files are bundled into the app; some of them will be extracted to disk.

AppContext.BaseDirectory is set to the extraction directory. Please see the associated design and discussion.

In that case, can you please add a comment into the code describing at least roughly what it does?
Do we think we will "get rid of it" in 6 - or deprecate it? I know we can't remove the bit from the struct, but maybe in 6 it would always be false or something. I would very much like to have a strategy how to get rid of it 😄

We can certainly deprecate it in upcoming .net versions when:

A static host with linked runtime is available on all platforms, (for self-contained single-file apps). This is not available for Windows in .net5.

With the upcoming corssgen2, we'll likely have cross platform native tools available for linking user's native dependencies.

src/installer/corehost/cli/apphost/bundle/header.h

lpereira · 2020-03-10T16:15:18Z

src/installer/corehost/cli/apphost/bundle/header.cpp

+bool header_fixed_t::is_valid(bool exact_match) const
 {
+    // The AppHost expects the bundle_header to be an exact_match for which it was built.
+    // The framework accepts backwards compatible header versions.
+
    return num_embedded_files > 0 &&
-           ((major_version < header_t::major_version) ||
-            (major_version == header_t::major_version && minor_version <= header_t::minor_version));
+          (exact_match) ?
+              (major_version == header_t::major_version) && (minor_version == header_t::minor_version) :
+              ((major_version < header_t::major_version) ||
+                   (major_version == header_t::major_version && minor_version <= header_t::minor_version));
+
 }


Can you write this in multiple if statements with an early return style? This is starting to get difficult to read.

Also, please check if the version in the header is either 1 or 2 before any other test, and return false otherwise. Just a sanity check before making any assumptions.

I've rewritten the check using if() {} else {} blocks.

Also, please check if the version in the header is either 1 or 2 before any other test

I think this check is implicit, because major_version is a uint, that's bounded by header.major_version -- the constant 2.

src/installer/managed/Microsoft.NET.HostModel/AppHost/ElfUtils.cs

lpereira · 2020-03-10T16:27:46Z

src/installer/managed/Microsoft.NET.HostModel/AppHost/ElfUtils.cs

+
+        unsafe public static bool IsElfImage(string filePath)
+        {
+            using (var mappedFile = MemoryMappedFile.CreateFromFile(filePath, FileMode.Open, mapName: null, capacity: 0, MemoryMappedFileAccess.Read))


Might be better for this function to use a good ol' read() instead, as you're only interested in reading 4 bytes of a file. You won't probably even need this method to be unsafe also.

Thanks @lpereira, I've fixed this for all three binary utils now.

swaroop-sridhar · 2020-03-10T20:47:23Z

General concern: The bundler "blindly" uses file extensions and probing to determine the behavior for a file. There doesn't seem to be a way to override this behavior.
Is there a way (combined with SDK and such) to do things like:

Include a select native library as a "data file" - so that it's bundled but not extracted

Include file with .pdb extension as a "data file" - bundled but not extracted - when combined with "don't bundle symbols" setting

I'm not sure we need to support these scenarios, but I think we should be explicit about whatever behavior we chose. Maybe the guidance for these cases would be to embed such files as managed resources instead?

@vitek-karas: For files other than assemblies and some config files, there are only three possible outcomes:

Embed them as resources -- as you've recommended above
Leave them beside the app -- the SDK today supports the ExcludeFromSingleFile annotation on any file, which can be used to achieve this purpose,
Use .net core 3 compatibility mode through IncludeAllContentInSingleFile in which case they are all bundled and extracted to disk, (So, specific type doesn't matter).

We can certainly add support for specifying types in the input FileSpecs but I don't think it is necessary. We can come back to it if there's a customer scenario.

dsplaisted · 2020-03-10T21:27:30Z

We've had bugs in the past in cases where we relied on file extensions for this type of thing. I believe the information about how the files are being used is available via the MSBuild item types or metadata in the build, so you should theoretically be able to pass it down.

swaroop-sridhar · 2020-03-10T21:53:16Z

@dsplaisted can you please elaborate about what information is available from meta-data?

We use filename/extension only in the following two cases:

We recognize the app's deps.json/runtimeconfig.json files by name-- which I think is reasonable.
For .pdb files:

The SDK recognizes the app's PDB by name. Actually, no processing is necessary for app.pdb in
the HostModel because the SDK doesn't pass it down at all (when IncludeSymbolsInSingleFile is
not set).
The HostModel recognizes any other .pdb files passed in and excludes them too. I spoke with
@tmat last year about recognizing PDB files, and he'd recommended using the extension in this
case.

I think a reasonable approach to take here is for HostModel to not treat symbols files specially at all:

The app.pdb case is handled by the SDK already
All other PDB files will be treated as content files and left behind -- that is, IncludeSymbolsInSingleFile will have no impact on these.

I think IncludeSymbolsInSingleFile is not a very useful switch; we're just keeping it for compatibility with netcoreapp3 settings.

vitek-karas · 2020-03-10T22:01:33Z

I think IncludeSymbolsInSingleFile is not a very useful switch; we're just keeping it for compatibility with netcoreapp3 settings.

While I might not like it - I think we will be surprised:

We have a bug in the trimming (ILLinker) which makes it so that stack traces in exceptions don't get source line info even in debug builds. First time I learned about this I was like "Why do you trim the debug build" - but the answer is basically "We want to do the same everywhere". Another answer I got was - it's not a debug build - "we want source line info in production".

So I would not be surprised to see similar asks for source line info in single-file - which requires pdbs.

swaroop-sridhar · 2020-03-10T22:07:02Z

I think IncludeSymbolsInSingleFile is not a very useful switch; we're just keeping it for compatibility with netcoreapp3 settings.

While I might not like it - I think we will be surprised:

We have a bug in the trimming (ILLinker) which makes it so that stack traces in exceptions don't get source line info even in debug builds. First time I learned about this I was like "Why do you trim the debug build" - but the answer is basically "We want to do the same everywhere". Another answer I got was - it's not a debug build - "we want source line info in production".

So I would not be surprised to see similar asks for source line info in single-file - which requires pdbs.

The recommended solution for this is to build IL with embedded PDBs.
I'd like to see this flag also go away in subsequent releases.

dsplaisted · 2020-03-11T02:44:40Z

@dsplaisted can you please elaborate about what information is available from meta-data?

@swaroop-sridhar What you've described sounds reasonable. I think where we ran into trouble before was where we were trying to use a heuristic to determine if an asset was a native library, where the extension varies by platform (and I'm not sure if it's a fixed set).

swaroop-sridhar · 2020-03-11T07:26:50Z

I think IncludeSymbolsInSingleFile is not a very useful switch; we're just keeping it for compatibility with netcoreapp3 settings.

While I might not like it - I think we will be surprised:
We have a bug in the trimming (ILLinker) which makes it so that stack traces in exceptions don't get source line info even in debug builds. First time I learned about this I was like "Why do you trim the debug build" - but the answer is basically "We want to do the same everywhere". Another answer I got was - it's not a debug build - "we want source line info in production".
So I would not be surprised to see similar asks for source line info in single-file - which requires pdbs.

The recommended solution for this is to build IL with embedded PDBs.
I'd like to see this flag also go away in subsequent releases.

I complete agree that we should generate correct PDBs -- but don't know of a case where the PDB should be bundled within the single-file and cannot use IL-embedded-PDBs.

Given this, and the fact that we're trying to move away from extraction, I suggest that we only support IncludeSymbolsInSingleFile when IncludeAllContentInSingleFile (which is netcoreapp3.0 compatibility mode).

vitek-karas · 2020-03-11T09:57:35Z

Is there a simple way to tell the build system to include the symbols in the IL? I must admit that this is the first time I've heard about this possibility.

src/installer/managed/Microsoft.NET.HostModel/Bundle/Bundler.cs

src/installer/managed/Microsoft.NET.HostModel/Bundle/Manifest.cs

src/installer/managed/Microsoft.NET.HostModel/Bundle/TargetInfo.cs

swaroop-sridhar · 2020-03-11T19:47:41Z

Is there a simple way to tell the build system to include the symbols in the IL? I must admit that this is the first time I've heard about this possibility.

I was talking about the csc -debug:embedded ... option, which embeds the PDB within the target .dll. This only works for portable PDBs. If the developer wants to bundle Windows PDBs, then they'll have to fall back to IncludeSymbolsInSingleFile.

Also fix a test failure

swaroop-sridhar · 2020-03-12T01:06:01Z

@lpereira I've addressed your feedback in d844b72. Please let me know if you have any other comments. Thanks.

lpereira · 2020-03-12T03:49:44Z

src/installer/managed/Microsoft.NET.HostModel/AppHost/ElfUtils.cs

+                    return false;
+                }
+
+                byte[] eIdent = reader.ReadBytes(4);


Newbie C# question: can you read directly on a struct ElfHeader and just return header.IsValid()?

Binary Reader/Writer doesn't support structs (even explicit layout).
I looked through the docs, but couldn't find a way to do it.

So, I decided to just read the 4 bytes; we anyway didn't have the full header structure coded in this class -- just enough to check the magic bytes in the beginning.

lpereira

LGTM with a question

Update GenerateBundle task and a few others to use the new APIs in HostModel intruduced by dotnet/runtime#33413. This change is in preparation for moving to new PublishSingleFile semantics in .net 5. In GenerateBundle task, the SDK constructs the bundler with `BundleAllContent` option for all builds. This keeps the behavior of PublishSingleFile unchanged until Host/Runtime components of the feature are ready.

Some depricated APIs were maintained in dotnet#33413 so that SDK build is green. Now that the SDK is updated dotnet/sdk#10849, remove the unused APIs. Also move out a test-only methods from the product to the test helpers.

Some depricated APIs were maintained in #33413 so that SDK build is green. Now that the SDK is updated dotnet/sdk#10849, remove the unused APIs. Also move out a test-only methods from the product to the test helpers.

Dotnet-GitSync-Bot added the area-HostModel Microsoft.NET.HostModel issues label Mar 10, 2020

Make License headers consistent

8e34bef

Some of the AppHost rewriter files imported from the SDK repo had the license banner in a differnt format. Make them consistent with rest of the files in the installer partition.

swaroop-sridhar requested review from lpereira and vitek-karas March 10, 2020 08:54

vitek-karas reviewed Mar 10, 2020

View reviewed changes

lpereira reviewed Mar 10, 2020

View reviewed changes

src/installer/managed/Microsoft.NET.HostModel/AppHost/ElfUtils.cs Outdated Show resolved Hide resolved

lpereira reviewed Mar 10, 2020

View reviewed changes

Address feedback by @vitek-karas

53d47c0

vitek-karas approved these changes Mar 11, 2020

View reviewed changes

swaroop-sridhar added 3 commits March 11, 2020 12:58

Use System.Version for TFMVersion input

0cef6cc

Also fix a test failure

Address feedback by @lpereira

d844b72

Fix a test failure

39dbad1

lpereira reviewed Mar 12, 2020

View reviewed changes

lpereira approved these changes Mar 12, 2020

View reviewed changes

swaroop-sridhar merged commit 7f52377 into dotnet:master Mar 12, 2020

swaroop-sridhar mentioned this pull request Mar 12, 2020

Use new HostModel APIs dotnet/sdk#10849

Merged

swaroop-sridhar mentioned this pull request Mar 13, 2020

HostModel: Remove deprecated API #33545

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Bundler version 2 #33413

Implement Bundler version 2 #33413

swaroop-sridhar commented Mar 10, 2020

swaroop-sridhar commented Mar 10, 2020

vitek-karas left a comment

vitek-karas Mar 10, 2020

vitek-karas Mar 10, 2020

filipnavara Mar 10, 2020 •

edited

Loading

swaroop-sridhar Mar 10, 2020 •

edited

Loading

filipnavara Mar 10, 2020

vitek-karas Mar 10, 2020

swaroop-sridhar Mar 10, 2020

vitek-karas Mar 10, 2020

swaroop-sridhar Mar 10, 2020

lpereira Mar 10, 2020

lpereira Mar 10, 2020

swaroop-sridhar Mar 11, 2020

lpereira Mar 10, 2020

swaroop-sridhar Mar 11, 2020

swaroop-sridhar commented Mar 10, 2020

dsplaisted commented Mar 10, 2020

swaroop-sridhar commented Mar 10, 2020 •

edited

Loading

vitek-karas commented Mar 10, 2020

swaroop-sridhar commented Mar 10, 2020

dsplaisted commented Mar 11, 2020

swaroop-sridhar commented Mar 11, 2020

vitek-karas commented Mar 11, 2020 •

edited by swaroop-sridhar

Loading

swaroop-sridhar commented Mar 11, 2020

swaroop-sridhar commented Mar 12, 2020

lpereira Mar 12, 2020

swaroop-sridhar Mar 12, 2020

lpereira left a comment

		/// BundleOptions: Optional settings for configuring the type of files
		/// included in the single file bundle.

Implement Bundler version 2 #33413

Implement Bundler version 2 #33413

Conversation

swaroop-sridhar commented Mar 10, 2020

swaroop-sridhar commented Mar 10, 2020

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

filipnavara Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

swaroop-sridhar Mar 10, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swaroop-sridhar commented Mar 10, 2020

dsplaisted commented Mar 10, 2020

swaroop-sridhar commented Mar 10, 2020 • edited Loading

vitek-karas commented Mar 10, 2020

swaroop-sridhar commented Mar 10, 2020

dsplaisted commented Mar 11, 2020

swaroop-sridhar commented Mar 11, 2020

vitek-karas commented Mar 11, 2020 • edited by swaroop-sridhar Loading

swaroop-sridhar commented Mar 11, 2020

swaroop-sridhar commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira left a comment

Choose a reason for hiding this comment

filipnavara Mar 10, 2020 •

edited

Loading

swaroop-sridhar Mar 10, 2020 •

edited

Loading

swaroop-sridhar commented Mar 10, 2020 •

edited

Loading

vitek-karas commented Mar 11, 2020 •

edited by swaroop-sridhar

Loading