Sdk: PackageDefinition metadata seems to be missing

[benPearce1]

Before upgrading to DotNet Core 2.1.3, the following minimal example worked as expected:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="FSharp.Compiler.Tools" Version="4.0.0.1"/>
  </ItemGroup>
 <Target Name="GetToolFiles">
    <CreateItem Include="@(PackageDefinitions)" Condition="'%(Name)' == 'FSharp.Compiler.Tools'">
      <Output TaskParameter="Include" ItemName="FSharpCompilerToolsRef"/>
    </CreateItem>
    <PropertyGroup>
      <FSharpCompilerTools>@(FSharpCompilerToolsRef->'%(ResolvedPath)')/tools/*.*</FSharpCompilerTools>
    </PropertyGroup>
    <ItemGroup>
      <FSharpFiles Include="$(FSharpCompilerTools)"/>
    </ItemGroup>
  </Target>
  <Target Name="CopyToolsAfterBuild" AfterTargets="Build" DependsOnTargets="GetToolFiles">
    <Copy SourceFiles="@(FSharpFiles)" DestinationFolder="$(OutDir)/FSharp/"/>
  </Target>
</Project>

The expected output would be the contents of the Tools directory under the FSharp.Compiler.Tools would be copied to Debug/netcoreapp2.0/FSharp.
Since upgrading to 2.1.300, the %(ResolvedPath) metadata attribute is not being populated with a value, and the result would copy files located in a tools folder off the root of the current drive if one exists.
The following lines are written to a detailed output:
Copying file from "/tools\file1.txt" to "bin\Debug\netcoreapp2.0\/FSharp/file1.txt".

Modifiying the SDK version using either global.json or uninstalling the most recent versions of DotnetCore (2.1.300), results in the expected files being copied from the source package reference.
Copying file from "C:\Users\username\.nuget\packages\fsharp.compiler.tools\4.0.0.1\tools\fsc.exe" to "bin\Debug\netcoreapp2.0\\FSharp\fsc.exe".

I have tested this issue using SDK version 2.1.300 and 2.1.400 and get the same missing metadata, any earlier version seems set the ResolvedPath value to the correct value for the PackageReference

[livarcocc]

@nguerrera to take a look.

[nguerrera]

Calculating those PackageDefinitions is very expensive so we made a modest breaking change in exchange for vastly improved build performance to stop doing it in every build since it is usually not required. You can request them explicitly by adding a dependency on RunResolvePackageDependencies.

-<Target Name="GetToolFiles">
+<Target Name="GetToolFiles" DependsOnTargets="RunResolvePackageDependencies">

[livarcocc]

Seems like @nguerrera 's workaround addresses the issue. This was something we knew would potentially break some people and I believe the change is fantastic given the perf gains we had.

@nguerrera should we be good to close this issue then?

[matt-richardson]

This is not a great solution. Can we at least fail the build or something? Silently skipping this step, with no warning and breaking builds is not good.

[nguerrera]

Unfortunately, we cannot fail the build because there is nothing in the way MSBuild works to allow hooking onto an attempt to read these items. MSBuild is a dynamic language with the semantic that accessing arbitrary items that haven't been populated just returns you an empty set. Our only other choice would have been to give up on the change, but the perf benefits of this change were too huge for that.

[benPearce1]

This solution also has some issues when the project is targeting multiple frameworks (core and full) as you get different behavior for each framework.
The RunResolvePackageDependencies doesn't seem to be available for full framework.

[nguerrera]

@benPearce1 That's unexpected. Please file a separate bug on that with repro steps. The behavior should be exactly the same as before once you add the dependency. The code that produces PackageDefinitions is exactly the same RunResolvePackageDependencies target as before. The only difference is that the build no longer depends on it.

[droyad]

@nguerrera Should this be added to the .Net Core release notes?

[nguerrera]

@droyad, yes that is a good idea. @livarcocc What is the process for updating those?

[livarcocc]

@leecow for release notes update.

[leecow]

Submit a PR against https://github.com/dotnet/core/blob/master/release-notes/2.1/2.1.1-known-issues.md with the content you want added.

[SimonCropp]

so we made a modest breaking change in exchange for vastly improved build performance

just to be clear. this was a breaking change deployed in a patch version with no mention in the release notes?

[nguerrera]

I've submitted https://github.com/dotnet/core/pull/1964 to update the release notes

The SDK version that changed this is not considered a "patch". See https://docs.microsoft.com/en-us/dotnet/core/versions/

[SimonCropp]

given the proposed work around

<Target Name="GetToolFiles" DependsOnTargets="RunResolvePackageDependencies">

will cause The target "RunResolvePackageDependencies" does not exist in the project in the old csproj format.

So in the context of a extension being consumed by other projects (both old and new csproj formats), how would you propose doing a "DependsOnTargets if exists"?

[matt-richardson]

I found that if you add:

  <Target Name="RunResolvePackageDependencies" Condition="'$(RunResolvePackageDependencies)' != ''"/>

It will define a new empty target if it doesn't already exist, and then it'll work for both new code and old.