Platform Negotiation fails between two projects that offer the same platform

[AArnott]

Issue Description

When library A references library B, and both support arm64 as a platform, building library A for arm64 should also build library B for arm64. But instead, library B builds for AnyCPU.

Steps to Reproduce

Check out this minimal repro:

platformnegotiation.zip

Run dotnet build lib2 /p:platform=arm64 and see how only lib2 builds for arm64 while lib1 (which lib2 references) builds for AnyCPU.

Expected Behavior

dotnet build lib2 /p:platform=arm64
  lib1 -> C:\temp\platformnegotiation\lib1\bin\arm64\Debug\net6.0\lib1.dll
  lib2 -> C:\temp\platformnegotiation\lib2\bin\arm64\Debug\net6.0\lib2.dll

Note the arm64 in the lib1 output path.

Actual Behavior

dotnet build lib2 /p:platform=arm64
  lib1 -> C:\temp\platformnegotiation\lib1\bin\Debug\net6.0\lib1.dll
  lib2 -> C:\temp\platformnegotiation\lib2\bin\arm64\Debug\net6.0\lib2.dll

Note the lack of the arm64 in the output path.

Analysis

When the _GetProjectReferencePlatformProperties target determines that a referenced project supports an exact match with the originating project, it thinks it can just drop the need to set the Platform (because it would propagate naturally, I guess):

Platform property of referenced project '..\x\x.csproj' matches current project's platform: 'arm64'. Referenced project will be built without a global Platform property.

But it doesn't work, because when the ProjectReference is ultimately built, it's passed with this metadata:

UndefineProperties = ;TargetFramework;Platform

That means the Platform property won't propagate. As a result, the referenced project will build with its default Platform instead of the matching one.

Versions & Configurations

This is with .NET SDK 7.0.100-preview.5.22307.18.

Note that this is impacting work that targets a .NET 6 SDK.

[benvillalobos]

I'm not able to repro what you're seeing. Are the projects set up properly? It looks like neither project has a default platform, should the command line repro be dotnet build lib2 /p:platform=arm64? If so, I think I see what you're seeing.

What's happening is that during the GetTargetFrameworks call on lib1, it gets passed the SetPlatform value (platform=arm64), which then gets picked up because in this target I assumed that $(Platform) is the "default" platform this project would have built as. This is very much an oversight.

Because of that, platform negotiation improperly detects arm64 as what lib1 would have built as, but as it builds it defaults to anycpu instead.

My next question, can we make this MSBuild call explicitly not pass any properties? This target does not rely on $(Platform) or $(Configuration), I'm not sure why it's passed. @dsplaisted might know.

msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets

Lines 1776 to 1786 in 62b690b

	<MSBuild
	Projects="@(_MSBuildProjectReferenceExistent)"
	Targets="GetTargetFrameworks"
	BuildInParallel="$(BuildInParallel)"
	Properties="%(_MSBuildProjectReferenceExistent.SetConfiguration); %(_MSBuildProjectReferenceExistent.SetPlatform)"
	ContinueOnError="!$(BuildingProject)"
	RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove);TargetFramework;RuntimeIdentifier$(_GlobalPropertiesToRemoveFromProjectReferences)"
	Condition="'%(_MSBuildProjectReferenceExistent.SkipGetTargetFrameworkProperties)' != 'true'"
	SkipNonexistentTargets="true">
	<Output TaskParameter="TargetOutputs" ItemName="_ProjectReferenceTargetFrameworkPossibilities" />
	</MSBuild>

Option number 2 is to separate MSBuild calls got GetTargetFrameworks based on the value of EnableDynamicPlatformResolution, and when true, not pass anything since we're already a special case.

[AArnott]

It looks like neither project has a default platform, should the command line repro be dotnet build lib2 /p:platform=arm64? If so, I think I see what you're seeing.

Yes, I believe you're right in that I dropped that switch from the repro steps. I've corrected the issue description.

[AArnott]

Reactivating as @benvillalobos requested, given I have more cases of P2P graphs that fail. Not all align exactly with the original issue, and we can certainly farm them out to distinct new issues if that's helpful.

These repro with:

MSBuild version 17.4.0-preview-22366-04+d2871ca13 for .NET Framework
17.4.0.36604

Newly discovered cases:

• TraversalPlusP2PToVcxproj.zip
  This includes a traversal project that simply references all project recursively, and an AnyCPU csproj that references a vcxproj with explicit SetPlatform metadata.
  Expected: When the dirs.proj is built, the vcxproj builds once, as its only platform (x64).
  Traversal projects should build all platforms of the projects they directly reference.
  Actual: The vcxproj builds twice: once (successfully) because of the csproj with the explicit SetPlatform metadata, and once more (failure) because the dirs.proj didn't supply any Platform global property.
