Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Commit

Permalink
Update ILLinkTrimAssembly condition
Browse files Browse the repository at this point in the history 
Make it so that we don't set ILLinkTrimAssembly if its already set.
  • Loading branch information
@ericstj
ericstj committed Apr 4, 2017
1 parent 2277db9 commit 145762c
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions dir.targets
View file
Expand Up @@ -60,8 +60,8 @@
<PackageFileRefPath Condition="'$(IsNETCoreAppRef)' == 'true'">$(NETCoreAppPackageRefPath)</PackageFileRefPath>
<PackageFileRuntimePath>$(NETCoreAppPackageRuntimePath)</PackageFileRuntimePath>
<RuntimePath Condition="'$(BinPlaceNETCoreAppPackage)' == 'true'">$(NETCoreAppPackageRuntimePath)\..\runtime</RuntimePath>
<!-- enable trimming for any project that's part of the shared framework -->
<SetProperties Condition="'$(IsRuntimeAssembly)' == 'true'">ILLinkTrimAssembly=true</SetProperties>
<!-- enable trimming for any runtime project that's part of the shared framework and hasn't already set ILLinkTrimAssembly -->

This comment has been minimized.

Sign in to view

Copy link
@stephentoub

stephentoub Jun 20, 2018

Member

@ericstj, why do we limit this to only be for assemblies in the shared framework? I was surprised today to find we don't run this on System.Drawing.Common, for example.

This comment has been minimized.

Sign in to view

Copy link
@ericstj

ericstj Jun 20, 2018

Author Member

It was a scoping decision back when we first added it. Folks weren't willing to test the output of things that weren't going in the shared framework (eg: UAP assemblies, packages that would be used by desktop, etc) so we constrained it to only the shared framework. Folks were confident that we had better test coverage of the shared framework since that's what we were shipping at the time. We could broaden that scope but we'd probably want to make sure such a change had decent test coverage + auditing of the new assemblies it touched across all their supported frameworks.

This comment has been minimized.

Sign in to view

Copy link
@stephentoub

stephentoub Jun 20, 2018

Member

Hmm. Ok, thanks for the background info.

This comment has been minimized.

Sign in to view

Copy link
@danmoseley

danmoseley Jun 20, 2018

Member

We should have enough test coverage of the Windows compat pack assemblies. What would the change be if I wanted to include those?

This comment has been minimized.

Sign in to view

Copy link
@ericstj

ericstj via email Jun 20, 2018

Author Member
<SetProperties Condition="'$(BinPlaceRuntime)' == 'true' AND '$(ILLinkTrimAssembly)' == ''">ILLinkTrimAssembly=true</SetProperties>
</BinPlaceConfiguration>
<BinPlaceConfiguration Condition="'$(IsUAP)' == 'true' AND '$(BuildingUAPVertical)' == 'true'" Include="uap-$(_bc_OSGroup)">
<PackageFileRefPath Condition="'$(IsUAPRef)'=='true'">$(UAPPackageRefPath)</PackageFileRefPath>
Expand Down

0 comments on commit 145762c

Please sign in to comment.