Report that a project only has one TFM #797

rainersigwald · 2017-02-02T23:28:59Z

Partially fixes #739.

The first iteration of cross-targeting support code unconditionally
queried each ProjectReference for the best TFM to build against, and
then explicitly specified that TFM when building the reference. This
caused a race condition when building a set of projects that had a
single-TFM project and another project that had a reference to it. The
entry point (probably .sln) build would build the referenced project
with no TF specified, and then the referencing project would build it
with an explicit TF specified. These two builds appeared different to
the MSBuild engine (because they had different sets of global
properties) and so were both executed, resulting in a race condition.

The fix is in two parts:

Allow a project to report back through GetTargetFrameworkProperties
that it only has one TF to build.
When a project reports that it has only one TF to build, issue its
build request without specifying any TF. (Respect projects that say they only have one TFM msbuild#1667)

This commit is the former.

nguerrera · 2017-02-02T23:45:51Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.Common.targets

@@ -55,13 +55,16 @@ Copyright (c) .NET Foundation. All rights reserved.
    with the referencing project's target framework.
  ============================================================
   -->
-  <Target Name="GetTargetFrameworkProperties" Returns="TargetFramework=$(NearestTargetFramework)">
+  <Target Name="GetTargetFrameworkProperties" Returns="TargetFramework=$(NearestTargetFramework);ProjectHasSingleTargetFramework=$(_HasSingleTargetFramework)">


Can we use metadata to indicate ProjectHasSingleTargetFramework? Seems like it would simplify the consuming side.

Indeed it would, but it requires returning an item rather than a property which seemed more invasive.

nguerrera · 2017-02-02T23:46:30Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.Common.targets


    <PropertyGroup>
      <!-- If a ReferringTargetFramework was not specified, and we only have one TargetFramework, then don't try to check compatibility -->
      <_SkipNearestTargetFrameworkResolution Condition="'$(TargetFramework)' != '' and '$(ReferringTargetFramework)' == ''">true</_SkipNearestTargetFrameworkResolution>
      <NearestTargetFramework Condition="'$(_SkipNearestTargetFrameworkResolution)' == 'true'">$(TargetFramework)</NearestTargetFramework>

+      <_HasSingleTargetFramework Condition="'$(TargetFramework)' != '' and '$(TargetFrameworks)' == ''">true</_HasSingleTargetFramework>
+      <_HasSingleTargetFramework Condition="'$(_HasSingleTargetFramework)' == ''">false</_HasSingleTargetFramework>
+


I believe everywhere else we consider TargetFramework to win over TargetFrameworks, so this might be better as TargetFramework != ''

I don't think I understand. Can you rephrase?

If I have a project that sets both TargetFramework and TargetFrameworks, TargetFramework will win (we won't go to cross-targeting targets), but this won't report it as such.

rainersigwald · 2017-02-03T00:21:34Z

Alternative with an item + metadata instead:

diff --git "a/c:\\Program Files\\dotnet - Copy\\sdk\\1.0.0-rc4-004689/Microsoft.Common.CurrentVersion.targets" "b/C:\\Program Files\\dotnet\\sdk\\1.0.0-rc4-004689/Microsoft.Common.CurrentVersion.targets"
index da772b0..96e8fb4 100644
--- "a/c:\\Program Files\\dotnet - Copy\\sdk\\1.0.0-rc4-004689/Microsoft.Common.CurrentVersion.targets"	
+++ "b/C:\\Program Files\\dotnet\\sdk\\1.0.0-rc4-004689/Microsoft.Common.CurrentVersion.targets"	
@@ -1524,13 +1524,20 @@ Copyright (C) Microsoft Corporation. All rights reserved.
         RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove);TargetFramework"
         Condition="'%(_MSBuildProjectReferenceExistent.Extension)' != '.vcxproj'">
 
-      <Output TaskParameter="TargetOutputs" PropertyName="_ProjectReferenceTargetFrameworkProperties" />
+      <Output TaskParameter="TargetOutputs" ItemName="_ProjectReferenceTargetFrameworkProperties" />
     </MSBuild>
 
+    <Error Condition="@(_ProjectReferenceTargetFrameworkProperties->Count()) != 1" Text="Assertion failed: @(_ProjectReferenceTargetFrameworkProperties->Count()) @(_ProjectReferenceTargetFrameworkProperties)" />
+
     <ItemGroup>
       <_MSBuildProjectReferenceExistent Condition="'%(_MSBuildProjectReferenceExistent.Identity)' == '%(Identity)'">
         <SetTargetFramework>$(_ProjectReferenceTargetFrameworkProperties)</SetTargetFramework>
+
+        <UndefineProperties Condition="'@(_ProjectReferenceTargetFrameworkProperties->'%(HasSingleTargetFramework)')' == 'true'">%(_MSBuildProjectReferenceExistent.UndefineProperties);TargetFramework</UndefineProperties>
       </_MSBuildProjectReferenceExistent>
+
+      <!-- Clear the list so the next iteration through the loop has a single item again. -->
+      <_ProjectReferenceTargetFrameworkProperties Remove="@(_ProjectReferenceTargetFrameworkProperties)" />
     </ItemGroup>
 
     <PropertyGroup>
diff --git "a/c:\\Program Files\\dotnet - Copy\\sdk\\1.0.0-rc4-004689/Sdks/Microsoft.NET.Sdk/build/Microsoft.NET.Sdk.Common.targets" "b/C:\\Program Files\\dotnet\\sdk\\1.0.0-rc4-004689/Sdks/Microsoft.NET.Sdk/build/Microsoft.NET.Sdk.Common.targets"
index c1c71c2..ae43aa0 100644
--- "a/c:\\Program Files\\dotnet - Copy\\sdk\\1.0.0-rc4-004689/Sdks/Microsoft.NET.Sdk/build/Microsoft.NET.Sdk.Common.targets"	
+++ "b/C:\\Program Files\\dotnet\\sdk\\1.0.0-rc4-004689/Sdks/Microsoft.NET.Sdk/build/Microsoft.NET.Sdk.Common.targets"	
@@ -55,7 +55,7 @@ Copyright (c) .NET Foundation. All rights reserved.
     with the referencing project's target framework.
   ============================================================
    -->
-  <Target Name="GetTargetFrameworkProperties" Returns="TargetFramework=$(NearestTargetFramework)">
+  <Target Name="GetTargetFrameworkProperties" Returns="@(ProjectWithTargetFrameworkProperties)">
 
     <PropertyGroup>
       <!-- If a ReferringTargetFramework was not specified, and we only have one TargetFramework, then don't try to check compatibility -->
@@ -72,6 +72,14 @@ Copyright (c) .NET Foundation. All rights reserved.
                                Condition="'$(_SkipNearestTargetFrameworkResolution)' != 'true'">
       <Output PropertyName="NearestTargetFramework" TaskParameter="NearestTargetFramework" />
     </GetNearestTargetFramework>
+
+    <ItemGroup>
+      <ProjectWithTargetFrameworkProperties Include="$(MSBuildProjectFullPath)">
+        <TargetFramework>$(NearestTargetFramework)</TargetFramework>
+        <HasSingleTargetFramework Condition="'$(TargetFrameworks)' != ''">false</HasSingleTargetFramework>
+        <HasSingleTargetFramework Condition="'$(TargetFramework)' != '' and '$(TargetFrameworks)' == ''">true</HasSingleTargetFramework>
+      </ProjectWithTargetFrameworkProperties>
+    </ItemGroup>
   </Target>
   
 </Project>

I don't think I prefer it. Have to jump through hoops to operate on a single result at a time, where that's implicit with the property. Not that the property way in the PR is clean or nice . . .

nguerrera · 2017-02-03T00:27:58Z

Hmm that is a lot messier than I imagined. I trust your judgement on this.

srivatsn · 2017-02-03T13:50:32Z

@natidea can you test this change with whatever repros we've got?

srivatsn · 2017-02-03T13:51:26Z

You'll need this change as well - dotnet/msbuild#1667

srivatsn · 2017-02-03T14:07:08Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.Common.targets


    <PropertyGroup>
      <!-- If a ReferringTargetFramework was not specified, and we only have one TargetFramework, then don't try to check compatibility -->
      <_SkipNearestTargetFrameworkResolution Condition="'$(TargetFramework)' != '' and '$(ReferringTargetFramework)' == ''">true</_SkipNearestTargetFrameworkResolution>
      <NearestTargetFramework Condition="'$(_SkipNearestTargetFrameworkResolution)' == 'true'">$(TargetFramework)</NearestTargetFramework>

+      <_HasSingleTargetFramework Condition="'$(TargetFramework)' != ''">true</_HasSingleTargetFramework>


If a SDK-based project doesn't define a TargetFramework, do we issue an error today? I'm thinking if someone takes an existing project where TFI, TFV are set and makes it use the SDK and doesn't set TargetFramework, will we now think of them as not-single-targeting projects?

Good point. I think we need this to be '$(IsCrossTargetingBuild)' != 'true'. Whatever condition causes us not to use the x-targeting is the same as what should get no additional properties.

Makes perfect sense. Thanks.

rainersigwald · 2017-02-03T16:19:55Z

@natidea MSBuild package 545 should have the common-targets side of this baked in.

nguerrera · 2017-02-03T16:24:02Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.Common.targets

@@ -29,7 +29,7 @@ Copyright (c) .NET Foundation. All rights reserved.
    <Version Condition=" '$(Version)' == '' ">$(VersionPrefix)</Version>
    <Authors Condition=" '$(Authors)'=='' ">$(AssemblyName)</Authors>
    <Company Condition=" '$(Company)'=='' ">$(Authors)</Company>
-    <AssemblyTitle Condition="'$(AssemblyTitle)' == ''">$(AssemblyName)</AssemblyTitle>
+-    <AssemblyTitle Condition="'$(AssemblyTitle)' == ''">$(AssemblyName)</AssemblyTitle>


Looks like there are some whitespace and extra leading character issues

That's what I get for trusting the GitHub web editor.

nguerrera · 2017-02-03T21:23:29Z

Customer scenario: Parallel solution builds with single-targeted sdk-based projects fail intermittently (but frequently)

Bugs this fixes:: #739

Workarounds, if any: None

Risk: Low

Performance impact: Low

Is this a regression from a previous update?: No

Root cause analysis:: Complicated interaction between cross-targeting P2P protocol, sln builds, and /m > 1.

How was the bug found?: Customer and partner reported

nguerrera · 2017-02-03T21:23:50Z

@MattGertz for approval

MattGertz · 2017-02-03T21:25:40Z

@srivatsn Feel free to add this to your VSO bug now if you are comfortable. It'll be another 40 minutes before they get to us...

srivatsn · 2017-02-03T21:35:53Z

Added

Partially fixes dotnet#739. The first iteration of cross-targeting support code unconditionally queried each ProjectReference for the best TFM to build against, and then explicitly specified that TFM when building the reference. This caused a race condition when building a set of projects that had a single-TFM project and another project that had a reference to it. The entry point (probably .sln) build would build the referenced project with no TF specified, and then the referencing project would build it with an explicit TF specified. These two builds appeared different to the MSBuild engine (because they had different sets of global properties) and so were both executed, resulting in a race condition. The fix is in two parts: * Allow a project to report back through GetTargetFrameworkProperties that it only has one TF to build. * When a project reports that it has only one TF to build, issue its build request without specifying any TF (dotnet/msbuild#1667) This commit is the former.

natidea · 2017-02-03T22:47:23Z

Shiproom Update: Waiting for Weekend validation

natidea

After applying both patches, I no longer see the race through multiple builds of both CLI build and DependencyInjection.sln

…0190720.4 (dotnet#797) - Microsoft.AspNetCore.Mvc.Analyzers - 3.0.0-preview8.19370.4 - Microsoft.AspNetCore.Mvc.Api.Analyzers - 3.0.0-preview8.19370.4 - Microsoft.AspNetCore.Analyzers - 3.0.0-preview8.19370.4 - Microsoft.AspNetCore.Components.Analyzers - 3.0.0-preview8.19370.4

rainersigwald requested a review from nguerrera February 2, 2017 23:28

dnfclas added the cla-already-signed label Feb 2, 2017

rainersigwald mentioned this pull request Feb 2, 2017

Respect projects that say they only have one TFM dotnet/msbuild#1667

Merged

rainersigwald changed the title ~~Respect projects that say they only have one TFM~~ Report that a project only has one TFM Feb 2, 2017

nguerrera reviewed Feb 2, 2017

View reviewed changes

srivatsn reviewed Feb 3, 2017

View reviewed changes

nguerrera reviewed Feb 3, 2017

View reviewed changes

nguerrera approved these changes Feb 3, 2017

View reviewed changes

MattGertz added the Pending Shiproom Approval label Feb 3, 2017

rainersigwald force-pushed the sln-build-race-fix branch from 4c6c522 to ecc9b6c Compare February 3, 2017 22:32

natidea approved these changes Feb 4, 2017

View reviewed changes

MattGertz added the Approved to merge label Feb 4, 2017

srivatsn merged commit 0d73192 into dotnet:master Feb 4, 2017

rainersigwald deleted the sln-build-race-fix branch February 13, 2017 20:18

nguerrera mentioned this pull request Sep 19, 2017

Multi-proc build of a .sln file is 'The process cannot access the file because it is being used by another process.' #739

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report that a project only has one TFM #797

Report that a project only has one TFM #797

rainersigwald commented Feb 2, 2017 •

edited

Loading

nguerrera Feb 2, 2017

rainersigwald Feb 2, 2017

nguerrera Feb 2, 2017

rainersigwald Feb 2, 2017

nguerrera Feb 3, 2017

rainersigwald commented Feb 3, 2017

nguerrera commented Feb 3, 2017

srivatsn commented Feb 3, 2017

srivatsn commented Feb 3, 2017

srivatsn Feb 3, 2017

nguerrera Feb 3, 2017

rainersigwald Feb 3, 2017

rainersigwald commented Feb 3, 2017

nguerrera Feb 3, 2017

rainersigwald Feb 3, 2017

rainersigwald Feb 3, 2017

nguerrera commented Feb 3, 2017

nguerrera commented Feb 3, 2017

MattGertz commented Feb 3, 2017

srivatsn commented Feb 3, 2017

natidea commented Feb 3, 2017

natidea left a comment

Report that a project only has one TFM #797

Report that a project only has one TFM #797

Conversation

rainersigwald commented Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald commented Feb 3, 2017

nguerrera commented Feb 3, 2017

srivatsn commented Feb 3, 2017

srivatsn commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera commented Feb 3, 2017

nguerrera commented Feb 3, 2017

MattGertz commented Feb 3, 2017

srivatsn commented Feb 3, 2017

natidea commented Feb 3, 2017

natidea left a comment

Choose a reason for hiding this comment

rainersigwald commented Feb 2, 2017 •

edited

Loading