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

Add more trimming options to TrimMode and change defaults #2856

Merged
merged 14 commits into from
Jul 7, 2022
Merged

Add more trimming options to TrimMode and change defaults #2856

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
774a7b5
Add more trimming options to TrimMode and change defaults
agocke Jun 22, 2022
d284611
Fix test and cleanup
agocke Jun 22, 2022
67a526a
Lint
agocke Jun 22, 2022
6e5faa3
Respond to PR comments
agocke Jun 22, 2022
381e390
WIP
agocke Jun 22, 2022
92f1b11
Keep defaults in the targets and move full/partial logic to the task
agocke Jun 24, 2022
ea1ea46
Remove extra argument
agocke Jun 24, 2022
06fcb73
Remove _LinkFramework
agocke Jun 24, 2022
17055e1
Duplicate condition
agocke Jun 24, 2022
46518c5
Set nothing for assemblies with blank IsTrimmable
agocke Jun 24, 2022
c7f5e88
Fix after change to actions
agocke Jul 1, 2022
11e5cd3
Lint
agocke Jul 1, 2022
83620fa
Remove unused parameter
agocke Jul 1, 2022
1d93fe4
Add more assembly metadata testing and change IsTrimmable=false to 'c…
agocke Jul 1, 2022
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
12 changes: 9 additions & 3 deletions src/ILLink.Tasks/ILLink.Tasks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,17 @@
</ItemGroup>

<ItemGroup>
<Content Include="sdk/Sdk.props" PackagePath="Sdk/" />
<Content Include="build/$(PackageId).props" PackagePath="build/" />
<Content Include="sdk/Sdk.props" PackagePath="Sdk/">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>
<Content Include="build/$(PackageId).props" PackagePath="build/">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>
<!-- Note: this should not match the package name, because we don't want the targets
to be imported by nuget. The SDK will import them in the right order. -->
<Content Include="build/Microsoft.NET.ILLink.targets" PackagePath="build/" />
<Content Include="build/Microsoft.NET.ILLink.targets" PackagePath="build/">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>
</ItemGroup>

<ItemGroup>
Expand Down
51 changes: 47 additions & 4 deletions src/ILLink.Tasks/LinkTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,13 @@ public class ILLink : ToolTask
public bool RemoveSymbols { set => _removeSymbols = value; }
bool? _removeSymbols;

