-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Enable compression in singlefile #16838
Conversation
VSadov
commented
Apr 11, 2021
•
edited
Loading
edited
- enable bundle compression by default for 6.0+
- add an msbuild property
- add tests
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
71385a7
to
7c03052
Compare
IsExe = true, | ||
}; | ||
|
||
testProject.AdditionalProperties.Add("EnableCompressionInSingleFile", "true"); |
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.
You might have to explicitly set SelfContained to false since it defaults to true when there is a RuntimeIdentifier. I think you can also get rid of the exilicit SelfContained=true in the other tests.
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.
Ah, I see. I thought it is the opposite and it is not SelfContained unless explicitly specified. Thanks!
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.
@sbomer - do you know if there is an easy way to include a native lib dependency in these tests? I think the compression tests are failing because there is nothing in the bundle that we can compress.
We do not compress managed assemblies yet. We had a bug that would cause all native libraries be included as dependencies and tests were passing (with IncludeAllContent
) , but I think the fix for that has merged to SDK yesterday. Since all we have in the bundle is managed assemblies, we have nothing to compress thus the size of the app is unaffected by the compression setting.
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.
Not sure if there's a better way, but I found this test which adds a packagereference to sqlite and checks that native dependencies are included: https://github.com/dotnet/sdk/blob/main/src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToCopyLocalDependencies.cs#L64
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.
This is not just tests - the only "truly supported" way to include native libraries (as in it works in all configs) is through nuget package reference. So the sqllite is as good as any for that purpose.
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 will try sqllite. Thanks.
It is point-in-time problem though. Once we compress managed assemblies, this will not be an issue, so if sqllite does not work I can also wait for that change that adds support for managed.