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

Implement Single-file Bundler #5286

Merged
merged 10 commits into from Mar 15, 2019

Conversation

Projects
None yet
9 participants
@swaroop-sridhar
Copy link
Contributor

commented Mar 1, 2019

This change implements the single-file bundler, according to this
design document

The change adds a test-case to bundle the contents of a
self-contained console app, extract the contents, and run the app.

Other tests will be added once the host-changes to process the bundle
are implemented.

Issue: dotnet/coreclr#20287

@swaroop-sridhar swaroop-sridhar force-pushed the swaroop-sridhar:bundler branch from 764018f to 70aa1d4 Mar 1, 2019

@swaroop-sridhar swaroop-sridhar requested review from sbomer, vitek-karas and jkotas Mar 1, 2019

Implement Single-file Bundler
This change implements the single-file bundler, according to this
[design document](https://github.com/dotnet/designs/blob/master/accepted/single-file/design.md)

The change adds a test-case to bundle the contents of a
self-contained console app, extract the contents, and run the app.

Other tests will be added once the host-changes to process the bundle
are implemented.

Issue: dotnet/coreclr#20287

@swaroop-sridhar swaroop-sridhar force-pushed the swaroop-sridhar:bundler branch from 70aa1d4 to cf5a946 Mar 2, 2019

@vitek-karas
Copy link
Member

left a comment

I think this tool belongs to either dotnet/sdk or dotnet/cli.
What would be the delivery mechanism if we keep it in dotnet/core-setup?

Also for consideration, maybe we should make it into a dotnet tool - so it would be used like dotnet bundle.

using System.Reflection.PortableExecutable;
using System.Linq;

namespace SingleFile

This comment has been minimized.

Copy link
@vitek-karas

vitek-karas Mar 4, 2019

Member

Naming: Typically the root namespace should have the same name as the assembly, so that would be bundle, or maybe in this case something a bit more namespace like: Microsoft.NET.Build.Bundle or something like that.

This comment has been minimized.

Copy link
@swaroop-sridhar

swaroop-sridhar Mar 6, 2019

Author Contributor

I renamed the bundler package to Microsoft.NET.Build.Bundle.
CC: @KathleenDollard @richlander for naming.

Show resolved Hide resolved src/managed/tools/bundle/bundle.cs Outdated
Show resolved Hide resolved src/managed/tools/bundle/bundle.cs Outdated
Show resolved Hide resolved src/managed/tools/bundle/bundle.cs Outdated
Show resolved Hide resolved src/managed/tools/bundle/bundle.cs Outdated
Show resolved Hide resolved src/managed/tools/bundle/bundle.cs Outdated
Show resolved Hide resolved src/managed/tools/bundle/bundle.cs Outdated
Console.WriteLine(" [-?] Display usage information");
}

static void ParseArgs(string[] args)

This comment has been minimized.

Copy link
@vitek-karas

vitek-karas Mar 4, 2019

Member

Do we have an existing command line parser we could use instead? I think it's just unfortunate to have to reimplement all of this every time we write a new tool.

Maybe @richlander would know.

This comment has been minimized.

Copy link
@swaroop-sridhar

swaroop-sridhar Mar 6, 2019

Author Contributor

I don't know of a CoreFX library for command line processing.
The dotnet cli uses an inbuilt command line processor

The bundler's command line is very simple, so I think it's best to keep these few lines of parsing code.

This comment has been minimized.

Copy link
@nguerrera

nguerrera Mar 6, 2019

Member

There is System.CommandLine, which is really what you'd want to use here.

Adding @KathleenDollard and @jonsequitur

One consideration here: can we ship components in the .NET Core 3.0 SDK that use this API. I'm unaware of timelines, etc...

Show resolved Hide resolved src/managed/tools/bundle/bundle.cs Outdated
Show resolved Hide resolved src/managed/tools/bundle/bundle.cs Outdated
Name bundler as Microsoft.DotNet.Build.Bundle
This commit makes two modifications:
* Name the bundler as Microsoft.DotNet.Build.Bundle.dll
* Reorganize the bundler code to separate files.

@swaroop-sridhar swaroop-sridhar force-pushed the swaroop-sridhar:bundler branch from b523559 to 37339a8 Mar 6, 2019

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

I think this tool belongs to either dotnet/sdk or dotnet/cli.
We don't want to add custom tools into the SDK repo for reasons for SDK repo ownership/maintainence.
The msbuild task that invokes the bundler will live in the SDK repo, but the actual tool will live in core-setup. This arrangement is similar to ready-to-run, where crossgen lives in coreclr, whereas the build-task helper will be in SDK.

