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.

Sign up for GitHub

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

Change linker defaults to "link" with warnings #16327

Merged
merged 6 commits into from
Mar 16, 2021
Merged

Change linker defaults to "link" with warnings #16327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
81eaf67
Change default TrimMode to link
sbomer Mar 12, 2021
7da4001
Turn on trim analysis warnings by default
sbomer Mar 15, 2021
ee37188
Fix test
sbomer Mar 16, 2021
2e17ed7
PR feedback
sbomer Mar 16, 2021
d93be0f
Don't pass unneccesary TrimMode=Link
sbomer Mar 16, 2021
0a9d59c
Move Web SDK settings to props
sbomer Mar 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.Current.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<PublishTrimmed Condition="'$(PublishTrimmed)' == ''">true</PublishTrimmed>
<TrimMode Condition="'$(TrimMode)' == ''">link</TrimMode>
<TrimmerRemoveSymbols Condition="'$(TrimmerRemoveSymbols)' == ''">false</TrimmerRemoveSymbols>
<SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == ''">true</SuppressTrimAnalysisWarnings>
</PropertyGroup>

<Import Sdk="Microsoft.NET.Sdk.Razor" Project="Sdk.props" />
Expand Down
23 changes: 16 additions & 7 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')">
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I asked a similar question here: #16094 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

<!-- Trim analysis warnings are suppressed for .NET < 6. -->
<SuppressTrimAnalysisWarnings Condition="$([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '6.0'))">true</SuppressTrimAnalysisWarnings>
<!-- Suppress for WPF/WinForms -->
<SuppressTrimAnalysisWarnings Condition="'$(UseWpf)' == 'true' Or '$(UseWindowsForms)' == 'true'">true</SuppressTrimAnalysisWarnings>
<!-- Otherwise, for .NET 6+, warnings are on by default -->
<SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == ''">false</SuppressTrimAnalysisWarnings>
</PropertyGroup>

<!-- Suppress warnings produced by the linker or by the ILLink Roslyn analyzer. Warnings produced
Expand Down Expand Up @@ -178,14 +183,18 @@ Copyright (c) .NET Foundation. All rights reserved.
<ILLinkWarningLevel Condition=" '$(ILLinkWarningLevel)' == '' ">0</ILLinkWarningLevel>
</PropertyGroup>

<PropertyGroup>
<ILLinkTreatWarningsAsErrors Condition=" '$(ILLinkTreatWarningsAsErrors)' == '' ">$(TreatWarningsAsErrors)</ILLinkTreatWarningsAsErrors>
<_ExtraTrimmerArgs>--skip-unresolved true $(_ExtraTrimmerArgs)</_ExtraTrimmerArgs>
<!-- Defaults for .NET < 6 -->
<PropertyGroup Condition=" $([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '6.0')) ">
<TrimMode Condition=" '$(TrimMode)' == '' ">copyused</TrimMode>
<!-- For .NET < 6, the defaults are the same regardless of whether the assembly has an IsTrimmable attribute. (The attribute didn't exist until .NET 6.) -->
<_TrimmerDefaultAction Condition=" $([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '6.0')) ">$(TrimMode)</_TrimmerDefaultAction>
<!-- Action is the same regardless of whether the assembly has an IsTrimmable attribute. (The attribute didn't exist until .NET 6.) -->
<_TrimmerDefaultAction>$(TrimMode)</_TrimmerDefaultAction>
</PropertyGroup>
<PropertyGroup>
<TrimMode Condition=" '$(TrimMode)' == '' ">link</TrimMode>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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. :)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)"

Copy link
Member Author

Choose a reason for hiding this comment

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

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"

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

<!-- For .NET 6+, assemblies without IsTrimmable attribute get the "copy" action. -->
<_TrimmerDefaultAction Condition=" '$(_TrimmerDefaultAction)' == '' ">copy</_TrimmerDefaultAction>
<ILLinkTreatWarningsAsErrors Condition=" '$(ILLinkTreatWarningsAsErrors)' == '' ">$(TreatWarningsAsErrors)</ILLinkTreatWarningsAsErrors>
<_ExtraTrimmerArgs>--skip-unresolved true $(_ExtraTrimmerArgs)</_ExtraTrimmerArgs>
</PropertyGroup>