/// <summary>
/// The version of the link task. Used to determine the meaning of certain properties.
/// Roughly maps to the TargetFrameworkVersion.
/// </summary>
[Required]
public int LinkVersion { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Works - but I would prefer if we simply passed something like TargetFrameworkVersion - could be the same numerical value, but make it clear that it's the TFM version - I don't want to introduce yet another version number to think about.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not to introduce any version to this task. The task should aim to be a straightforward mapping to the command-line arguments. Introducing version-based logic here increases the probability that we set version-specific defaults in the task that can't be overridden by the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to make that work, but that means we have to do all of the mapping logic in MSBuild properties. That seems much harder to get right, and also very hard to test

Copy link
Member

Choose a reason for hiding this comment

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

We should have end-to-end tests that exercise the targets+task either way.

It is unfortunately harder to get right. I personally think that is a price worth paying:

  • to make sure we don't break the ability to override defaults, and
  • to keep the task a thin wrapper for the tool (unless we want to introduce TFM-versioned logic to the tool itself).


/// <summary>
/// Sets the default action for trimmable assemblies.
/// Maps to '--trim-mode'
Expand Down Expand Up @@ -323,10 +330,10 @@ protected override string GenerateResponseFileCommands ()

args.Append ("-reference ").AppendLine (Quote (assemblyPath));

string trimMode = assembly.GetMetadata ("TrimMode");
if (!String.IsNullOrEmpty (trimMode)) {
string assemblyTrimMode = assembly.GetMetadata ("TrimMode");
agocke marked this conversation as resolved.
Show resolved Hide resolved
if (!String.IsNullOrEmpty (assemblyTrimMode)) {
args.Append ("--action ");
args.Append (trimMode);
args.Append (assemblyTrimMode);
args.Append (' ').AppendLine (Quote (assemblyName));
}

Expand Down Expand Up @@ -441,8 +448,44 @@ protected override string GenerateResponseFileCommands ()
if (_removeSymbols == false)
args.AppendLine ("-b");

string trimMode;
if (TrimMode != null)
args.Append ("--trim-mode ").AppendLine (TrimMode);
{
trimMode = TrimMode switch
{
"full" => "link",
sbomer marked this conversation as resolved.
Show resolved Hide resolved
"partial" => "link",
var x => x
agocke marked this conversation as resolved.
Show resolved Hide resolved
};
}
else
{
trimMode = LinkVersion switch
{
< 6 => "copyused",
sbomer marked this conversation as resolved.
Show resolved Hide resolved
_ => "link"
};
}
args.Append ("--trim-mode ").AppendLine (trimMode);

string defaultAction;
if (DefaultAction != null && LinkVersion < 7)
{
defaultAction = DefaultAction;
}
else
{
defaultAction = (LinkVersion, TrimMode) switch
{
(_, "full") => "link",
(_, "partial") => "copy",
(< 6, _) => trimMode, // Use resolved trim mode, not original
(6, _) => "copy",
_ => "link",
};
}
args.Append("--action ").AppendLine (defaultAction);


if (DefaultAction != null)
args.Append ("--action ").AppendLine (DefaultAction);
sbomer marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
30 changes: 18 additions & 12 deletions src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ Copyright (c) .NET Foundation. All rights reserved.
<PropertyGroup Condition="'$(SuppressTrimAnalysisWarnings)' == '' And '$(PublishTrimmed)' == 'true' And '$(EnableTrimAnalyzer)' != 'true'">
<!-- Trim analysis warnings are suppressed for .NET < 6. -->
<SuppressTrimAnalysisWarnings Condition="$([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '6.0'))">true</SuppressTrimAnalysisWarnings>
<!-- Suppress for WPF/WinForms (unless linking everything) -->
<SuppressTrimAnalysisWarnings Condition="'$(TrimmerDefaultAction)' != 'link' And ('$(UseWpf)' == 'true' Or '$(UseWindowsForms)' == 'true')">true</SuppressTrimAnalysisWarnings>
<!-- Suppress for WPF/WinForms -->
<SuppressTrimAnalysisWarnings Condition="'$(UseWpf)' == 'true' Or '$(UseWindowsForms)' == 'true'">true</SuppressTrimAnalysisWarnings>
Comment on lines -54 to +55
Copy link
Member

Choose a reason for hiding this comment

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

Why? If I trim WinForms app and ask for TrimMode=full, I should get all the warnings. Similarly I would expect this behavior in all other places, basically if I say TrimMode=full - I should get warnings by default (since I asked for the "hard " mode).

Copy link
Member Author

Choose a reason for hiding this comment

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

It's currently blocked off completely, which is why we hide the warnings at all. I think we should either show the warnings or disallow trimming.

Copy link
Member

Choose a reason for hiding this comment

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

OK - if we rely on the error for this, then I agree. We should at least add a comment here (why it's OK to suppress). Ideally if the error is disabled we would reenable the warnings (this should really only affect analyzer anyway, the error will trigger before we get to run the linker anyway)

<!-- Otherwise, for .NET 6+, warnings are on by default -->
<SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == ''">false</SuppressTrimAnalysisWarnings>
</PropertyGroup>
Expand Down Expand Up @@ -154,12 +154,21 @@ Copyright (c) .NET Foundation. All rights reserved.
<_DotNetHostFileName Condition="$([MSBuild]::IsOSPlatform(`Windows`))">dotnet.exe</_DotNetHostFileName>
</PropertyGroup>

<!-- Define a LinkVersion to support different options behavior based on TFM. -->
<PropertyGroup>
<_LinkVersion Condition="$([MSBuild]::VersionGreaterThan('$(TargetFrameworkVersion)', '6.0'))">7</_LinkVersion>
<_LinkVersion Condition="$([MSBuild]::VersionEquals('$(TargetFrameworkVersion)', '6.0'))">6</_LinkVersion>
<_LinkVersion Condition="$([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '6.0'))">5</_LinkVersion>
<_LinkVersion Condition="$([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '5.0'))">3</_LinkVersion>
</PropertyGroup>

<Delete Files="@(_LinkedResolvedFileToPublishCandidate)" />
<ILLink AssemblyPaths="@(ManagedAssemblyToLink)"
ReferenceAssemblyPaths="@(ReferencePath)"
RootAssemblyNames="@(TrimmerRootAssembly)"
LinkVersion="$(_LinkVersion)"
TrimMode="$(TrimMode)"
DefaultAction="$(TrimmerDefaultAction)"
DefaultAction="$(_TrimmerDefaultAction)"
RemoveSymbols="$(TrimmerRemoveSymbols)"
FeatureSettings="@(_TrimmerFeatureSettings)"
CustomData="@(_TrimmerCustomData)"
Expand Down Expand Up @@ -212,6 +221,10 @@ Copyright (c) .NET Foundation. All rights reserved.
potentially problematic when warnings are suppressed. -->
<NETSdkInformation Condition="'$(PublishTrimmed)' == 'true' And '$(SuppressTrimAnalysisWarnings)' == 'true'" ResourceName="ILLinkOptimizedAssemblies" />

<!-- In .NET 7, TrimmerDefaultAction is deprecated. TrimMode can be used for the supported configurations. -->
<Warning Condition="'$(TrimmerDefaultAction)' != '' And $([MSBuild]::VersionGreaterThan('$(TargetFrameworkVersion)', '6.0'))"
Copy link
Contributor

Choose a reason for hiding this comment

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

This change will need to be propagated to all SDKs like https://github.com/xamarin/xamarin-macios/blob/main/msbuild/Xamarin.iOS.Tasks.Windows/Xamarin.iOS.Common.After.targets#L333

Text="Property 'TrimmerDefaultAction' is deprecated in .NET 7." />
agocke marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Please make the warning actionable - maybe just add "Please use TrimMode instead" or something like that.


<!-- The defaults currently root non-framework assemblies, which
is a no-op for portable apps. If we later support more ways
to customize the behavior we can allow linking portable apps
Expand All @@ -225,16 +238,9 @@ Copyright (c) .NET Foundation. All rights reserved.
<ILLinkWarningLevel Condition=" '$(ILLinkWarningLevel)' == '' ">0</ILLinkWarningLevel>
</PropertyGroup>

<!-- Defaults for .NET < 6 -->
<PropertyGroup Condition=" $([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '6.0')) ">
<TrimMode Condition=" '$(TrimMode)' == '' ">copyused</TrimMode>
<!-- 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>
<!-- For .NET 6+, assemblies without IsTrimmable attribute get the "copy" action. -->
<TrimmerDefaultAction Condition=" '$(TrimmerDefaultAction)' == '' ">copy</TrimmerDefaultAction>
<!-- Set TrimmerDefaultAction for compat -->
<_TrimmerDefaultAction>$(TrimmerDefaultAction)</_TrimmerDefaultAction>
agocke marked this conversation as resolved.
Show resolved Hide resolved
<ILLinkTreatWarningsAsErrors Condition=" '$(ILLinkTreatWarningsAsErrors)' == '' ">$(TreatWarningsAsErrors)</ILLinkTreatWarningsAsErrors>
<_ExtraTrimmerArgs>--skip-unresolved true $(_ExtraTrimmerArgs)</_ExtraTrimmerArgs>
<TrimmerSingleWarn Condition=" '$(TrimmerSingleWarn)' == '' ">true</TrimmerSingleWarn>
Expand Down
80 changes: 80 additions & 0 deletions test/ILLink.Tasks.Tests/ILLink.Tasks.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,86 @@ public class TaskArgumentTests
}
}
};

