Use correct target path for runtimeTargets items when resolving conflicts

[dsplaisted]

Fixes #1510

EDIT: Also fixes #1465 now

@nguerrera @ericstj @eerhardt for review

[dsplaisted]

@dotnet-bot test Windows_NT_FullFramework Debug

[nguerrera]

We should fix related #1465 at the same time. We set "Destination Sub Path" too late there. We should be setting it when resolving "all copy local items".

(Aside: it's unfortunate that we use an item list called "all copy local items" for purposes other than copying things locally now.)

I think we can get the correct sub path from the joined file definition locale rather than the path convention we use now.

[ericstj]

Don't we need some sort of condition on this for frameworks that don't understand RuntimeTarget items, or did you have you decided that you're adding the RuntimeTarget concept to desktop?

[dsplaisted]

This just tries to match the path that will come out of PublishAssembliesResolver here when doing conflict resolution. If you have a package that assumes that native assets under runtimes/ will be loaded when used in an AnyCPU .NET Framework app, then those assets will be placed in the runtimes/ subfolders when the app is published and will fail to load at runtime. This doesn't change that.

If you publish for a specific RID, then the items will be runtime items instead of runtimeTargets items and will get copied directly to your output folder, and conflict resolution will use the right path. For example, under the .NETCoreApp,Version=v2.0 target in an assets file:

"System.Diagnostics.TraceSource/4.0.0": {
  "type": "package",
  "dependencies": {
    "Microsoft.NETCore.Platforms": "1.0.1",
    "System.Collections": "4.0.11",
    "System.Diagnostics.Debug": "4.0.11",
    "System.Globalization": "4.0.11",
    "System.Resources.ResourceManager": "4.0.1",
    "System.Runtime": "4.1.0",
    "System.Runtime.Extensions": "4.1.0",
    "System.Threading": "4.0.11",
    "runtime.native.System": "4.0.0"
  },
  "compile": {
    "ref/netstandard1.3/System.Diagnostics.TraceSource.dll": {}
  },
  "runtimeTargets": {
    "runtimes/unix/lib/netstandard1.3/System.Diagnostics.TraceSource.dll": {
      "assetType": "runtime",
      "rid": "unix"
    },
    "runtimes/win/lib/netstandard1.3/System.Diagnostics.TraceSource.dll": {
      "assetType": "runtime",
      "rid": "win"
    }
  }
},

While for the .NETCoreApp,Version=v2.0/win10-x64 target, it looks like this:

"System.Diagnostics.TraceSource/4.0.0": {
  "type": "package",
  "dependencies": {
    "Microsoft.NETCore.Platforms": "1.0.1",
    "System.Collections": "4.0.11",
    "System.Diagnostics.Debug": "4.0.11",
    "System.Globalization": "4.0.11",
    "System.Resources.ResourceManager": "4.0.1",
    "System.Runtime": "4.1.0",
    "System.Runtime.Extensions": "4.1.0",
    "System.Threading": "4.0.11",
    "runtime.native.System": "4.0.0"
  },
  "compile": {
    "ref/netstandard1.3/System.Diagnostics.TraceSource.dll": {}
  },
  "runtime": {
    "runtimes/win/lib/netstandard1.3/System.Diagnostics.TraceSource.dll": {}
  }
},

[ericstj]

I understand, I'm talking about the desktop case where runtimeTargets are meaningless since nothing knows how to load them. If you'd like you can treat that separately but make sure we have an issue on it.

[dsplaisted]

@nguerrera @ericstj @eerhardt I've also added a fix for #1465 to this now, at @nguerrera's suggestion

[dsplaisted]

@dotnet-bot test this please

[dsplaisted]

@dotnet-bot test OSX10.12 Release

[sbomer]

Just stumbled across this PR because I was looking into a related issue where the trimming logic doesn't handle resource dlls. I'm wondering: would it make sense to use DestinationSubPath instead of DestinationSubDirectory for satellite assemblies? This would seem more consistent to me, and would require less of a special case for satellite assemblies in GetTargetPath.

[dsplaisted]

@sbomer DestinationSubDirectory is what MSBuild uses when copying CopyLocal files to the output path: https://github.com/Microsoft/msbuild/blob/9ed9a33898750567e1ac653174038e542158cf02/src/Tasks/Microsoft.Common.CurrentVersion.targets#L4236

The SDK uses DestinationSubPath for some things, perhaps it should have used the same name. :-\

[sbomer]

@dsplaisted That's a bit unfortunate. Thanks for the explanation!

[wli3]

26m13.112s on my machine

[dsplaisted]

Customer scenario

Use System.Security.Cryptography.Algorithms in project targeting .NET Framework and AnyCPU. (The fix applies to a lot of scenarios involving conflict resolution. For some of those, the impact is only spurious conflict messages in the build log. The scenario with System.Security.Cryptography.Algorithms, however, is an example of a case that leads to a runtime failure.)

Bugs this fixes

#1510 and #1465

Workarounds, if any

A workaround for one of the scenarios was to add the following:

  <Target Name="WorkaroundConflictResolution" AfterTargets="_ComputeActiveTFMFileDependencies"
          Condition="'$(PlatformTarget)' == 'AnyCPU' And '$(TargetFrameworkIdentifier)' == '.NETFramework'">
    <ItemGroup>
      <_ActiveTFMFileDependencies Remove="@(_ActiveTFMFileDependencies->WithMetadataValue('FileGroup', 'RuntimeTarget'))" />
    </ItemGroup>
  </Target>

For other scenarios, other workarounds may be necessary.

Risk

Low - changes are targeted to consider extra information during conflict resolution

Performance impact

Low

Root cause analysis

One of the inputs to conflict resolution is the path that a file would be published to. We're not actually publishing so we are essentially duplicating part of that logic for conflict resolution. The "duplicated" logic didn't take into account some cases where the target path would be in a subfolder of the output.

How was the bug found?

Customer and internal reports

@MattGertz for approval