Sdk: Satellite assemblies from PackageReference all copy to root bin folder

[AArnott]

_From @AArnott on June 20, 2017 15:19_

Details about Problem

NuGet product used (NuGet.exe | VS UI | Package Manager Console | dotnet.exe): 4.3.0

VS version (if appropriate): 15.3.26616.2.d15rel

OS version (i.e. win10 v1607 (14393.321)): Win10 15063.rs2_release.170317-1834

Worked before? If so, with which NuGet version: Yes, this worked in 15.1 or 15.2 I believe.

Detailed repro steps so we can see the same problem

Create a new .NET Core library, then replace the project file content with this:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net46</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.VisualStudio.Composition" Version="15.0.71" />
  </ItemGroup>
</Project>

Restore, and build.

Expected

The project's bin folder has just a few DLLs, and a bunch of culture folders, each with a few *.resources.dll assemblies.

Actual

The bin folder that contains the project output no sub-folders. Instead, "all" the satellite assemblies are copied into that one root bin folder, causing each one to overwrite another so that only one culture wins. And at runtime, the satellite assemblies cannot be found.

_Copied from original issue: NuGet/Home#5458_

[AArnott]

_From @rohit21agrawal on June 20, 2017 18:36_

shouldn't this go to msbuild?

[AArnott]

I expected NuGet owned the .targets that copy files from nuget packages into the user's bin folder based on PackageReference.

[AArnott]

_From @rohit21agrawal on June 20, 2017 18:57_

NuGet doesn't decide where the DLLs would be copied to on disk. It only creates an assets file which is a dependency graph, then NuGetBuildTasks convert these to References. Finally, SDK targets decide what to do with these references. So I would lean towards this being something that SDK isn't handling well.

[AArnott]

Very well. It only seems to repro for .NET SDK projects so I'll move to dotnet/sdk

[nguerrera]

This is the same as #1006 and #1132, fixed by #1224, but that fix is only in v2.0 of the SDK, which requires the v2.0 .NET Core SDK to be installed as it does not ship with Visual Studio 15.3 (only v1.1 does).

I'll be documenting a work-around here shortly for projects that must work on v1.1 too.

cc @jaredpar

[nguerrera]

Here is a workaround when using v2.0 is not possible:

  <!-- Work around bug in Microsoft.NET.Sdk < v2.0 where satellites were deployed on top of each other in root. -->
  <!-- https://github.com/dotnet/sdk/issues/1360 -->
  <Target Name="WorkaroundIncorrectSatelliteDeployment" AfterTargets="ResolveLockFileCopyLocalProjectDeps">
    <ItemGroup>
      <ReferenceCopyLocalPaths Remove="@(ResourceCopyLocalItems)" />
      <ReferenceCopyLocalPaths Include="@(ResourceCopyLocalItems)" Condition="'@(ResourceCopyLocalItems)' != ''">
        <DestinationSubDirectory>$([System.IO.Directory]::GetParent(%(ResourceCopyLocalItems.FullPath)).get_Name())\</DestinationSubDirectory>
      </ReferenceCopyLocalPaths>
    </ItemGroup>
  </Target>

[nguerrera]

Closing as duplicate since this is already fixed in latest bits.

[nguerrera]

Worked before? If so, with which NuGet version: Yes, this worked in 15.1 or 15.2 I believe.

This is not in fact a regression. 15.0, 15.1, 15.2 would have exhibited the same behavior for Microsoft.NET.Sdk-using project targeting net4x (where we copy-local of nuget dependencies is on by default).

[AArnott]

Thanks, @nguerrera

[AArnott]

@onovotny do you want to include this workaround in https://github.com/onovotny/MSBuildSdkExtras?

[clairernovotny]

@nguerrera I assume that workaround should be disabled with the 2.0 SDK so we don't interfere?

Would a check like this work to only apply to SDK's lower than 2.0? https://github.com/paulcbetts/refit/blob/master/Refit/targets/refit.targets#L15-L27

[AArnott]

@onovotny The myapps link you shared doesn't work for me.

