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

Jump to bottom

Remove hardcoded non trimmable assemblies in oob.proj #72171

Merged
merged 3 commits into from
Jul 14, 2022
Merged

Remove hardcoded non trimmable assemblies in oob.proj #72171

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
871e197
Remove hardcoded non trimmable assemblies in oob.proj
ViktorHofer Jul 14, 2022
717163e
Update oob.proj
ViktorHofer Jul 14, 2022
4738171
Update illink.targets
ViktorHofer Jul 14, 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
8 changes: 8 additions & 0 deletions eng/illink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@
<!-- ApiCompat should perform compatibility checks on the trimmed assemblies. -->
<ApiCompatDependsOn>$(ApiCompatDependsOn);ILLinkTrimAssembly</ApiCompatDependsOn>
</PropertyGroup>

<!-- Flow the IsTrimmable property down to consuming repositories, in order for oob.proj
ViktorHofer marked this conversation as resolved.
Show resolved Hide resolved
to exclude non trimmable assemblies. -->
<ItemDefinitionGroup>
<TargetPathWithTargetPlatformMoniker>
<IsTrimmable>$(IsTrimmable)</IsTrimmable>
</TargetPathWithTargetPlatformMoniker>
</ItemDefinitionGroup>

<!-- Inputs and outputs of ILLinkTrimAssembly -->
<PropertyGroup>
Expand Down
25 changes: 2 additions & 23 deletions src/libraries/oob.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,29 +45,8 @@
<!-- Include suppression XML files bin-placed in earlier per-library linker run. -->
<OOBLibrarySuppressionsXml Include="$(ILLinkTrimAssemblyOOBSuppressionsXmlsDir)*.xml" />

<!-- The following is the list of all the OOBs we will ignore for now -->
<OOBAssemblyToIgnore Include="System.CodeDom;
System.ComponentModel.Composition;
System.ComponentModel.Composition.Registration;
System.Composition.AttributedModel;
System.Composition.Convention;
System.Composition.Hosting;
System.Composition.Runtime;
System.Composition.TypedParts;
System.Configuration.ConfigurationManager;
System.Speech;
Microsoft.Extensions.DependencyInjection.Specification.Tests" />

<!-- Move items to FileName so that we can subtract them. -->
<OOBAssemblyWithFilename Include="@(OOBAssembly->Metadata('Filename'))"
OriginalIdentity="%(Identity)" />
<OOBAssemblyToTrimWithFilename Include="@(OOBAssemblyWithFilename)"
Exclude="@(OOBAssemblyToIgnore)" />
<OOBAssemblyToIgnoreWithFilename Include="@(OOBAssemblyWithFilename)"
Exclude="@(OOBAssemblyToTrimWithFilename)" />

<OOBAssemblyToTrim Include="@(OOBAssemblyToTrimWithFilename->Metadata('OriginalIdentity'))" />
<OOBAssemblyReference Include="@(OOBAssemblyToIgnoreWithFilename->Metadata('OriginalIdentity'));
<OOBAssemblyToTrim Include="@(OOBAssembly->WithMetadataValue('IsTrimmable', 'true'))" />
<OOBAssemblyReference Include="@(OOBAssembly->WithMetadataValue('IsTrimmable', 'false'));
Copy link
Member

Choose a reason for hiding this comment

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

I know this was existing behavior, but I find it kind of weird that we are passing in these "untrimmable" assemblies as References for the trimmable assemblies.

@joperezr - do you remember why it was done this way? I would hope we don't have any "trimmable" assemblies that reference these "untrimmable" assemblies... that would seem backwards.

Copy link
Member Author

@ViktorHofer ViktorHofer Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is what you get when you don't pass them in as references:

  oob -> Trimming win-x64 out-of-band assemblies with ILLinker...
C:\git\runtime2\src\libraries\System.Diagnostics.PerformanceCounter\src\System\Diagnostics\DiagnosticsConfiguration.cs(
69,13): Trim analysis warning IL2075: System.Diagnostics.DiagnosticsConfiguration.CanInitialize(): 'this' argument does
 not satisfy 'DynamicallyAccessedMemberTypes.NonPublicProperties' in call to 'System.Type.GetProperty(String, BindingFl
