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

Microsoft.NET.Sdk.Web should default to TrimMode=full #30059

Closed
eerhardt opened this issue Jan 13, 2023 · 7 comments · Fixed by #29984
Closed

Microsoft.NET.Sdk.Web should default to TrimMode=full #30059

eerhardt opened this issue Jan 13, 2023 · 7 comments · Fixed by #29984
Assignees
Labels
Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch
Milestone

Comments

@eerhardt
Copy link
Member

Today, we are defaulting TrimMode=partial in Microsoft.NET.Sdk.Web:

<!-- If TrimMode has not already been set, default to partial trimming, as AspNet is not currently fully trim-compatible -->
<TrimMode Condition="'$(PublishTrimmed)' == 'true' and '$(TrimMode)' == ''">partial</TrimMode>

    <!-- If TrimMode has not already been set, default to partial trimming, as AspNet is not currently fully trim-compatible -->
    <TrimMode Condition="'$(PublishTrimmed)' == 'true' and '$(TrimMode)' == ''">partial</TrimMode>

In the "api" template being added in dotnet/aspnetcore#46064, we are explicitly setting TrimMode=full.

Instead, we should do one of the following:

  1. Remove the above code, and just let the TrimMode=full default from the runtime be applied to Microsoft.NET.Sdk.Web projects.
  2. Change the default to partial only if we aren't also publishing for NativeAOT. Basically adding and '$(PublishAot)' != 'true' above.

My goal would be we do (1). For places in ASP.NET that aren't fully trim-compatible, the app developer should get a warning (even from within ASP.NET code) to tell them what are things that aren't going to work.

We may consider just doing (2) in .NET 8, until things like MVC and Razor pages work correctly in fully trimmed apps.

cc @JamesNK @DamianEdwards @davidfowl

@ghost
Copy link

ghost commented Jan 20, 2023

@dotnet/linker-contrib a new issue has been filed in the ILLink area, please triage

@halter73 halter73 removed their assignment Jan 20, 2023
@halter73
Copy link
Member

We should think about defaulting to full with PublishTrimmed true as well

#29984 (comment)

There's some debate one whether we'll be ready in .NET 8, but we will keep this issue open to track changing the TrimMode default from "partial" to "full" like the Microsoft.NET.Sdk default.

@halter73 halter73 reopened this Jan 20, 2023
@eerhardt eerhardt added Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch and removed Area-ILLink labels Jan 20, 2023
@danroth27 danroth27 added this to the .NET 8.0 milestone Mar 27, 2023
@mkArtakMSFT mkArtakMSFT removed the untriaged Request triage from a team member label Apr 19, 2023
@mkArtakMSFT
Copy link
Member

@adityamandaleeka assigning this to you as this is minimal related.

@eerhardt
Copy link
Member Author

as this is minimal related.

Note that this issue applies to "all" ASP.NET server app models - minimal APIs, MVC, Razor Pages, etc.

@agocke
Copy link
Member

agocke commented May 26, 2023

I would go for (1) as well.

That code was specifically meant to provide compatibility for Blazor WASM, which was a pre-existing trimming scenario. Having it apply to all of ASP.NET wasn't really intentional on my part.

partial isn't really intended for new trimming scenarios. It has no effect on trim warnings, so it doesn't really help in the task of making things trim compatible, and our current goal for trimming is to give users a very clear statement on whether or not their app works, based on the warning behavior.

@adityamandaleeka
Copy link
Member

@eerhardt Can this be closed now?

@eerhardt
Copy link
Member Author

eerhardt commented Aug 2, 2023

Yes, this was fixed with #33962 and #34030.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants