Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overbuild and timing breaks when multi-targeting csproj has a P2P to a vcxproj #5830

Closed
AArnott opened this issue Oct 23, 2020 · 1 comment · Fixed by #5838
Closed

Overbuild and timing breaks when multi-targeting csproj has a P2P to a vcxproj #5830

AArnott opened this issue Oct 23, 2020 · 1 comment · Fixed by #5838
Assignees

Comments

@AArnott
Copy link
Contributor

AArnott commented Oct 23, 2020

Issue Description

A multi-targeting SDK style csproj project that includes a ProjectReference to a vcxproj ends up building that vcxproj twice, and potentially in parallel causing timing breaks.

Steps to Reproduce

msbuild_tfm_p2p.sln.zip

  1. Create a new VS solution with a .NET SDK csproj.
  2. Add a new vcxproj project to the solution.
  3. Modify the csproj to multitarget:
     <TargetFrameworks>netcoreapp3.1;net472</TargetFrameworks>
  4. Add a project reference from csproj to vcxproj

Build the csproj from the command line with the /bl switch.

msbuild some.csproj /bl

Expected Behavior

The vcxproj only builds once

Actual Behavior

The vcxproj builds twice, as observable with the msbuild.binlog file.
Sometimes the build can break when these builds are in parallel.

Analysis

It builds twice because the C# ResolveProjectReferences target does not remove the TargetFramework global property that the vcxproj otherwise inherits from the csproj.

image

If the ProjectReference to vcxproj would have had GlobalPropertiesToRemove metadata that included TargetFramework, I think the problem would be solved:

image

Attach a binlog

binlog

@AArnott AArnott added bug needs-triage Have yet to determine what bucket this goes in. labels Oct 23, 2020
@benvillalobos benvillalobos added needs-triage Have yet to determine what bucket this goes in. and removed needs-triage Have yet to determine what bucket this goes in. labels Oct 28, 2020
@marcpopMSFT marcpopMSFT linked a pull request Oct 28, 2020 that will close this issue
Forgind added a commit to Forgind/msbuild that referenced this issue Nov 5, 2020
Fixes dotnet#5830. See explanation there.
@benvillalobos benvillalobos removed the needs-triage Have yet to determine what bucket this goes in. label Nov 11, 2020
@Forgind
Copy link
Member

Forgind commented Nov 11, 2020

Here's what we think is happening here:

vcxproj files compile down to OS-specific binaries, either native or .NET. They don't currently permit multitargeting, so you must specify exactly one TargetFramework. In the _GetProjectReferenceTargetFrameworkProperties target of Microsoft.Common.CurrentVersion.targets, SkipTargetFrameworkProperties is set to true for vcxproj to account for that.

This means we do not fill the Item _ProjectReferenceTargetFrameworkPossibilities or, by extension, the AnnotatedProjects Item.

For single-targeted projects, we normally decorate the AnnotatedProjects Item with UndefineProperties metadata specifying that TargetFramework should be undefined. Because it isn't defined properly at that stage, however, this does not happen, and TargetFramework is defined at this point in addition to having been defined globally. Currently, this is always true for vcxproj.

MSBuild permits building the same project twice as long as it has different sets of global properties. Because the TargetFramework global property is not being removed as expected by the multitargeting part of MSBuild, the engine recognizes that there are differences and builds it twice. This can become more noticeable if the projects build in parallel, since they could try to access the same resources and conflict, failing the build. Note, however, that building the same project twice in this way is always wrong even if it seems minor because they do not conflict, and the second build is relatively fast.

Forgind added a commit that referenced this issue Nov 13, 2020
* Undefine TargetFramework if skipping TargetFramework

Fixes #5830. See explanation there.
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants