-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Singlefile compression of native files #49855
Conversation
Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov Issue DetailsIn progress, just running tests
|
src/installer/managed/Microsoft.NET.HostModel/Bundle/Bundler.cs
Outdated
Show resolved
Hide resolved
src/installer/managed/Microsoft.NET.HostModel/Bundle/FileEntry.cs
Outdated
Show resolved
Hide resolved
I think this PR has everything that I planned for this part. |
src/installer/managed/Microsoft.NET.HostModel/Bundle/Bundler.cs
Outdated
Show resolved
Hide resolved
@@ -47,6 +47,7 @@ public static string UseFrameworkDependentHost(TestProjectFixture testFixture) | |||
Version targetFrameworkVersion = null) | |||
{ | |||
UseSingleFileSelfContainedHost(testFixture); | |||
options |= BundleOptions.EnableCompression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also add a couple of tests which disable compression on purpose. I know we expect compression to be on by default, but will allow users to disable it, so we should validate that it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some files are not compressed, and .json never will be, so the case with uncompressed files is validated.
Turning off compression intentionally in one of self-contained tests is an easy thing to do though.
@vitek-karas - I think I have addressed all the concerns. Let me know if I missed something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, but nothing major... looks good!
@@ -9,6 +9,7 @@ | |||
using System.IO; | |||
using System.Reflection.PortableExecutable; | |||
using System.Runtime.InteropServices; | |||
using System.IO.Compression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: sort usings
UseSingleFileSelfContainedHost(fixture); | ||
options |= BundleOptions.EnableCompression; | ||
var singleFile = BundleHelper.BundleApp(fixture, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we plan to have this on by default - I would probably change the default test behavior as well. Make the BundleHelper
turn on compression by default and have a way for the test to selectively disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the perspective of the Bundler compression will be off by default. That is because the Bundler does not know if we use framework-dependent bundle and it would be a dangerous default.
I think BundleHelper should default to "off" for the same reasons. Compression must be an opt-in feature at the bundler level. SDK can have different defaults since it has more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree to a point - basically if I add a new test - I will definitely forget to set this option - and thus my test will not cover the most common scenario. Given the history of bad and few tests in this area I'd like to make it so that we have at least reasonable coverage for the most common cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have explored making UseSingleFileSelfContainedHost
to enable compression automatically, but it was not very natural since the “fixture” stuff operates at proj file level and does not know much about the bundle.
Perhaps I should revisit that direction. Maybe set a flag on the fixture, that BundleHelper would read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope going further that direction only make things worse. If UseSingleFileSelfContainedHost
sets "can do compression" flag, then there are two ways to specify whether compression is desired - because fixture says so and via flags passed to bundler. The flag on the fixture must take precedence and that is a bit unintuitive and could be error prone if fixture is reused. Also for the unusual/error cases I would need another argument to override the override. It gets messy.
Here what seems to work better.
- Add a helper
BundleSelfContainedApp
which both picks the right host and then adds compression flags and passes that to Bundler. - Make all tests that could use
BundleSelfContainedApp
use it - that is nearly all tests that do singlefile stuff. - A a few tests that need to tests unusual/error combinations go the long route - they will have to use
UseSingleFileSelfContainedHost
and explicitly and have spell out what they need in terms of compression toBundleApp
. There are only a few tests like that.
I would expect the obvious difference in verbosity will direct future testcase writers towards the "default" path.
@mateoatr could you please also take a look. It's a relatively major change so it would be best to have multiple people look at it. I'm especially nervous about the versioning and compat behaviors (always a tricky topic). |
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
@@ -59,6 +59,8 @@ public void Bundled_Framework_dependent_App_Run_Succeeds(BundleOptions options) | |||
public void Bundled_Self_Contained_App_Run_Succeeds(BundleOptions options) | |||
{ | |||
var fixture = sharedTestState.TestSelfContainedFixture.Copy(); | |||
UseSingleFileSelfContainedHost(fixture); | |||
options |= BundleOptions.EnableCompression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have something similar in NetCoreApp3CompatModeTests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per @vitek-karas suggestion I am making compression a default behavior in all singlefile tests. Only few tests will explicitly use singlefilehost and no compression.
I think it will take care of this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks!! |
Hello @VSadov! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Adding lzma compression in the future would be interesting, further reducing size at the cost of slightly longer build time and slightly longer decompression times. |
First part of implementing compression in singlefile apps.
In this change:
Current use is only in tests. Eventually SDK will use this flag, also plans for exposing the flag in msbuild.