Skip to content

add .NET 6 target framework for System.CommandLine and add support for trimming#1572

Merged
jonsequitur merged 13 commits into
dotnet:mainfrom
jonsequitur:trimmability
Jan 22, 2022
Merged

add .NET 6 target framework for System.CommandLine and add support for trimming#1572
jonsequitur merged 13 commits into
dotnet:mainfrom
jonsequitur:trimmability

Conversation

@jonsequitur
Copy link
Copy Markdown
Contributor

@jonsequitur jonsequitur commented Jan 9, 2022

This is so far just a sketch of an approach to support trimming of apps using System.CommandLine. There are a large number of types that won't be bindable without a custom binder or source generator. This also removes support for using TypeConverterAttribute and default type converters.

This will fix #1465.

else
if (type == typeof(string))
{
enumerableInterface = type
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typically this kind of pattern is trimming safe, but the trimmer doesn't know that you are only looking for one interface. Since you are hard-coding looking for typeof(IEnumerable), the trimmer will never trim the IEnumerable interface. It could trim other interfaces (which is why GetInterfaces is marked as unsafe). But you don't care about the other interfaces, so you can suppress this.

See the following for the same examples, and reasoning:

https://github.com/dotnet/runtime/blob/cf5b231fcbea483df3b081939b422adfb6fd486a/src/libraries/System.Linq.Queryable/src/System/Linq/EnumerableRewriter.cs#L140-L157

https://github.com/dotnet/runtime/blob/cf5b231fcbea483df3b081939b422adfb6fd486a/src/libraries/System.Linq.Queryable/src/System/Linq/EnumerableRewriter.cs#L204-L213

</PropertyGroup>

<ItemGroup>
<TrimmableAssembly Include="System.CommandLine" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is needed since you set IsTrimmable=true in the assembly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

</PropertyGroup>

<PropertyGroup>
<SystemCommandLineDllPath Condition="'$(SystemCommandLineDllPath)'==''">..\..\System.CommandLine\bin\Release\netstandard2.0\System.CommandLine.dll</SystemCommandLineDllPath>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not just a ProjectReference?

Also - don't point to ns2.0 build. Point to net6.0.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since you aren't using a ProjectReference, all the warnings from System.CommandLine are getting collapsed into a single warning IL2014.

Here's the logic that says to not collapse warnings for ProjectReferences:

https://github.com/dotnet/sdk/blob/8f1a762aee466715a984f5757a622b85daba5387/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L322-L325

If you can't use a ProjectReference here, you can still get full warnings for this assembly by setting the TrimmerSingleWarn=false metadata on the System.CommandLine assembly.

Alternatively, since your app only uses System.CommandLine, it would be simpler to just turn off the single warning behavior globally:

<PropertyGroup>
  <TrimmerSingleWarn>false</TrimmerSingleWarn>

fyi @sbomer

Comment thread src/System.CommandLine/Binding/ArgumentConverter.DefaultValues.cs Outdated
@@ -0,0 +1,97 @@
// adapted from: https://github.com/dotnet/runtime/blob/a5159b1a8840632ad34cf59c5aaf77040cb6ceda/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/UnconditionalSuppressMessageAttribute.cs#L21

#if !NET6_0_OR_GREATER
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(nit) you use NET5_0_OR_GREATER elsewhere, but 6 here.

@jonsequitur jonsequitur marked this pull request as ready for review January 21, 2022 23:03
@jonsequitur jonsequitur changed the title add support for trimming add .NET 6 target framework for System.CommandLine and add support for trimming Jan 21, 2022
@jonsequitur jonsequitur requested a review from Keboo January 21, 2022 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IL2104 when trimming application using System.CommandLine

2 participants