Change linker defaults to "link" with warnings #16327
Conversation
|
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. |
sbomer
changed the title
Except in Web/Blazor/WinForms/WPF scenarios.
sbomer
changed the title
sbomer
changed the title
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets
Outdated
Show resolved
Hide resolved
src/WebSdk/Web/Sdk/Sdk.targets
Outdated
| @@ -22,6 +22,8 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| Microsoft.AspNetCore.App. This needs to happen after the .NET SDK has evaluated TFMs. | |||
| --> | |||
| <AddRazorSupportForMvc Condition="'$(DisableImplicitFrameworkReferences)' != 'true' And '$(AddRazorSupportForMvc)' == '' And '$(TargetFrameworkIdentifier)' == '.NETCoreApp' And '$(_TargetFrameworkVersionWithoutV)' >= '3.0'">true</AddRazorSupportForMvc> | |||
|
|
|||
| <SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == ''">true</SuppressTrimAnalysisWarnings> | |||
There was a problem hiding this comment.
Doesn't this get set imported after Microsoft.NET.ILLink.targets? Meaning by this time, SuppressTrimAnalysisWarnings will never be empty because Microsoft.NET.ILLink.targets has already set it to false?
I think this should probably move to a .props file. Probably here:
There was a problem hiding this comment.
Oh, I see you have it in both places. I think this one should be reverted.
In reply to: 595273084 [](ancestors = 595273084)
There was a problem hiding this comment.
The intention of this change was to suppress warnings for aspnet apps. From what I could tell, this isn't imported in blazorwasm projects - and blazorserver projects don't import the BlazorWebAssembly.Current.props. It would be great to have someone double-check this, but this is the SDK dependency graph I came up with:
- .NET
- Web.ProjectSystem
- Publish
- Razor -> .NET
- Worker -> .NET, Web.ProjectSystem
- Web -> .NET, Web.ProjectSystem, Razor, Publish
- BlazorWebassembly -> .NET, Web.ProjectSystem, Razor, Publish
I wonder if we should also suppress warnings for the Worker SDK.
One thing I wasn't sure about was whether it's better to set the web sdk defaults in props or in targets. The .NET SDK defaults are only set in targets, when PublishTrimmed is set. But the BlazorWebAssembly.Current.props do it early (since they also set PublishTrimmed early).
There was a problem hiding this comment.
The intention of this change was to suppress warnings for aspnet apps.
Ah, I missed that. I thought this was for Blazor WASM.
One thing I wasn't sure about was whether it's better to set the web sdk defaults in props or in targets.
This is always a tricky decision. In general, when defaulting properties in a higher-layer, the earlier you can do it the better. This is because the higher the layer you are in, the more information you know and have a better idea at what the value should be.
A higher layer's .props file gets imported before a lower layer's. And a higher layer's .targets file gets imported after a lower layer's. This allows for the higher layer to default properties before the lower layer. And it allows the higher layer to overwrite things after the lower layer has set them.
The issue here is that Microsoft.NET.ILLink.targets is going to default it first. And by the time this setting gets executed, it will already be defaulted and the Condition will no longer hold.
There was a problem hiding this comment.
Moved this to the Web SDK props for now which should fix the ordering issue you pointed out (I'd forgotten that ILLink targets are part of the .NET SDK targets, not the Publish SDK targets).
src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs
Outdated
Show resolved
Hide resolved
I think that makes sense and it could be combined with the removal of hardcoded linkmode too for these SDKs. |
- Keep suppressions for .NET < 6 - Fix missing condition - Capitalize And/Or - Remove unnecessary arguments in tests
To match how blazorwasm does it. This also fixes an import order issue - this setting needs to be set before importing the Microsoft.NET.Sdk targets.
| @@ -40,8 +40,13 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <CustomResourceTypesSupport>false</CustomResourceTypesSupport> | |||
| </PropertyGroup> | |||
|
|
|||
| <PropertyGroup Condition="'$(EnableTrimAnalyzer)' == 'true' or '$(PublishTrimmed)' == 'true'"> | |||
| <SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == ''">true</SuppressTrimAnalysisWarnings> | |||
| <PropertyGroup Condition="'$(SuppressTrimAnalysisWarnings)' == '' And ('$(EnableTrimAnalyzer)' == 'true' Or '$(PublishTrimmed)' == 'true')"> | |||
There was a problem hiding this comment.
Should we check TFM as well? Looks like we are only checking framework version but not that they are targeting Core. I can't really think now of a case where it would matter since other TFMs like .NET Framework don't really have a 6.0 version, but I might be not considering some.
There was a problem hiding this comment.
I asked a similar question here: #16094 (comment)
There was a problem hiding this comment.
Yup, we already check for netcoreapp in ILLink. Technically these global properties could be defined (but unused) on .NET Framework if someone sets "PublishTrimmed"... if anything, maybe we should be emitting a warning if someone sets PublishTrimmed while targeting .NET Framework as it will currently do nothing.
| <_TrimmerDefaultAction>$(TrimMode)</_TrimmerDefaultAction> | ||
| </PropertyGroup> | ||
| <PropertyGroup> | ||
| <TrimMode Condition=" '$(TrimMode)' == '' ">link</TrimMode> |
There was a problem hiding this comment.
NIT: I wonder if we should have a message now whenever 'link' is set and warnings are on just so the user understands why all of a sudden their app started to have all of these warnings that it didn't have in the previous version of the sdk...
Doesn't need to happen on this PR but perhaps it is something we should just consider as a feature for better experience. Maybe we could even just add this note on the message that we already print out today where we say that linking may affect functionality of your app.
There was a problem hiding this comment.
Good point. We're already planning to improve the warning experience in P4 - we want to "collapse" them by assembly to be less noisy, which should make it more obvious what they're about (so you would only get a warning like "SomeAssembly is not annotated for trimming."). I think that plus the existing message we print out, and that trimming is still considered "preview" will make it clear enough - and I'm also happy to change the message.
How about this?
"Optimizing assemblies for size, which may introduce warnings or change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink".
I guess we could also change the message only for .NET6+ (or condition it based on SuppressTrimAnalysisWarnings?) - but I think just changing the existing message is good enough. I don't want to overthink this. :)
There was a problem hiding this comment.
Sure I'm fine with whatever is decided. My comment here was more around the fact that we are now actually linking very differently the apps by default, so imagine a person who was using a previous version of the SDK, tested their app at runtime due to our message and ensured it ran fine, and then update the SDK but wouldn't re-test as they had already tested their app before and the warning they got is the same. I'm sure I'm overthinking this 😆 so again not really required for you to change things as they are now.
There was a problem hiding this comment.
My comment here was more around the fact that we are now actually linking very differently the apps by default
That's not correct as it really depends on SDKs and the user settings.
"Optimizing assemblies for size, which may introduce warnings or change the behavior of the app.
I think we should go with a simple message not to add all sort of ifs/whens.
"Optimizing managed assemblies for size. Unresolved illink warnings could break the application (aka.ms/dotnet-illink)"
There was a problem hiding this comment.
Agree simple is better. The issue with "Unresolved illink warnings could break the application" is that the warnings are disabled when targeting net5.0, but linking could still break the app. FWIW, this is the current message - maybe it's good enough as-is? The aka.ms link should make it easy enough to find the options for turning on/off the warnings.
"Optimizing assemblies for size, which may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink"
There was a problem hiding this comment.
I'd really like to avoid using the word "publishing" and we are running the illink before we run the tests sometimes too, hence "Be sure to test after publishing." sentence is not really helpful.
Fixes: dotnet#5751 After bumping to .NET 6 Preview 3, we get hundreds of ILLINK warnings. This is because of: dotnet/sdk#16327 We need to default `$(SuppressTrimAnalysisWarnings)` to ignore these warnings for now. We should continue to use `TrimMode=link`, as that is the behavior Xamarin developers get today.
Fixes: dotnet#10887 After bumping to .NET 6 Preview 3, we get hundreds of ILLINK warnings. This is because of: dotnet/sdk#16327 We need to default `$(SuppressTrimAnalysisWarnings)` to ignore these warnings for now. We should continue to use `TrimMode=link`, as that is the behavior Xamarin developers get today.
Fixes: #5751 After bumping to .NET 6 Preview 3, we get hundreds of ILLINK warnings. This is because of: dotnet/sdk#16327 We need to default `$(SuppressTrimAnalysisWarnings)` to ignore these warnings for now. We should continue to use `TrimMode=link`, as that is the behavior Xamarin developers get today.
Fixes: #10887 After bumping to .NET 6 Preview 3, we get hundreds of ILLINK warnings. This is because of: dotnet/sdk#16327 We need to default `$(SuppressTrimAnalysisWarnings)` to ignore these warnings for now. We should continue to use `TrimMode=link`, as that is the behavior Xamarin developers get today.
Fixes: #10887 After bumping to .NET 6 Preview 3, we get hundreds of ILLINK warnings. This is because of: dotnet/sdk#16327 We need to default `$(SuppressTrimAnalysisWarnings)` to ignore these warnings for now. We should continue to use `TrimMode=link`, as that is the behavior Xamarin developers get today.
This changes the default
TrimModeto link (fixes #15905), and turns on the linker analysis warnings by default in the .NET SDK, with exceptions for Blazor, ASP.NET, WinForms, and WPF:Another way to suppress warnings for WinForms/WPF might be to do it in https://github.com/dotnet/wpf/blob/master/packaging/Microsoft.NET.Sdk.WindowsDesktop/targets/Microsoft.NET.Sdk.WindowsDesktop.props. Unlike the Blazor/web SDKs, these targets are imported directly by the .NET SDK - since the dependency is from the .NET SDK to the WindowsDesktop SDK and not the other way around, I think it's acceptable to set the WinForms/WPF defaults directly in the .NET SDK (which also avoids spreading linker logic across repos).
However, for xamarin-android and xamarin-macios I think we need to set SuppressTrimAnalysisWarnings in those repos. If we agree on this approach I'll file issues there.