ags)'. The return value of method 'System.Type.GetTypeFromHandle(RuntimeTypeHandle)' does not have matching annotations
. The source value must declare at least the same requirements as those declared on the target location it is assigned
to. [C:\git\runtime2\src\libraries\oob.proj]
C:\git\runtime2\src\libraries\System.Diagnostics.PerformanceCounter\src\System\Diagnostics\DiagnosticsConfiguration.cs(
86,17): Trim analysis warning IL2075: System.Diagnostics.DiagnosticsConfiguration.Initialize(): 'this' argument does no
t satisfy 'DynamicallyAccessedMemberTypes.NonPublicProperties' in call to 'System.Type.GetProperty(String, BindingFlags
)'. The return value of method 'System.Type.GetTypeFromHandle(RuntimeTypeHandle)' does not have matching annotations. T
he source value must declare at least the same requirements as those declared on the target location it is assigned to.
 [C:\git\runtime2\src\libraries\oob.proj]
ILLink : Trim analysis warning IL2062: System.Runtime.Caching.Configuration.MemoryCacheElement.Name: Value passed to pa
rameter 'type' of method 'System.ComponentModel.TypeConverterAttribute.TypeConverterAttribute(Type)' can not be statica
lly determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements. [C:\git\runtime2\src\libraries\oob.
proj]
ILLink : Trim analysis warning IL2062: System.Runtime.Caching.Configuration.MemoryCacheElement.Name: Value passed to pa
rameter 'type' of method 'System.ComponentModel.TypeConverterAttribute.TypeConverterAttribute(Type)' can not be statica
lly determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements. [C:\git\runtime2\src\libraries\oob.
proj]
ILLink : Trim analysis warning IL2062: System.Runtime.Caching.Configuration.MemoryCacheElement.Name: Value passed to pa
rameter 'type' of method 'System.ComponentModel.TypeConverterAttribute.TypeConverterAttribute(Type)' can not be statica
lly determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements. [C:\git\runtime2\src\libraries\oob.
proj]
ILLink : Trim analysis warning IL2062: System.Runtime.Caching.Configuration.MemoryCacheElement.PollingInterval: Value p
assed to parameter 'type' of method 'System.ComponentModel.TypeConverterAttribute.TypeConverterAttribute(Type)' can not
 be statically determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements. [C:\git\runtime2\src\lib
raries\oob.proj]
ILLink : Trim analysis warning IL2062: System.Runtime.Caching.Configuration.MemoryCacheElement.PollingInterval: Value p
assed to parameter 'type' of method 'System.ComponentModel.TypeConverterAttribute.TypeConverterAttribute(Type)' can not
 be statically determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements. [C:\git\runtime2\src\lib
raries\oob.proj]
ILLink : Trim analysis warning IL2062: System.Runtime.Caching.Configuration.MemoryCacheElement.PollingInterval: Value p
assed to parameter 'type' of method 'System.ComponentModel.TypeConverterAttribute.TypeConverterAttribute(Type)' can not
 be statically determined and may not meet 'DynamicallyAccessedMembersAttribute' requirements. [C:\git\runtime2\src\lib
raries\oob.proj]

Copy link
Member Author

@ViktorHofer ViktorHofer Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I.e. PerformanceCounter relies on ConfigurationManager here:

runtime/src/libraries/System.Diagnostics.PerformanceCounter/src/System/Diagnostics/DiagnosticsConfiguration.cs

Line 69 in f249dc7

bool setConfigurationSystemInProgress = (bool)(typeof(ConfigurationManager).GetProperty("SetConfigurationSystemInProgress", BindingFlags.NonPublic | BindingFlags.Static).GetValue(null));

Runtime.Caching depends on ConfigurationManager as well:

runtime/src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/Configuration/MemoryCacheElement.cs

Line 72 in c9feedf

[StringValidator(MinLength = 1)]

Copy link
Member

Choose a reason for hiding this comment

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

System.Runtime.Caching also relies on ConfigurationManager:

runtime/src/libraries/System.Runtime.Caching/src/System.Runtime.Caching.csproj

Line 85 in f249dc7

<ProjectReference Include="$(LibrariesProjectRoot)System.Configuration.ConfigurationManager\src\System.Configuration.ConfigurationManager.csproj" />

Copy link
Member Author

@ViktorHofer ViktorHofer Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I verified that ConfigurationManager is the only dependency that needs to be passed in. Should we filter this down and just pass ConfigurationManager in? Of course, ideally we would not have this dependency at all.

Is there an issue that tracks marking these remaining assemblies as trimmable? (if that's even possible)

Copy link
Member

Choose a reason for hiding this comment

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

It is not necessarily wrong for trimmable assembly to depend on non-trimmable assembly. The trimmable assembly may have this dependency just for a small part of its functionality. If your app does not use this small part, it is still going be fine with trimming.

Is there an issue that tracks marking these remaining assemblies as trimmable

#49062

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 filter this down and just pass ConfigurationManager in?

No, I would leave it for now.

@(SharedFrameworkAssembly)" />
</ItemGroup>
</Target>
Expand Down