Implement Single-file Bundler #5286
Conversation
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: https://github.com/dotnet/coreclr/issues/20287
src/managed/tools/bundle/bundle.cs
Outdated
| using System.Reflection.PortableExecutable; | ||
| using System.Linq; | ||
|
|
||
| namespace SingleFile |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I renamed the bundler package to Microsoft.NET.Build.Bundle.
CC: @KathleenDollard @richlander for naming.
src/managed/tools/bundle/bundle.cs
Outdated
| Console.WriteLine(" [-?] Display usage information"); | ||
| } | ||
|
|
||
| static void ParseArgs(string[] args) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
This commit makes two modifications: * Name the bundler as Microsoft.DotNet.Build.Bundle.dll * Reorganize the bundler code to separate files.
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.
CC: @nguerrera |
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. |
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() |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
|
@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. |
|
CC: @fadimounir |
|
@KathleenDollard @richlander
|
| // Allign assemblies, since they are loaded directly from bundle | ||
| if (type == FileType.Assembly) | ||
| { | ||
| long padding = AssemblyAlignment - (bundle.Position % AssemblyAlignment); |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
Should all input files be sorted so the output file is deterministic?
There was a problem hiding this comment.
What about files in sub directories? Shouldn't they also be bundled?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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+": |
There was a problem hiding this comment.
Should this be --pdb just like what's printed in Usage()?
|
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? |
|
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. |
OK I'll make the change to bundle sub-trees. The answer will be that:
|
|
|
||
| bundle.Position = bundle.Length; | ||
| string [] contents = Directory.GetFiles(ContentDir); | ||
| Array.Sort(contents); // Sort the file names to keep the bundle construction deterministic. |
There was a problem hiding this comment.
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.
|
Thanks for the comments @KathleenDollard.
|
Based on feedback from @KathleenDollard and Bri Achtman
|
@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 |
swaroop-sridhar
force-pushed
the
bundler
branch
from
017c5b0
to
bd26f90
Compare
| { | ||
| // 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); |
There was a problem hiding this comment.
Nit: indentation of comments
* 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).
|
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? 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. |
|
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 https://github.com/dotnet/core-setup/issues/6436 to track this issue. |
|
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? |
|
@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. |
|
Great to hear! Hoping to implement this for https://github.com/DigitalRuby/IPBan :) |
* 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 also adds a test-case that: * Bundles the contents of a self-contained console app publish directory along with all sub-directories * Extracts the contents to a new location * Runs the extracted app to verify successful execution TBD: * Add more tests tests once the host-changes to process the bundle are implemented. * Localization of output strings. Issue: https://github.com/dotnet/coreclr/issues/20287 Commit migrated from dotnet/core-setup@1bcd5d5
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: https://github.com/dotnet/coreclr/issues/20287