<!-- Suppress warnings produced by the linker. Any warnings shared with the analyzer should be
Expand Down
74 changes: 61 additions & 13 deletions src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public void ILLink_links_simple_app_without_analysis_warnings_and_it_runs(string
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(testAsset);
publishCommand.Execute($"/p:RuntimeIdentifier={rid}", "/p:SelfContained=true", "/p:PublishTrimmed=true", $"/p:TrimMode={trimMode}")
publishCommand.Execute($"/p:RuntimeIdentifier={rid}", "/p:SelfContained=true", "/p:PublishTrimmed=true", $"/p:TrimMode={trimMode}", "/p:SuppressTrimAnalysisWarnings=true")
.Should().Pass()
.And.NotHaveStdOutContaining("warning IL2075")
.And.NotHaveStdOutContaining("warning IL2026");
Expand Down Expand Up @@ -184,17 +184,17 @@ public void PrepareForILLink_can_set_TrimMode(string targetFramework)
}

[RequiresMSBuildVersionTheory("16.8.0")]
[InlineData("net5.0")]
[InlineData("net6.0")]
public void ILLink_respects_global_TrimMode(string targetFramework)
[InlineData("net5.0", "link")]
[InlineData("net6.0", "copyused")]
public void ILLink_respects_global_TrimMode(string targetFramework, string trimMode)
{
var projectName = "HelloWorld";
var referenceProjectName = "ClassLibForILLink";
var rid = EnvironmentInfo.GetCompatibleRid(targetFramework);

var testProject = CreateTestProjectForILLinkTesting(targetFramework, projectName, referenceProjectName);
var testAsset = _testAssetsManager.CreateTestProject(testProject)
.WithProjectChanges(project => SetGlobalTrimMode(project, "link"))
.WithProjectChanges(project => SetGlobalTrimMode(project, trimMode))
.WithProjectChanges(project => SetIsTrimmable(project, referenceProjectName))
.WithProjectChanges(project => AddRootDescriptor(project, $"{referenceProjectName}.xml"));

Expand All @@ -208,9 +208,14 @@ public void ILLink_respects_global_TrimMode(string targetFramework)

File.Exists(publishedDll).Should().BeTrue();
File.Exists(isTrimmableDll).Should().BeTrue();
// Check that the assembly was trimmed at the member level
DoesImageHaveMethod(isTrimmableDll, "UnusedMethodToRoot").Should().BeTrue();
DoesImageHaveMethod(isTrimmableDll, "UnusedMethod").Should().BeFalse();
if (trimMode == "link") {
// Check that the assembly was trimmed at the member level
DoesImageHaveMethod(isTrimmableDll, "UnusedMethod").Should().BeFalse();
} else {
// Check that the assembly was trimmed at the assembxly level
DoesImageHaveMethod(isTrimmableDll, "UnusedMethod").Should().BeTrue();
}
}

