dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.72k forks source link

System.Private.CoreLib's "Platform" is always set to AnyCPU when building inside Visual Studio which causes build failures #97984

Open ViktorHofer opened 8 months ago

ViktorHofer commented 8 months ago

Reported in https://github.com/dotnet/runtime/pull/97969

When building System.Private.CoreLib as a dependency of i.e. System.Threading.csproj, the Platform global property is always set to AnyCPU even though dotnet/runtime sets it to TargetArchitecture: https://github.com/dotnet/runtime/blob/cb0e160e50593ab014064a2e675b8d2e54ca3107/src/coreclr/Directory.Build.props#L3

Looking at a full fidelity binlog created by the project system tools, I can see that this is related to our custom Microsoft.DotNet.Build.Tasks.TargetFramework component:

image

The target from our custom component is slightly different than what is inbox in msbuild: https://github.com/dotnet/msbuild/blob/bea6b4aebe6548d714ca643db9107162965b94d5/src/Tasks/Microsoft.Common.CurrentVersion.targets#L1755-L1951

Probably most relevant is that our custom target doesn't support the EnableDynamicPlatformResolution setting which might be the reason why SetPlatform is passed in with a value of "AnyCPU".

This causes builds to fail, i.e. .\build.cmd -os browser -subset mono+libs && .\build.cmd -vs src\libraries\System.Runtime.InteropServices.JavaScript\System.Runtime.InteropServices.JavaScript.sln -rf Mono -os browser:

image

cc @ericstj

ghost commented 8 months ago

Tagging subscribers to this area: @dotnet/runtime-infrastructure See info in area-owners.md if you want to be subscribed.

Issue Details
Reported in https://github.com/dotnet/runtime/pull/97969 When building System.Private.CoreLib as a dependency of i.e. `System.Threading.csproj`, the `Platform` global property is always set to AnyCPU even though dotnet/runtime sets it to `TargetArchitecture`: https://github.com/dotnet/runtime/blob/cb0e160e50593ab014064a2e675b8d2e54ca3107/src/coreclr/Directory.Build.props#L3 Looking at a full fidelity binlog created by the project system tools, I can see that this is related to our custom Microsoft.DotNet.Build.Tasks.TargetFramework component: ![image](https://github.com/dotnet/runtime/assets/7412651/b4fd50d7-fd89-43b4-bc99-2e287f8a1a49) The target from our custom component is slightly different than what is inbox in msbuild: https://github.com/dotnet/msbuild/blob/bea6b4aebe6548d714ca643db9107162965b94d5/src/Tasks/Microsoft.Common.CurrentVersion.targets#L1755-L1951 Probably most relevant is that our custom target doesn't support the `EnableDynamicPlatformResolution` setting which might be the reason why SetPlatform is passed in with a value of "AnyCPU". This causes builds to fail, i.e.: ![image](https://github.com/dotnet/runtime/assets/7412651/ec1d205a-c2d5-454d-8fae-26a0c205a552) cc @ericstj
Author: ViktorHofer
Assignees: -
Labels: `area-Infrastructure`
Milestone: -
ericstj commented 8 months ago

Seems like you're on to something. IIRC the way the TargetFramework selection works is we run a target early to calculate the TargetFramework properties ourselves, then we set SkipGetTargetFrameworkProperties on those projects to apply the TFM.

I noticed this: https://github.com/dotnet/msbuild/blob/bea6b4aebe6548d714ca643db9107162965b94d5/src/Tasks/Microsoft.Common.CurrentVersion.targets#L1834-L1860

The second call when '$(EnableDynamicPlatformResolution)' == 'true'" removes additional properties Platform;Configuration;

@ViktorHofer - maybe give a try of modifying https://github.com/dotnet/arcade/blob/8cfc9489d3e51071fedec9dcb99071dc912718bd/src/Microsoft.DotNet.Build.Tasks.TargetFramework/src/build/Microsoft.DotNet.Build.Tasks.TargetFramework.targets#L43-L54 to do this and see if it fixes the issue?

dotnet-policy-service[bot] commented 3 months ago

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries See info in area-owners.md if you want to be subscribed.

ViktorHofer commented 3 months ago

We don't opt into using EnableDynamicPlatformResolution in runtime. That second call in the Common.targets from the link above can be ignored.