[Theory]
[InlineData(3, AssemblyAction.CopyUsed)]
[InlineData(5, AssemblyAction.CopyUsed)]
[InlineData(6, AssemblyAction.Copy)]
[InlineData(7, AssemblyAction.Link)]
public void DefaultDefaultAction(int linkVersion, AssemblyAction expectedDefault)
{
var task = new MockTask() {
LinkVersion = linkVersion
};
using (var driver = task.CreateDriver()) {
Assert.Equal (expectedDefault, driver.Context.DefaultAction);
}
}

[Theory]
[InlineData(3, "copyused", AssemblyAction.CopyUsed)]
[InlineData(3, "copy", AssemblyAction.Copy)]
[InlineData(3, "link", AssemblyAction.Link)]
[InlineData(5, "copyused", AssemblyAction.CopyUsed)]
[InlineData(5, "copy", AssemblyAction.Copy)]
[InlineData(5, "link", AssemblyAction.Link)]
// Trim Mode settings don't affect DefaultAction after 5, unless 'full' or 'partial'
[InlineData(6, "copyused", AssemblyAction.Copy)]
[InlineData(6, "copy", AssemblyAction.Copy)]
[InlineData(6, "link", AssemblyAction.Copy)]
[InlineData(7, "copyused", AssemblyAction.Link)]
[InlineData(7, "copy", AssemblyAction.Link)]
[InlineData(7, "link", AssemblyAction.Link)]
// 'Full' and 'partial' are always respected
[InlineData(3, "full", AssemblyAction.Link)]
[InlineData(5, "full", AssemblyAction.Link)]
[InlineData(6, "full", AssemblyAction.Link)]
[InlineData(7, "full", AssemblyAction.Link)]
[InlineData(3, "partial", AssemblyAction.Copy)]
[InlineData(5, "partial", AssemblyAction.Copy)]
[InlineData(6, "partial", AssemblyAction.Copy)]
[InlineData(7, "partial", AssemblyAction.Copy)]
public void DefaultActionWithTrimMode(int linkVersion, string trimMode, AssemblyAction expectedDefault)
{
var task = new MockTask () {
LinkVersion = linkVersion,
TrimMode = trimMode
};
using var driver = task.CreateDriver();
Assert.Equal (expectedDefault, driver.Context.DefaultAction);
}