We could develop the bundler in CLI or core-setup repos. core-setup is better because the bundler is closely related to the AppHost. This will facilitate easier development and end-to-end testing of the feature in one repo. This issue was discussed here.

What would be the delivery mechanism if we keep it in dotnet/core-setup?
The Bundler will be a built as a package that will be imported into the toolset repo. From here, the tool will be included in the core-sdk. This arrangement is similar to the DependencyModel package.

CC: @nguerrera

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Also for consideration, maybe we should make it into a dotnet tool - so it would be used like dotnet bundle.

The design proposed an option to add the single-file bundling to Dotnet-cli here. But we agreed to start with keeping it separate from the dotnet tool here, and have a single-file build property instead. The dotnet CLI integration may be added in future.

@swaroop-sridhar swaroop-sridhar force-pushed the swaroop-sridhar:bundler branch from 79b72cd to 79c3171 Mar 6, 2019

@vitek-karas
Copy link
Member

left a comment

There's still plenty of comments in my previous review which I would like to see addressed, but GitHub marked them as outdated (since you moved the code around). Please go through those as well.

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

There's still plenty of comments in my previous review which I would like to see addressed, but GitHub marked them as outdated (since you moved the code around). Please go through those as well.

Sure, I wasn't done yet @vitek-karas :) I'm testing out more changes that I'll push soon. Thanks.

// Extract options:
static string BundleToExtract;

static void Usage()

This comment has been minimized.

Copy link
@swaroop-sridhar

swaroop-sridhar Mar 6, 2019

Author Contributor

@KathleenDollard @richlander
Can you please review the command line interface of the bundler tool?

The developers are only expected to interact with the bundler via the build-system, as explained here. But this is still a tool available to the public.

This comment has been minimized.

Copy link
@KathleenDollard

KathleenDollard Mar 11, 2019

Contributor

@swaroop-sridhar I'm sorry that I didn't realize this earlier - you are writing your own command line parsing? Is there a reason you are not using System.CommandLine? It will allow much easier maintenance and development for you. And provide help a consistent help format.

I'll do the specific things first, then the general:

  • All switches/options should be in Posix form with two dashes (--directory) and words dash separated
  • On occasion, where there is a very common switch an abbreviation is fine. But not for all switches.
  • Common CLI switches should be used where they appear, along with common abbreviations. Output is an example.
  • -v is --verbosity and has arguments for detailed, normal, etc. It's fine to have multiple verbosity levels display the same thing, but users can predict what to type.
  • We use --help and -h.

General: You are using a switch as a command. I think it is confusing to your users (unless I misunderstand your context) and makes it almost impossible to validate user input (free with System.CommandLine). You're not communicating that app host only makes sense on publish. I don't understand the problem space well enough, but it looks like

bundle publish --app-host -o
bundle extract -o

So your user gets help on the root, which tells them the two things they can do. Once they decide what they want to do, they get help on the right options. (See dotnet CLI help, or git help).

OK, that sounds like a parsing nightmare, I know. But @jonsequitur or I can get show you how to build your CLI in under an hour and then you can drop your parsing logic. You'll get help for free, validation if you want it and other features like tab completion if the user installs it.

I've been out sick, but I should be available most of this week - limited availability next week.

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@vitek-karas I've addressed all of your comments now. I've made the changes you suggested, except as noted in the comments. Thanks for the detailed review.

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@vitek-karas
Copy link
Member

left a comment

Looks good, thanks!

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@KathleenDollard @richlander
This PR is waiting for your review regarding naming/interface. Please take a look at the following two places, at your earliest convenience. Thanks.

