Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Replace OPTIMIZE_FOR_SIZE with feature switch #111743
Replace OPTIMIZE_FOR_SIZE with feature switch #111743
Changes from 7 commits
5412071
eb3fe5c
4f9fad1
4636aba
e31d983
b869eea
0c8ae74
77f2636
60297de
da36781
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
OptimizeForSize
was set true for some targets but it's not anymore.WasmFeatures.props
set the default for wasm workload, but default runtime pack also needs to be optimized for size - when people don't use workload in Blazor.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.
Yes, that is the purpose of this PR. To only have one build of this library (#111743 (comment))
Could you please point me to which .targets we need to update? Is it the Microsoft.NET.Sdk.BlazorWebAssembly.6_0.targets linked from #111743 (comment)?
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.
(To be clear, the OptimizeForSize property controls whether we build System.Linq assembly in a not-fully-compatible manner (see the tests I'm touching where it behaves differently) that has better size characteristics. This PR deletes that build and changes it to always build the compatible Linq implementation. It introduces a feature switch that allows switching to the not-fully-compatible mode. The default for the feature switch is disabled. It can be enabled where the tradeoff is worth it (I have PRs out enabling it in places, linked above). The advantage is that now if a customer runs into the incompatibility, they can just set a property and unblock themselves.)
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 think maybe we need both
https://github.com/dotnet/sdk/blob/b89ea044c3ac0e028228e73abcc8310b101b63b8/src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.6_0.targets#L43-L50
@maraf
I hope that we run ILLink during publish when wasm-workload is not installed ?
Is there better place ?
and for workload
https://github.com/dotnet/runtime/blob/f1901a00dfcfbcf0a4f8b6aa7459fec30846d1ab/src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/WasmFeatures.props
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.
Thank you! Folded the BlazorWebAssembly.targets change into dotnet/sdk#46375.
WasmFeatures.props change is already part of the PR here.