[RequiresMSBuildVersionTheory("16.8.0")]
Expand Down Expand Up @@ -317,8 +322,7 @@ public void ILLink_TrimMode_applies_to_IsTrimmable_assemblies(string targetFrame
string projectName = "HelloWorld";
var rid = EnvironmentInfo.GetCompatibleRid(targetFramework);
var testProject = CreateTestProjectWithIsTrimmableAttributes(targetFramework, projectName);
var testAsset = _testAssetsManager.CreateTestProject(testProject)
.WithProjectChanges(project => SetGlobalTrimMode(project, "link"));
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(testAsset);
publishCommand.Execute($"/p:RuntimeIdentifier={rid}").Should().Pass();
Expand Down Expand Up @@ -359,8 +363,8 @@ public void ILLink_can_set_TrimmerDefaultAction(string targetFramework)
var unusedTrimmableDll = Path.Combine(publishDirectory, "UnusedTrimmableAssembly.dll");
var unusedNonTrimmableDll = Path.Combine(publishDirectory, "UnusedNonTrimmableAssembly.dll");

// Trimmable assemblies are trimmed at assembly level
DoesImageHaveMethod(trimmableDll, "UnusedMethod").Should().BeTrue();
// Trimmable assemblies are trimmed at member level
DoesImageHaveMethod(trimmableDll, "UnusedMethod").Should().BeFalse();
File.Exists(unusedTrimmableDll).Should().BeFalse();
// Unattributed assemblies are trimmed at member level
DoesImageHaveMethod(nonTrimmableDll, "UnusedMethod").Should().BeFalse();
Expand Down Expand Up @@ -389,6 +393,28 @@ public void ILLink_analysis_warnings_are_disabled_by_default(string targetFramew
.And.NotHaveStdOutContaining("warning IL2093");
}

[RequiresMSBuildVersionTheory("16.8.0")]
[InlineData("net6.0")]
public void ILLink_analysis_warnings_are_enabled_by_default(string targetFramework)
{
var projectName = "AnalysisWarnings";
var rid = EnvironmentInfo.GetCompatibleRid(targetFramework);

var testProject = CreateTestProjectWithAnalysisWarnings(targetFramework, projectName);
testProject.AdditionalProperties["PublishTrimmed"] = "true";
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(testAsset);
publishCommand.Execute($"/p:RuntimeIdentifier={rid}", $"/p:SelfContained=true")
.Should().Pass()
// trim analysis warnings are enabled
.And.HaveStdOutMatching("warning IL2075.*Program.IL_2075")
.And.HaveStdOutMatching("warning IL2026.*Program.IL_2026.*Testing analysis warning IL2026")
.And.HaveStdOutMatching("warning IL2043.*Program.IL_2043.get")
.And.HaveStdOutMatching("warning IL2046.*Program.Derived.IL_2046")
.And.HaveStdOutMatching("warning IL2093.*Program.Derived.IL_2093");
}

[RequiresMSBuildVersionTheory("16.8.0")]
[InlineData("net5.0")]
public void ILLink_accepts_option_to_enable_analysis_warnings(string targetFramework)
Expand All @@ -411,6 +437,28 @@ public void ILLink_accepts_option_to_enable_analysis_warnings(string targetFrame
.And.HaveStdOutMatching("warning IL2093.*Program.Derived.IL_2093");
}

[RequiresMSBuildVersionTheory("16.8.0")]
[InlineData("net6.0")]
public void ILLink_accepts_option_to_disable_analysis_warnings(string targetFramework)
{
var projectName = "AnalysisWarnings";
var rid = EnvironmentInfo.GetCompatibleRid(targetFramework);

var testProject = CreateTestProjectWithAnalysisWarnings(targetFramework, projectName);
testProject.AdditionalProperties["PublishTrimmed"] = "true";
testProject.AdditionalProperties["SuppressTrimAnalysisWarnings"] = "true";
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(testAsset);
publishCommand.Execute($"/p:RuntimeIdentifier={rid}", $"/p:SelfContained=true")
.Should().Pass()
.And.NotHaveStdOutContaining("warning IL2075")
.And.NotHaveStdOutContaining("warning IL2026")
.And.NotHaveStdOutContaining("warning IL2043")
.And.NotHaveStdOutContaining("warning IL2046")
.And.NotHaveStdOutContaining("warning IL2093");
}

[RequiresMSBuildVersionTheory("16.8.0")]
[InlineData("net5.0")]
public void ILLink_accepts_option_to_enable_analysis_warnings_without_PublishTrimmed(string targetFramework)
Expand Down Expand Up @@ -487,7 +535,7 @@ public void ILLink_verify_analysis_warnings_hello_world_app_trim_mode_copyused(s
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
var result = publishCommand.Execute($"/p:RuntimeIdentifier={rid}", $"/p:SelfContained=true", "/p:PublishTrimmed=true", "/p:SuppressTrimAnalysisWarnings=false");
var result = publishCommand.Execute($"/p:RuntimeIdentifier={rid}", $"/p:SelfContained=true", "/p:PublishTrimmed=true", "/p:TrimMode=copyused");
result.Should().Pass();
ValidateWarningsOnHelloWorldApp(publishCommand, result, expectedOutput, targetFramework, rid);
}
Expand All @@ -512,7 +560,7 @@ public void ILLink_verify_analysis_warnings_hello_world_app_trim_mode_link(strin
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
var result = publishCommand.Execute($"/p:RuntimeIdentifier={rid}", $"/p:SelfContained=true", "/p:PublishTrimmed=true", "/p:SuppressTrimAnalysisWarnings=false", "/p:TrimMode=link");
var result = publishCommand.Execute($"/p:RuntimeIdentifier={rid}", $"/p:SelfContained=true", "/p:PublishTrimmed=true");
result.Should().Pass();
ValidateWarningsOnHelloWorldApp(publishCommand, result, expectedOutput, targetFramework, rid);
}
Expand Down
1 change: 1 addition & 0 deletions src/WebSdk/Web/Sdk/Sdk.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- Default properties that shouldn't be replaced by the microsoft.net.sdk.props -->
<PropertyGroup>
<DebugSymbols Condition="'$(DebugSymbols)' == ''">true</DebugSymbols>
<SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == ''">true</SuppressTrimAnalysisWarnings>
</PropertyGroup>

<Import Sdk="Microsoft.NET.Sdk" Project="Sdk.props" />
Expand Down