dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.21k stars 1.35k forks source link

Building vcxproj with P2P to multi-targeting csproj fails in Visual Studio build or CLI with BuildProjectReferences=false #7518

Open AArnott opened 2 years ago

AArnott commented 2 years ago

Issue Description

vcxproj fails to set the TargetFramework global property on a P2P to a multi-targeting csproj when BuildProjectReferences is set to false, which VS does by default.

Steps to Reproduce

Repro solution: MTvcTest.zip

  1. Create a VS solution
  2. Add a csproj (SDK style) that targets net6.0 and net472
  3. Add a vcxproj
  4. Add a P2P from the vcxproj to the csproj
  5. Build

Expected Behavior

The build succeeds in VS and msbuild.exe

Actual Behavior

The build succeeds at the command line and fails in VS. It also fails at the CLI when /p:buildprojectreferences=false is specified. See binlog.

"C:\temp\MTvcTest\VCApp\VCApp.vcxproj" (default target) (1) -> "C:\temp\MTvcTest\CSharpLib\CSharpLib.csproj" (GetTargetPath target) (2:4) -> C:\temp\MTvcTest\CSharpLib\CSharpLib.csproj : error MSB4057: The target "GetTargetPath" does not exist in the project.

Analysis

The TargetFramework global property isn't set when invoking the P2P.

Versions & Configurations

Microsoft (R) Build Engine version 17.2.0-preview-22178-04+ce7523296 for .NET Framework

benvillalobos commented 2 years ago

The chain of events for SetTargetFramework not getting set:

  1. Microsoft.CppBuild.targets intentionally clears out TargetFrameworkMoniker (not super relevant for the provided binlog)

    image
  2. Common.CurrentVersion.Targets tries to set ReferringTargetFrameworkForProjectReferences to TargetFrameworkMoniker, but it's empty. https://github.com/dotnet/msbuild/blob/18fc0b3dd9dfd7cbb83272f13de4a89e8aa30531/src/Tasks/Microsoft.Common.CurrentVersion.targets#L1705-L1708

  3. The GetReferenceNearestTargetFrameworkTask task doesn't run because it's conditioned on ReferringTargetFrameworkForProjectReferences having a value.

https://github.com/dotnet/msbuild/blob/18fc0b3dd9dfd7cbb83272f13de4a89e8aa30531/src/Tasks/Microsoft.Common.CurrentVersion.targets#L1811

What's inherently confusing here is setting BuildProjectReferences to false, but expecting P2P's to build?

The docs say setting it to false would prevent up-to-date checks, but does that mean they should still build?

benvillalobos commented 2 years ago

Team Triage: The error makes sense considering a multi-targeted app doesn't have a single defined output like a single-targeted project does. We agree that there should be a way around this though, as a vcxproj targeting a multi-targeted app is a reasonable scenario. Marking as needs-design.

One potential path would be to add a stub GetTargetPath to avoid the error, but we want to preserve some sort of error for customers who disable TF negotiation and ProjRef a multi-targeted project.

AArnott commented 2 years ago

Thanks for investigating, @BenVillalobos.

This isn't a multi-targeted app though. The app is a vcxproj (since that's the top-level project). It references a multi-targeted library, which is certainly expected to work. That library may be used by other projects in the solution, such that multi-targeting makes sense. The vcxproj should pick the best TargetFramework from that library based on the vcxproj's own TargetFramework.

As for expecting its referenced projects to build, yes. BuildProjectReferences=false is used in VS, and VS's solution build manager guarantees that projects all build in dependency order. BuildProjectReferences is there to prevent double-builds, or otherwise to make one project's build fast by assuming its dependencies have already been built. It's acceptable in such a case to error out if the required dependencies are not actually built yet (if you need to read the file from disk that the dependency should have produced already). But choosing a best TargetFramework with which to invoke the csproj doesn't require the dll to be on disk anyway. csproj to csproj has no problem with this. Why does vcxproj?

benvillalobos commented 2 years ago

The vcxproj should pick the best TargetFramework from that library based on the vcxproj's own TargetFramework.

Do you actually want this .vcxproj to use the outputs of the library?

My 2c: It sounds like the root of your problem is that you want your .vcxproj to act as a pseudo dirs.proj and doesn't necessarily care about using the outputs of the csproj. If that's the case, the real solution here is to define a stub target for GetTargetPath in this scenario to avoid a failing build.

If you do want it to use the outputs, we need to design up a way for vcxproj's to pick a default, or define a way for the vcxproj to specify.

AArnott commented 2 years ago

Do you actually want this .vcxproj to use the outputs of the library?

In my repro it does. It's perfectly legit for a vcxproj to reference an assembly, and this should Just Work as far as TFM negotiation. The really unexpected bit is that it does work, unless BuildProjectReferences=false, which really suggests to me that something is broken here.