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 all commits
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
42 changes: 32 additions & 10 deletions src/ILLink.Tasks/LinkTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public class ILLink : ToolTask
public ITaskItem OutputDirectory { get; set; }

/// <summary>
/// The subset of warnings that have to be turned off.
/// The subset of warnings that have to be turned off.
/// Maps to '--nowarn'.
/// </summary>
public string NoWarn { get; set; }
Expand Down Expand Up @@ -200,6 +200,7 @@ public class ILLink : ToolTask
/// <summary>
/// Sets the default action for assemblies which have not opted into trimming.
/// Maps to '--action'
/// </summary>
public string DefaultAction { get; set; }

/// <summary>
Expand Down Expand Up @@ -312,6 +313,23 @@ protected override string GenerateResponseFileCommands ()
args.AppendLine ("--singlewarn-");
}

string trimMode = TrimMode switch {
"full" => "link",
"partial" => "link",
var x => x
};
if (trimMode != null)
args.Append ("--trim-mode ").AppendLine (trimMode);

string defaultAction = TrimMode switch {
"full" => "link",
"partial" => "copy",
_ => DefaultAction
};
if (defaultAction != null)
args.Append ("--action ").AppendLine (defaultAction);


HashSet<string> assemblyNames = new HashSet<string> (StringComparer.OrdinalIgnoreCase);
foreach (var assembly in AssemblyPaths) {
var assemblyPath = assembly.ItemSpec;
Expand All @@ -323,10 +341,20 @@ protected override string GenerateResponseFileCommands ()

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

string trimMode = assembly.GetMetadata ("TrimMode");
if (!String.IsNullOrEmpty (trimMode)) {
string assemblyTrimMode = assembly.GetMetadata ("TrimMode");
string isTrimmable = assembly.GetMetadata ("IsTrimmable");
if (string.IsNullOrEmpty (assemblyTrimMode)) {
if (isTrimmable.Equals ("true", StringComparison.OrdinalIgnoreCase)) {
// isTrimmable ~= true
assemblyTrimMode = trimMode;
} else if (isTrimmable.Equals ("false", StringComparison.OrdinalIgnoreCase)) {
// isTrimmable ~= false
assemblyTrimMode = "copy";
}
}
if (!string.IsNullOrEmpty (assemblyTrimMode)) {
args.Append ("--action ");
args.Append (trimMode);
args.Append (assemblyTrimMode);
args.Append (' ').AppendLine (Quote (assemblyName));
}

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

if (TrimMode != null)
args.Append ("--trim-mode ").AppendLine (TrimMode);

if (DefaultAction != null)
args.Append ("--action ").AppendLine (DefaultAction);

if (CustomSteps != null) {
foreach (var customStep in CustomSteps) {
args.Append ("--custom-step ");
Expand Down
39 changes: 18 additions & 21 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 @@ -159,7 +159,7 @@ Copyright (c) .NET Foundation. All rights reserved.
ReferenceAssemblyPaths="@(ReferencePath)"
RootAssemblyNames="@(TrimmerRootAssembly)"
TrimMode="$(TrimMode)"
DefaultAction="$(TrimmerDefaultAction)"
DefaultAction="$(_TrimmerDefaultAction)"
RemoveSymbols="$(TrimmerRemoveSymbols)"
FeatureSettings="@(_TrimmerFeatureSettings)"
CustomData="@(_TrimmerCustomData)"
Expand Down Expand Up @@ -225,16 +225,23 @@ 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>
<!-- In .NET 7, TrimmerDefaultAction is deprecated. TrimMode can be used for the supported configurations. -->
<Warning Condition="'$(TrimmerDefaultAction)' != '' And $([MSBuild]::VersionGreaterThan('$(TargetFrameworkVersion)', '6.0'))"
Text="Property 'TrimmerDefaultAction' is deprecated in .NET 7 and will be ignored. Use TrimMode instead." />

<!-- Set up TrimMode and TrimmerDefaultAction. -->
<PropertyGroup>
<TrimMode Condition="'$(TrimMode)' == '' And $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '6.0'))">copyused</TrimMode>
<TrimMode Condition="'$(TrimMode)' == '' And $([MSBuild]::VersionEquals($(TargetFrameworkVersion), '6.0'))">partial</TrimMode>
<TrimMode Condition="'$(TrimMode)' == ''">full</TrimMode>

<!-- Set TrimmerDefaultAction for compat in < 7.0 -->
<_TrimmerDefaultAction Condition="$([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '7.0'))">$(TrimmerDefaultAction)</_TrimmerDefaultAction>
<_TrimmerDefaultAction Condition="'$(_TrimmerDefaultAction)' == '' And $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '6.0'))">$(TrimMode)</_TrimmerDefaultAction>
<_TrimmerDefaultAction Condition="'$(_TrimmerDefaultAction)' == '' And $([MSBuild]::VersionEquals($(TargetFrameworkVersion), '6.0'))">copy</_TrimmerDefaultAction>
</PropertyGroup>

<PropertyGroup>
<TrimMode Condition=" '$(TrimMode)' == '' ">link</TrimMode>
<!-- 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>
<TrimmerSingleWarn Condition=" '$(TrimmerSingleWarn)' == '' ">true</TrimmerSingleWarn>
Expand Down Expand Up @@ -295,16 +302,6 @@ Copyright (c) .NET Foundation. All rights reserved.
</ManagedAssemblyToLink>
</ItemGroup>

<!-- In .NET6+, set the action explicitly for any with IsTrimmable MSBuild metadata -->
<ItemGroup Condition=" $([MSBuild]::VersionGreaterThanOrEquals('$(TargetFrameworkVersion)', '6.0')) ">
<ManagedAssemblyToLink Condition=" '%(ManagedAssemblyToLink.IsTrimmable)' == 'true' And '%(ManagedAssemblyToLink.TrimMode)' == '' ">
<TrimMode>$(TrimMode)</TrimMode>
</ManagedAssemblyToLink>
<ManagedAssemblyToLink Condition=" '%(ManagedAssemblyToLink.IsTrimmable)' == 'false' And '%(ManagedAssemblyToLink.TrimMode)' == '' ">
<TrimMode>$(TrimmerDefaultAction)</TrimMode>
</ManagedAssemblyToLink>
</ItemGroup>

<ItemGroup>
<!-- Don't collapse to a single warning for the intermediate assembly.
This just sets metadata on the items in ManagedAssemblyToLink that came from IntermediateAssembly. -->
Expand Down
39 changes: 39 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 @@ -56,6 +56,45 @@ public class TaskArgumentTests
}
};

[Theory]
[InlineData ("full", AssemblyAction.Link)]
[InlineData ("partial", AssemblyAction.Copy)]
public void TrimModeFullAndPartial (string trimMode, AssemblyAction expectedDefaultAction)
{
var task = new MockTask () {
TrimMode = trimMode
};
using (var driver = task.CreateDriver ()) {
Assert.Equal (AssemblyAction.Link, driver.Context.TrimAction);
Assert.Equal (expectedDefaultAction, driver.Context.DefaultAction);
}
}

[Theory]
[InlineData ("full")]
[InlineData ("partial")]
public void TrimModeAssemblyPaths (string trimMode)
{
var assemblyPaths = new ITaskItem[] {
new TaskItem("Assembly1.dll", new Dictionary<string, string> {{ "IsTrimmable", "true" }}),
new TaskItem("Assembly2.dll", new Dictionary<string, string> ()),
new TaskItem("Assembly3.dll", new Dictionary<string, string> {{ "IsTrimmable", "false" }}),
};
var task = new MockTask () {
TrimMode = trimMode,
AssemblyPaths = assemblyPaths
};
using var driver = task.CreateDriver ();
var context = driver.Context;
var references = driver.GetReferenceAssemblies ();
Assert.Equal ("", assemblyPaths[0].GetMetadata ("TrimMode"));
Assert.Equal (AssemblyAction.Link, context.Actions["Assembly1"]);
Assert.Equal ("", assemblyPaths[1].GetMetadata ("TrimMode"));
Assert.False (context.Actions.ContainsKey ("Assembly2"));
Assert.Equal ("", assemblyPaths[2].GetMetadata ("TrimMode"));
Assert.Equal (AssemblyAction.Copy, context.Actions["Assembly3"]);
}

[Theory]
[MemberData (nameof (AssemblyPathsCases))]
public void TestAssemblyPaths (ITaskItem[] assemblyPaths)
Expand Down