Use correct CoreLib configuration for shared framework linker trimming#45821
Use correct CoreLib configuration for shared framework linker trimming#45821layomia merged 5 commits intodotnet:masterfrom
Conversation
|
Tagging subscribers to this area: @safern, @ViktorHofer Issue Details
|
|
Curious, why does the |
|
@ViktorHofer - Any thoughts on how we could flow the |
|
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 |
|
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 |
|
This is a point in time problem until we start binplacing CoreLib... |
Yup, #40172 (comment). |
Thanks, trying this out now. What problem do we avoid by removing the |
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. |
| <!-- Retrieve CoreLib's targetpath via GetTargetPath as it isn't binplaced yet. --> | ||
| <!-- Retrieve CoreLib's path as it isn't binplaced alongside the libraries. --> | ||
| <MSBuild Projects="$(CoreLibProject)" | ||
| Properties="Configuration=$(RuntimeConfiguration)" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
Yes, my workaround was to build a Debug CoreLib. This is what gets passed to the linker today with |
|
I see, I wasn't sure if we had passed this point in time :)
Can you either close this PR and file an issue, or address @ViktorHofer's feedback and merge? |
|
I'll address the feedback and merge, since it's a small effort at this point. |
| <UndefineProperties>$(UndefineProperties);TargetFramework;Platform</UndefineProperties> | ||
| <!-- If conflicting, manually set the Configuration property of the CoreLib project so that it aligns with the specified RuntimeConfiguration in the libraries' build. --> | ||
| <Properties Condition="'$(RuntimeFlavor)' == 'CoreCLR' and | ||
| '$(Configuration)' != '$(CoreCLRConfiguration)'">Configuration=$(CoreCLRConfiguration)</Properties> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
b05b83a to
c59fc16
Compare
|
CI failures are unrelated - dup of #47374. |
From #45821 (comment):
GetTargetPathis 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 asbuild -s clr+libs -rc Release, we are passingDebuglibraries andDebugCoreLib, rather thanDebuglibraries andReleaseCoreLib to the linker which is not expected.