[clairernovotny]

arg....clipboard :(

[clairernovotny]

Let's try this again -- https://github.com/onovotny/MSBuildSdkExtras/blob/master/src/build/netstandard1.0/MSBuild.Sdk.Extras.targets#L4-L33

Trying to understand how this workaround and that one relate and if that one is needed anymore (or fixed in 2.0 as well?)

[AArnott]

The one you link to is sort of this one's counterpart. That workaround helps NuGet to properly pack satellite assemblies, while this workaround helps MSBuild _unpack_ them.

[clairernovotny]

Any idea if that pack issue is fixed in the 2.0 SDK? looking to start phasing out the workarounds if running on 2.0.

[AArnott]

The pack bug was fixed in 15.3, I _think_. So IMO it's too early to remove workarounds since most folks aren't on 15.3 yet. But as the .NET Core SDK 2.0 does _not_ ship even with 15.3 (it's a separate install), I think we must assume most folks are on 1.1 of the SDK for (sniff) a long while yet.

[clairernovotny]

@AArnott I think I can detect that though based on checking $(BundledNETCoreAppTargetFrameworkVersion). It's a new property in the 2.0 SDK and has the version of .NET Core in it. It's not quite the same as the SDK version, as the SDK could be higher, but that may work for now?

https://github.com/paulcbetts/refit/blob/master/Refit/targets/refit.targets#L15-L27

[nguerrera]

You could use $(UsingMicrosoftNETSdk) as we only started setting that to true in 2.0. Also, the workaround shouldn't cause any harm, only redundant work, if it runs on a fixed SDK.

[clairernovotny]

@nguerrera is there an actual SDK version number we can check in addition to that prop or is $(BundledNETCoreAppTargetFrameworkVersion) the right way?

[nguerrera]

There's nothing else other than only the boolean I just mentioned. Using BundledNETCoreAppTargetFrameworkVersion should be fine.

[clairernovotny]

ok...thanks. I thought there's an open issue to actually put the SDK version in as a property. Would seem to be useful if the SDK can version higher than the .NET Core version itself.

[clairernovotny]

@AArnott quick code review here? https://github.com/onovotny/MSBuildSdkExtras/commit/d1b552724ca4c7124fbf621903abd2da725a66cc

It's also on the MyGet feed https://www.myget.org/F/msbuildsdkextras/api/v3/index.json, if you wanted to try it out quickly.

[clairernovotny]

@nguerrera that code yields an error:

C:\Users\oren\.nuget\packages\msbuild.sdk.extras\1.0.4-beta.44\build\netstandard1.0\MSBuild.Sdk.Extras.targets(45,9): er
ror MSB4186: Invalid static method invocation syntax: "[System.IO.Directory]::GetParent().get_Name()". System.IO.Directo
ry.GetParent Static method invocation should be of the form: $([FullTypeName]::Method()), e.g. $([System.IO.Path]::Combi
ne(`a`, `b`)).  [C:\dev\Humanizer\src\Humanizer\Humanizer.csproj]
C:\dev\Humanizer\src\Humanizer [dev ≡ +1 ~2 -0 !]>

[nguerrera]

Yep. Fixing it. There's a missing condition on there actually being ResourceCopyLocalItems.

[nguerrera]

https://github.com/dotnet/sdk/issues/1360#issuecomment-310157372 is edited to include the fix. Sorry about that.

[nguerrera]

Edited one more time. Should be good now. Sorry again.

[jaredpar]

@nguerrera verified this works. Our build correctness leg is no longer flagging double writes and I manually verified the output directory contains the proper resource layout. Thanks!

[clairernovotny]

The MSBuild.Sdk.Extras package 1.0.4 contains this workaround too now. The existing workarounds for issues are now only included in when run on with 1.x SDK's, since I believe they've all been fixed in 2.0.
https://github.com/onovotny/MSBuildSdkExtras/blob/master/src/build/netstandard1.0/NetCoreSdk1Workarounds.targets

[AArnott]

Thanks, @onovotny!