• TraversalAndAnyCpuReferencesMultiPlatCsProj.zip
  This includes a traversal project that simply references all project recursively, and an AnyCPU csproj that references a multi-plat csproj.
  Expected: When the dirs.proj is built, the multi-plat csproj should be built twice (once per platform) and the AnyCPU csproj should build once.
  Actual: The multi-plat csproj is built twice as AnyCPU both times, leading to timing breaks. The AnyCPU csproj also builds once (which is correct).

High level requirements that I think fall out from these repro cases include:

1. All P2Ps should always specify a Platform global property, even if the default one would be the right one. Otherwise we end up in this world where a graph of projects may make divergent decisions regarding whether a particular platform of a project should be specified explicitly or not, leading to overbuild of the project. This also matches sln-based build behavior, so it has a strong precedent to be correct.
2. Traversals, which tend to be recursive, and certainly should not have to encode the implementation detail of all their underlying projects including all their allowed platforms, should dynamically 'negotiate' to build all platforms that those projects support. That in fact is what we already do for traversals in the VS repo already, but this is done through a proprietary technique instead of the msbuild feature.

[AArnott]

I would further suggest that the proposed traversal project capability of building all platforms of referenced projects should be exposed as metadata on a ProjectReference item so that any project can opt into that behavior, and the traversal SDK would simply set that metadata on the item definition. For example:

<ProjectReference Include="some.proj">
  <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
  <AllPlatforms>true</AllPlatforms>
</ProjectReference>

This would allow other project types such as setup/archival projects to build all platforms of a referenced project and consume their outputs in whatever way they need.

Alternatively, the metadata could be a semicolon-delimited list of platforms that should be built for that project. But if so, some special value (e.g. {all}) should allow avoiding hard-coding a list of platforms if the referencing project just wants them 'all'. Given that requirement, and that if you're willing to list a subset of platforms to be referenced you can already do that by writing out the ProjectReference multiple times, I'm leaning toward just a boolean metadata as proposed above.

[benvillalobos]

TraversalPlusP2PToVcxproj.zip
This includes a traversal project that simply references all project recursively, and an AnyCPU csproj that references a vcxproj with explicit SetPlatform metadata.
Expected: When the dirs.proj is built, the vcxproj builds once, as its only platform (x64).
Traversal projects should build all platforms of the projects they directly reference.
Actual: The vcxproj builds twice: once (successfully) because of the csproj with the explicit SetPlatform metadata, and once more (failure) because the dirs.proj didn't supply any Platform global property.

This is an interesting scenario. The dirs.proj build of the vcxproj fails to build because there's no project config set up for Debug|Win32, which it happens to default to. The responsibility of defaulting to something like x64 (because of Platforms) would lie in the traversal SDK. I now see what you meant. I filed an issue over in the traversal SDK for this: microsoft/MSBuildSdks#380 please add/fix my issue description if it doesn't quite match what you're asking for

As for the second scenario:

Actual: The multi-plat csproj is built twice as AnyCPU both times, leading to timing breaks. The AnyCPU csproj also builds once (which is correct).

I think this is no longer the case. Testing it out today, the anycpulib detects that the multiplat csproj would have built as AnyCPU without passing global properties, so it doesn't explicitly pass Platform=AnyCPU anymore. I also see one less evaluation as a result. I have both binlogs saved, message me if you'd like to see them.

Have you tested that scenario with an MSBuild that has https://github.com/dotnet/msbuild/pull/7511/files#diff-5407d46dd30ce4031e530c35cc2e0a62a6c96e54cb1def14fb316f351ef92de9 merged?

[AArnott]

Have you tested that scenario with an MSBuild that has...

dotnet build repros the problem with the 6.0.400 SDK. That evidently carries msbuild 17.3.0+92e077650
msbuild.exe exhibits the desired behavior in 17.4.0-preview-22451-06+2db11c256

What version of the .NET SDK does or will carry the msbuild that contains the fix?

[benvillalobos]

dotnet build repros the problem with the 6.0.400 SDK. That evidently carries msbuild 17.3.0+92e077650

92e0776 should have that change. Does the issue persist into the 7.0 SDK's? Is it possible for you to use the 7.0 SDK's?

[AArnott]

The 7.0.100-rc.1.22431.12 .NET SDK works. But 6.0.401 (which carries msbuild 17.3.1+2badb37d1) does not.

Using the 7.0 SDK is only an option for me once it ships as a stable version.

[benvillalobos]

@AArnott I just tested this with 6.0.402 and I see only one build of the multiplatcsproj, and one reduced evaluation (compared to 6.0.300 from globaljson). I think your global json may have been pinning you to to a previous version:

    "version": "6.0.300",
    "rollForward": "patch",
    "allowPrerelease": false

[AArnott]

Looks good at this point. Thank you.