Use correct CoreLib configuration for shared framework linker trimming

[layomia]

From #45821 (comment):

GetTargetPath is returning the path to CoreLib based on the libraries configuration (-lc), rather than the runtime configuration (-rc). This means that when we do a build such as build -s clr+libs -rc Release, we are passing Debug libraries and Debug CoreLib, rather than Debug libraries and Release CoreLib to the linker which is not expected.

[ghost]

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Author:	layomia
Assignees:	layomia
Labels:	area-Infrastructure-libraries
Milestone:	-

[ViktorHofer]

Curious, why does the GetTargetPath target not work for you? What are you trying to solve with this PR?

[layomia]

@ViktorHofer - GetTargetPath is returning the path to CoreLib based on the libraries configuration (-lc), rather than the runtime configuration (-rc). This means that when we do a build such as build -s clr+libs -rc Release, we are passing Debug libraries and Debug CoreLib, rather than Debug libraries and Release CoreLib to the linker which is not expected.

Any thoughts on how we could flow the RuntimeConfiguration prop to GetTargetPath, or thoughts on the current approach using CoreCLRArtifactsPath?

cc @eerhardt, @joperezr

[ViktorHofer]

Yes, you could do something similar to what we do for the ProjectReference to CoreLib:

runtime/src/libraries/Directory.Build.targets

Lines 175 to 179 in dcbf6e9

	<UndefineProperties>$(UndefineProperties);TargetFramework;Platform</UndefineProperties>
	<SetConfiguration Condition="'$(RuntimeFlavor)' == 'CoreCLR' and
	'$(Configuration)' != '$(CoreCLRConfiguration)'">Configuration=$(CoreCLRConfiguration)</SetConfiguration>
	<SetConfiguration Condition="'$(RuntimeFlavor)' == 'Mono' and
	'$(Configuration)' != '$(MonoConfiguration)'">Configuration=$(MonoConfiguration)</SetConfiguration>

[ViktorHofer]

You can pass in the "Properties" and "RemoveProperties" properties to the MSBuild task: https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-task?view=vs-2019

Pass in $(RuntimeConfiguration) as the value of configuration in the Properties property and just to be on the safe side, remove the Platform and TargetFramework properties.

[ViktorHofer]

This is a point in time problem until we start binplacing CoreLib...

[layomia]

This is a point in time problem until we start binplacing CoreLib...

Yup, #40172 (comment).

[layomia]

You can pass in the "Properties" and "RemoveProperties" properties to the MSBuild task: https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-task?view=vs-2019

Pass in $(RuntimeConfiguration) as the value of configuration in the Properties property and just to be on the safe side, remove the Platform and TargetFramework properties.

Thanks, trying this out now. What problem do we avoid by removing the Platform and TargetFramework properties?

[ViktorHofer]

What problem do we avoid by removing the Platform and TargetFramework properties?

We do this for performance as msbuild caches project evaluations based on the global properties passed in. Doing this here might reduce an unnecessary project evaluation of CoreLib by using the same global properties as for the ProjectReference to CoreLib.

[ViktorHofer]

With this, the Configuration property is always passed in as a global property even if the RuntimeConfiguration already matches the current (libraries) configuration. We should keep this in sync with how we set global properties for the ProjectReference to CoreLib.

[ViktorHofer]

add this to a new line before L38.

         <Properties Condition="'$(RuntimeFlavor)' == 'CoreCLR' and 
                                '$(Configuration)' != '$(CoreCLRConfiguration)'">Configuration=$(CoreCLRConfiguration)</Properties> 
         <Properties Condition="'$(RuntimeFlavor)' == 'Mono' and 
                                '$(Configuration)' != '$(MonoConfiguration)'">Configuration=$(MonoConfiguration)</Properties> 

[ericstj]

@layomia @eerhardt @joperezr is this PR still relevant?

[layomia]

@ericsj - I believe so. IIRC @eerhardt had a work around locally for the issue, but we need a proper fix checked in for correctness. I'll go ahead and drive this in.

[eerhardt]

IIRC @eerhardt had a work around locally for the issue

Yes, my workaround was to build a Debug CoreLib.

This is what gets passed to the linker today with dotnet build .\src\libraries\src.proj /t:ILLinkTrimSharedFramework /p:BuildingNetCoreAppVertical=true /p:RuntimeConfiguration=Release. Note the CoreLib path is to Debug even though I said /p:RuntimeConfiguration=Release.

[ericstj]

I see, I wasn't sure if we had passed this point in time :)

This is a point in time problem until we start binplacing CoreLib...

Can you either close this PR and file an issue, or address @ViktorHofer's feedback and merge?

[layomia]

I'll address the feedback and merge, since it's a small effort at this point.

[eerhardt]

Just a question, mostly for @ViktorHofer. Why do we have both $(CoreCLRConfiguration) and $(MonoConfiguration)? Why not just have $(RuntimeConfiguration)? Do we expect to be able to build the repo using different configurations for coreclr and mono?

[ViktorHofer]

There is a RuntimeConfiguration property but in this case we can't use that as we need to respect the inferred CoreCLRConfiguration and MonoConfiguration properties.

Do we expect to be able to build the repo using different configurations for coreclr and mono?

Yes

[layomia]

CI failures are unrelated - dup of #47374.