Msbuild: Audit ***DependsOn properties to not overwrite previous value

[KirillOsenkov]

See for example PrepareResourceNamesDependsOn:
http://source.roslyn.io/#MSBuildFiles/C/ProgramFiles(x86)/MSBuild/14.0/bin_/amd64/Microsoft.Common.CurrentVersion.targets,2473

<PrepareResourceNamesDependsOn>
      AssignTargetPaths;
      SplitResourcesByCulture;
      CreateManifestResourceNames;
      CreateCustomManifestResourceNames
</PrepareResourceNamesDependsOn>

Suppose you have a .targets file that wants to insert its targets like this:

<PrepareResourceNamesDependsOn>
      GenerateRuleSourceFromXaml;
      $(PrepareResourceNamesDependsOn)
</PrepareResourceNamesDependsOn>

If you happen to include your .targets file after Microsoft.Common.targets, then it's going to work as expected: PrepareResourceNames will trigger your target (GenerateRuleSourceFromXaml in this case).

However if you accidentally include your .targets file before Microsoft.Common.targets, your value for the PrepareResourceNamesDependsOn will be silently overwritten with the default value, and your target will run at an arbitrary later time, failing mysteriously.

This is unfortunately probably one of the biggest fundamental problems of MSBuild, with its global mutable state and multiple passes where order of things influences multiple assignments and other things. The fact that a property can be assigned multiple times in different orders and the last one wins, while all previous silently lose, is a general problem that I don't know how to alleviate.

But in this particular case my suggestion is to audit all Microsoft targets and change the *DependsOn properties to include its previous value, so that at least in this case the order of including the .targets file doesn't matter.

[KirillOsenkov]

Or maybe all these properties should really be items, so that adding an item is more declarative and not so imperative. However that ship has sailed (you need to keep existing properties for backwards compatibility). However maybe introducing another extensibility point which is items may be interesting to consider.

[anttirt]

This issue currently makes it impossible to use custom pre-compile targets with the new dotnet tools preview.

In particular, with 1.0.0-preview4-004233, running dotnet new -t lib creates the following .csproj:

<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0">

  <PropertyGroup>
    <TargetFramework>netstandard1.4</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="**\*.cs" />
    <EmbeddedResource Include="**\*.resx" />
  </ItemGroup>

  <ItemGroup>
    <PackageReference Include="NETStandard.Library" Version="1.6" />
  </ItemGroup>

</Project>

Trying to add targets to CoreCompileDependsOn as below:

  <PropertyGroup>
    <CoreCompileDependsOn>
      MyPreCompileTarget;
      $(CoreCompileDependsOn)
    </CoreCompileDependsOn>
  </PropertyGroup>

This fails because when building with dotnet build, the file Microsoft.Common.CurrentVersion.targets is automatically imported after the contents of the csproj, and https://github.com/Microsoft/msbuild/blob/9fb136160ab2f828e2350227dc06ac8cf1d1695d/src/XMakeTasks/Microsoft.CSharp.CurrentVersion.targets#L165 overwrites the definition.

I'm currently working around the issue by manually patching the file:

diff --git a/Microsoft.CSharp.CurrentVersion.targets b/Microsoft.CSharp.CurrentVersion.targets
index 705954a..f5f5a16 100644
--- a/Microsoft.CSharp.CurrentVersion.targets
+++ b/Microsoft.CSharp.CurrentVersion.targets
@@ -162,7 +162,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
     </ItemGroup>

     <PropertyGroup>
-        <CoreCompileDependsOn>_ComputeNonExistentFileProperty;ResolveCodeAnalysisRuleSet</CoreCompileDependsOn>
+        <CoreCompileDependsOn>_ComputeNonExistentFileProperty;ResolveCodeAnalysisRuleSet;$(CoreCompileDependsOn)</CoreCompileDependsOn>
         <ExportWinMDFile Condition="'$(ExportWinMDFile)' == '' and '$(OutputType)' == 'WinMDObj'">true</ExportWinMDFile>
     </PropertyGroup>

But it would be super cool to get this into the next preview release! I'm taking a look at migrating some of our projects to .NET Core and this is one of the biggest blocking issues right now.

[jeffkl]

The design we have settled on is allowing users to remove the Sdk attribute from the project and specify the location of the import instead. The project-level Sdk attribute implicitly adds imports at the top and bottom which does not allow users to control them. In the next preview, you'll be able to have a project that looks like this:

<Project ToolsVersion="15.0">

  <Import Project="Sdk\Sdk.props" Sdk="Microsoft.NET.Sdk" />

  <PropertyGroup>
    <TargetFramework>netstandard1.4</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="**\*.cs" />
    <EmbeddedResource Include="**\*.resx" />
  </ItemGroup>

  <ItemGroup>
    <PackageReference Include="NETStandard.Library" Version="1.6" />
  </ItemGroup>

  <Import Project="Sdk\Sdk.targets" Sdk="Microsoft.NET.Sdk" />

</Project>

This would allow you to define ItemGroups and PropertyGroups before/after any of the imports. The removal of the project-level Sdk attribute and addition of the <Import /> will be a manual process. We think a majority of users will stick with the default and only power users will want full control.

Does this design sound good?

[dsplaisted]

The design would allow someone to work around the issue here, but it would be better to also fix the issue with our definition of CoreCompileDependsOn and any other similar properties.

[KirillOsenkov]

@jeffkl this is good, but it feels orthogonal to the problem I described above. Keep in mind that the problem I faced was with the old projects where I had full control over the order of imports. Precisely that is the issue here: by having *DependsOn not recursively mention itself we depend on the order of inclusion.

If all *DependsOn properties are augmented to mention themselves we can lessen the dependency on the order of imports.

Of course at least having the ability to control import order with SDKs is a welcome change.

[jeffkl]

Okay sorry I misunderstood. I agree, all *DependsOn should be additive and not clear the list.

[kzu]

CoreCompileDependsOn

This one in particular is quite important.

Also, WRT to the ship having sailed to fix this, I think the following potentially trivial change on the common targets would completely solve the issue moving forward:

<CoreCompile DependsOnTargets="$(CoreCompileDependsOn);@(CoreCompileDependsOn)" ...>
  ...
</CoreCompile>

Maybe all targets with a *DependsOn should also append @(*DependsOn) by convention?

[StephaneDelcroix]

I second this.

We append and prepend some custom tasks on CoreCompileDependsOn and CompileDependsOn. Doesn't work on net standard projects

[StephaneDelcroix]

The workaround for me is to use BeforeTargets="BeforeCompile" and AfterTargets="AfterCompile"

[KirillOsenkov]

I ran into this again:
https://github.com/Microsoft/msbuild/blob/50639058f947ef3b21075cb110f06913124d2942/src/Tasks/Microsoft.CSharp.CurrentVersion.targets#L165

This was overwriting whatever value CoreCompileDependsOn was set to previously in the evaluation process.