// Allign assemblies, since they are loaded directly from bundle
if (type == FileType.Assembly)
{
long padding = AssemblyAlignment - (bundle.Position % AssemblyAlignment);

This comment has been minimized.

Copy link
@0xd4d

0xd4d Mar 11, 2019

padding will equal AssemblyAlignment if no alignment is needed, wasting AssemblyAlignment bytes.

Manifest manifest = new Manifest();

bundle.Position = bundle.Length;
foreach (string filePath in Directory.GetFiles(ContentDir))

This comment has been minimized.

Copy link
@0xd4d

0xd4d Mar 11, 2019

Should all input files be sorted so the output file is deterministic?

This comment has been minimized.

Copy link
@0xd4d

0xd4d Mar 11, 2019

What about files in sub directories? Shouldn't they also be bundled?

This comment has been minimized.

Copy link
@swaroop-sridhar

swaroop-sridhar Mar 11, 2019

Author Contributor

Thanks for the review @0xd4d.
The bundler is expected to be used with rid-specific publish operations, where all the published files will be in the same directory. So, I don't know if a use-case for sub-directories yet.

I'll fix the other issues you'be pointed out. Thanks.

This comment has been minimized.

Copy link
@0xd4d

0xd4d Mar 11, 2019

Shouldn't all app files should be bundled? Some of them could be in sub directories because not everyone wants to put everything in the base dir.

OutputDir = NextArg(arg);
break;

case "-pdb+":

This comment has been minimized.

Copy link
@0xd4d

0xd4d Mar 11, 2019

Should this be --pdb just like what's printed in Usage()?

@KathleenDollard

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

Two meta-questions...

This is a .NET Core global tool, right? Why is it in the core-setup repo? How are we expecting to deliver this? If it is a new verb, I was confused and have stronger opinions and we should definitely meet.

Are we sure bundle is the right name?

@KathleenDollard

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@0xd4d

This comment has been minimized.

Copy link

commented Mar 11, 2019

If anything in a sub directory isn't bundled (which is bad IMHO), then at least print a warning.

@vitek-karas

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

If anything in a sub directory isn't bundled (which is bad IMHO), then at least print a warning.

+1 - we should definitely bundle the entire subtree.

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

What about files in sub directories? Shouldn't they also be bundled?
+1 - we should definitely bundle the entire subtree.

OK I'll make the change to bundle sub-trees.
My concern with sub-directories was more about what the host would do with those files, and how they'll be accessed by the app.

The answer will be that:
The host during startup will create sub-directories at extraction location and extract non-assembly files to those sub-directories. The app can then access the files by:


bundle.Position = bundle.Length;
string [] contents = Directory.GetFiles(ContentDir);
Array.Sort(contents); // Sort the file names to keep the bundle construction deterministic.

This comment has been minimized.

Copy link
@0xd4d

0xd4d Mar 11, 2019

This seems to use the current culture unless you pass in your own comparer, so you must pass in eg. StringComparer.Ordinal to make it deterministic.

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Thanks for the comments @KathleenDollard.
@nguerrera expressed concern whether we can ship components in the 3.0 SDK that uses
System.Commandline. So wanted to check with you -- but I can give it a try.

This is a .NET Core global tool, right? Why is it in the core-setup repo? How are we expecting to deliver this? If it is a new verb, I was confused and have stronger opinions and we should definitely meet.
This tool is in the core-setup repo because it is closely related to the host. It will be a package consumed by the SDK. More notes are in this comment.

Are we sure bundle is the right name?
I called it bundle because of Mono's terminology. But I'm happy to change it if there's a better name. Thanks.

swaroop-sridhar added some commits Mar 11, 2019

Update Command line switches
Based on feedback from @KathleenDollard and Bri Achtman
@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

@KathleenDollard, I've updated the command line based on your feedback in this commit.

.NET Core Bundler (version 0.1)
Usage: bundle <options>

Bundle options:
  --source <PATH>    Directory containing files to bundle (required).
  --apphost <NAME>   Application host within source directory (required).
  --pdb              Embed PDB files.

Extract options:
  --extract <PATH>   Extract files from the specified bundle.

Common options:
  -o|--output <PATH> Output directory (default: current).
  -d|--diagnostics   Enable diagnostic output.
  -?|-h|--help       Display usage information.

Examples:
Bundle:  bundle --source <publish-dir> --apphost <host-exe> -o <output-dir>
Extract: bundle --extract <bundle-exe> -o <output-dir>

We agreed on the namespace/assembly names, and not to use System.Commandline for now.

@KathleenDollard

This comment has been minimized.

I like the approach you took here. It's not consistent with the CLI, but it will help customers understand which options make sense together.

@swaroop-sridhar swaroop-sridhar force-pushed the swaroop-sridhar:bundler branch from 017c5b0 to bd26f90 Mar 14, 2019

{
// filePath is the full-path of files within source directory, and any of its sub-directories.
// We only need the relative paths with respect to the source directory.
string relativePath = filePath.Substring(sourceDirLen);

This comment has been minimized.

Copy link
@vitek-karas

vitek-karas Mar 14, 2019

Member

Nit: indentation of comments

Test: Change bundle/extraction directory
* Move bundle/extraction location outside TestProject.OutputDirectory
  because all files within TestProject.OutputDirectory and its sub-directories
  will be bundled into one file.
* Fix specing (tab vs space usage).

@swaroop-sridhar swaroop-sridhar merged commit 1bcd5d5 into dotnet:master Mar 15, 2019

25 checks passed

WIP Ready for review
Details
dotnet.core-setup Build #20190314.17 succeeded
Details
dotnet.core-setup (Build_Linux_Arm debug) Build_Linux_Arm debug succeeded
Details
dotnet.core-setup (Build_Linux_Arm release) Build_Linux_Arm release succeeded
Details
dotnet.core-setup (Build_Linux_Arm64 debug) Build_Linux_Arm64 debug succeeded
Details
dotnet.core-setup (Build_Linux_Arm64 release) Build_Linux_Arm64 release succeeded
Details
dotnet.core-setup (Build_Linux_Arm64_Alpine37 debug) Build_Linux_Arm64_Alpine37 debug succeeded
Details
dotnet.core-setup (Build_Linux_Arm64_Alpine37 release) Build_Linux_Arm64_Alpine37 release succeeded
Details
dotnet.core-setup (Build_Linux_x64_Alpine36 debug) Build_Linux_x64_Alpine36 debug succeeded
Details
dotnet.core-setup (Build_Linux_x64_Alpine36 release) Build_Linux_x64_Alpine36 release succeeded
Details
dotnet.core-setup (Build_Linux_x64_Rhel6 debug) Build_Linux_x64_Rhel6 debug succeeded
Details
dotnet.core-setup (Build_Linux_x64_Rhel6 release) Build_Linux_x64_Rhel6 release succeeded
Details
dotnet.core-setup (Build_Linux_x64_glibc debug) Build_Linux_x64_glibc debug succeeded
Details
dotnet.core-setup (Build_Linux_x64_glibc release) Build_Linux_x64_glibc release succeeded
Details
dotnet.core-setup (Build_OSX debug) Build_OSX debug succeeded
Details
dotnet.core-setup (Build_OSX release) Build_OSX release succeeded
Details
dotnet.core-setup (Build_Windows_Arm debug) Build_Windows_Arm debug succeeded
Details
dotnet.core-setup (Build_Windows_Arm release) Build_Windows_Arm release succeeded
Details
dotnet.core-setup (Build_Windows_Arm64 debug) Build_Windows_Arm64 debug succeeded
Details
dotnet.core-setup (Build_Windows_Arm64 release) Build_Windows_Arm64 release succeeded
Details
dotnet.core-setup (Build_Windows_x64 debug) Build_Windows_x64 debug succeeded
Details
dotnet.core-setup (Build_Windows_x64 release) Build_Windows_x64 release succeeded
Details
dotnet.core-setup (Build_Windows_x86 debug) Build_Windows_x86 debug succeeded
Details
dotnet.core-setup (Build_Windows_x86 release) Build_Windows_x86 release succeeded
Details
license/cla All CLA requirements met.
Details

@swaroop-sridhar swaroop-sridhar deleted the swaroop-sridhar:bundler branch Mar 15, 2019

@SpiritBob

This comment has been minimized.

Copy link

commented May 15, 2019

In a case where App updates are downloaded based on this new approach (clients downloading the new .exe file), it seems the self-extracting .exe extracts the whole thing in a new temp directory, instead of replacing the old .net temp directory with the changed files?
I can see this causing major headaches, as with only 3 revisions in, if the application was 200 MB, the user's hard drive would have already eaten up to 600 MB, if not more.

For this reason, I decided to stick to the old --self-contained approach, but if you could make it smarter (to not simply extract the whole binary in a new location if it's different) - it'd be great.

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

The SDK support for publishing .net core apps as a single-file generates bundles with unique IDs every time. This behavior is by design, so that files from apps with different build configurations/versions do not get their extracted files mixed up.

However, I agree that in some circumstances, the user may want to re-generate bundles with the same ID -- say for testing purposes. In this case, it is the user's responsibility to clean out extracted files before using the new build with the same ID -- otherwise files previously extracted will be reused.

I filed #6436 to track this issue.

@jjxtra

This comment has been minimized.

Copy link

commented Jun 5, 2019

So is this always going to extract to a temp/cache folder to run? Will there be an option to run in place, and optionally provide app.config or appsettings.json in the same folder instead of inside the .exe?

@swaroop-sridhar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@jjxtra currently, everything is extracted. There is a plan to implement single-files without extraction.

You can choose what files are in the bundle and what files are left separately in the app.
Please see the ExcludeFromSingleFile option.

@jjxtra

This comment has been minimized.

Copy link

commented Jun 5, 2019

Great to hear! Hoping to implement this for https://github.com/DigitalRuby/IPBan :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.