[Theory]
[InlineData(3, "copyused", AssemblyAction.CopyUsed)]
[InlineData(3, "copy", AssemblyAction.Copy)]
[InlineData(3, "link", AssemblyAction.Link)]
[InlineData(5, "copyused", AssemblyAction.CopyUsed)]
[InlineData(5, "copy", AssemblyAction.Copy)]
[InlineData(5, "link", AssemblyAction.Link)]
[InlineData(6, "copyused", AssemblyAction.CopyUsed)]
[InlineData(6, "copy", AssemblyAction.Copy)]
[InlineData(6, "link", AssemblyAction.Link)]
[InlineData(7, "copyused", AssemblyAction.CopyUsed)]
[InlineData(7, "copy", AssemblyAction.Copy)]
[InlineData(7, "link", AssemblyAction.Link)]
// 'Full' and 'partial' are always respected
[InlineData(3, "full", AssemblyAction.Link)]
[InlineData(5, "full", AssemblyAction.Link)]
[InlineData(6, "full", AssemblyAction.Link)]
[InlineData(7, "full", AssemblyAction.Link)]
[InlineData(3, "partial", AssemblyAction.Link)]
[InlineData(5, "partial", AssemblyAction.Link)]
[InlineData(6, "partial", AssemblyAction.Link)]
[InlineData(7, "partial", AssemblyAction.Link)]
public void TrimActionWithTrimMode(int linkVersion, string trimMode, AssemblyAction expectedDefault)
{
var task = new MockTask () {
LinkVersion = linkVersion,
TrimMode = trimMode
};
using var driver = task.CreateDriver();
Assert.Equal (expectedDefault, driver.Context.TrimAction);
}

[Theory]
[MemberData (nameof (AssemblyPathsCases))]
Expand Down
7 changes: 6 additions & 1 deletion test/ILLink.Tasks.Tests/Mock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,13 @@ public static IEnumerable<string> GetOptimizationPropertyNames ()

public class MockBuildEngine : IBuildEngine
{
public readonly List<BuildWarningEventArgs> Warnings = new List<BuildWarningEventArgs>();

public void LogErrorEvent (BuildErrorEventArgs e) { }
public void LogWarningEvent (BuildWarningEventArgs e) { }
public void LogWarningEvent (BuildWarningEventArgs e)
{
Warnings.Add(e);
}
public void LogMessageEvent (BuildMessageEventArgs e) { }
public void LogCustomEvent (CustomBuildEventArgs e) { }
public bool BuildProjectFile (string projectFileName, string[] targetNames, IDictionary globalProperties, IDictionary targetOutputs) => false;
Expand Down