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

Update default TrimMode for WebSDK projects #33962

Merged
merged 2 commits into from
Jul 14, 2023
Merged

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Jul 12, 2023

Set TrimMode=full for web projects where PublishTrimmed=true.

Note: we could achieve the same effect by removing the property all together and relying on the defaults the linker targets set but I like the explicitness of this approach.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-WebSDK untriaged Request triage from a team member labels Jul 12, 2023
@captainsafia captainsafia force-pushed the safia/update-trim-mode branch 3 times, most recently from 401ba29 to 58f8858 Compare July 12, 2023 23:17
@captainsafia captainsafia force-pushed the safia/update-trim-mode branch from 58f8858 to 5808477 Compare July 13, 2023 16:59
@captainsafia captainsafia marked this pull request as ready for review July 14, 2023 00:24
@captainsafia captainsafia requested a review from vijayrkn as a code owner July 14, 2023 00:24
@DamianEdwards
Copy link
Member

@captainsafia might be worth getting @agocke to check this too. Also, just to confirm, this should have on impact on Blazor WebAssembly projects right? They have to still use <TrimMode>partial</TrimMode> as I understand it.

@captainsafia
Copy link
Member Author

might be worth getting @agocke to check this too.

Sure.

Also, just to confirm, this should have on impact on Blazor WebAssembly projects right? They have to still use partial as I understand it.

Yes, Blazor is unaffected. The fact that these tests pass helps us confirm this.

@captainsafia captainsafia requested a review from agocke July 14, 2023 01:20
@eerhardt
Copy link
Member

Also, just to confirm, this should have on impact on Blazor WebAssembly projects right? They have to still use <TrimMode>partial</TrimMode> as I understand it.

Blazor WASM sets it early in the project (in the .props file) here:

<!-- Trimmer defaults -->
<PublishTrimmed Condition="'$(PublishTrimmed)' == ''">true</PublishTrimmed>
<TrimMode Condition="'$(TrimMode)' == ''">partial</TrimMode>
<TrimmerRemoveSymbols Condition="'$(TrimmerRemoveSymbols)' == ''">false</TrimmerRemoveSymbols>

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Blazor may be worth a test somewhere -- I assume there's one in their repo.

@captainsafia
Copy link
Member Author

LGTM, thanks!

Blazor may be worth a test somewhere -- I assume there's one in their repo.

Yes, here.

@captainsafia captainsafia merged commit 9f5f518 into main Jul 14, 2023
@captainsafia captainsafia deleted the safia/update-trim-mode branch July 14, 2023 20:33

<!-- If TrimMode has not already been set, default to partial trimming, as AspNet is not currently fully trim-compatible -->
<!-- We allow the full trimming default with PublishAot enabled, because the parts that are not trim-compatible are not aot-compatible either -->
<TrimMode Condition="'$(TrimMode)' == '' and '$(PublishTrimmed)' == 'true' and '$(PublishAot)' != 'true'">partial</TrimMode>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uggh, I just realized that we probably should be leaving TrimMode=partial for TFMs below net8.0. I don't think we should be changing behavior for people when they upgrade their SDK. Only when they retarget from net6/7.0 => net8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-WebSDK untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants