Sdk: 3.0 SDK is copying runtimes folders and assets to desktop TF

[jaredpar]

Found during https://github.com/dotnet/roslyn/pull/34093

Repro Steps

• Clone https://github.com/jaredpar/roslyn
• Use branch fix-sdk
• Run Build.cmd -build -restore

Notice that after build every net472 directory has a runtimes folder under it with the assets you'd expect from .NET Core TF. Example:

E:\code\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472\runtimes\win\lib\net461\System.Text.Encoding.CodePages.dll
E:\code\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472\runtimes\win\native\Microsoft.DiaSymReader.Native.amd64.dll
E:\code\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472\runtimes\win\native\Microsoft.DiaSymReader.Native.arm.dll
E:\code\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472\runtimes\win\native\Microsoft.DiaSymReader.Native.x86.dll
E:\code\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472\runtimes\win-x64\native\Microsoft.DiaSymReader.Native.amd64.dll
E:\code\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472\runtimes\win-x86\native\Microsoft.DiaSymReader.Native.x86.dll
E:\code\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472\runtimes\win7-x64\native\e_sqlite3.dll
E:\code\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472\runtimes\win7-x86\native\e_sqlite3.dll
E:\code\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472\runtimes\win8-arm\native\Microsoft.DiaSymReader.Native.arm.dll

[peterhuene]

Hmm, I'm not seeing a runtimes directory on my Windows VM:

C:\Users\peterhuene\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472 (fix-sdk -> origin)
λ dir runtimes
 Volume in drive C has no label.
 Volume Serial Number is 5E75-1BF4

 Directory of C:\Users\peterhuene\roslyn\artifacts\bin\Roslyn.VisualStudio.Next.UnitTests\Debug\net472

File Not Found

[peterhuene]

The only runtimes directories I have seem to be under .NET Core TFMs:

C:\Users\peterhuene\roslyn\artifacts\bin (fix-sdk -> origin)
λ dir runtimes /s
 Volume in drive C has no label.
 Volume Serial Number is 5E75-1BF4

 Directory of C:\Users\peterhuene\roslyn\artifacts\bin\csc\Debug\netcoreapp2.1\publish

03/14/19  11:38 AM    <DIR>          runtimes
               0 File(s)              0 bytes

 Directory of C:\Users\peterhuene\roslyn\artifacts\bin\csi\Debug\netcoreapp2.1\publish

03/14/19  11:38 AM    <DIR>          runtimes
               0 File(s)              0 bytes

 Directory of C:\Users\peterhuene\roslyn\artifacts\bin\vbc\Debug\netcoreapp2.1\publish

03/14/19  11:47 AM    <DIR>          runtimes
               0 File(s)              0 bytes

 Directory of C:\Users\peterhuene\roslyn\artifacts\bin\VBCSCompiler\Debug\netcoreapp2.1\publish

03/14/19  11:47 AM    <DIR>          runtimes
               0 File(s)              0 bytes

     Total Files Listed:
               0 File(s)              0 bytes
               4 Dir(s)  197,285,339,136 bytes free

[jaredpar]

@peterhuene how did you build? What command I mean.

[peterhuene]

build.cmd -restore as instructed.

[peterhuene]

@jaredpar Are you able to reproduce this with a clean repo? I've been unable to reproduce. Was this the only 3.0 SDK version you attempted the upgrade with? Are the file versions recent?

[jaredpar]

Going to try on a different machine.

[jaredpar]

I see this behavior on multiple machines now. On the other machine I observe a runtimes folder in the following location: artifacts\bin\Microsoft.VisualStudio.LanguageServices.Implementation\Debug\net472\runtimes\win\lib\net461

This is the version of the SDK that I'm using https://dotnet.microsoft.com/download/thank-you/dotnet-sdk-3.0.100-preview3-windows-x64-installer

[peterhuene]

I'll continue to try to repro it; that was the SDK I was also using in my VM.

[jaredpar]

FWIW: I did a git clean -dxf . on teh new machine to ensure i was in a clean state.

[peterhuene]

My .NET Core SDK version is 3.0.100-preview3-010413, which matches the requested version in global.json.

I'm still unable to reproduce with a clean build or an incremental build, with and without restoring.

Could you email me a binlog? That would probably be the easiest way to figure out the problem at this point.

[peterhuene]

I'm now able to reproduce this and it is related to the copy-local on build feature. Note for anyone trying to reproduce this with a Visual Studio 16 RC installed: make sure you enable preview SDKs in Visual Studio options because the Roslyn repo builds with desktop MSBuild.

ResolvePackageAssets outputs runtime copy-local items for .NET Framework TFMs, which the copy-local code is copying over. This is the behavior in 2.2 and 3.0.

The copy local logic should limit copying runtime items to .NET Core TFMs.

I'm also going to investigate if a 2.2 application targeting a .NET Framework TFM is copying the runtime assets on publish. If so, then I believe we should be fixing this in ResolvePackageAssets.

[livarcocc]

Curious, what was the key to the repro?

[peterhuene]

I edited the comment above with that information. I was accidentally building with a 2.2 SDK because the repo builds with desktop MSBuild on Windows and I had a VS 16 RC installed (thus not using preview 3.0 SDKs by default).

[peterhuene]

It wasn't until I did a compare-and-contrast binlog with Jared's build logs that I noticed the targets being imported were 2.2 :man_facepalming:

[livarcocc]

binlogs are 🦄

[peterhuene]

I've managed to reproduce this with a 2.2 SDK:

> dotnet new console

Change TargetFramework in the project file to net472.

Add a package that has runtime assets:

> dotnet add package sqlitepclraw.bundle_green

Publish forcing RuntimeIdentifier to be empty (to recreate what we're seeing in the Roslyn repo, but at publish time):

> dotnet publish -r:""

We now have a runtimes folder under net472.

This is essentially what's happening with the 3.0 SDK, except we get in the build output due to the 3.0 copy-local-on-build feature.

As with the Roslyn build, an empty runtime identifier is causing ResolvePackageAssets to output additional rid-agnostic runtime assets despite it being a .NETFramework TFM.

[peterhuene]

More accurately with what's happening with the Roslyn build with a 2.2 SDK:

> dotnet new classlib

Change TargetFramework in the project file to net472.

> dotnet add package sqlitepclraw.bundle_green
> dotnet publish

We have a runtimes directory created in this case because RuntimeIdentifier is empty due to a condition to default it based on there being an executable output.

I'm trying to figure out how we should best address this issue; I'm at least now convinced it needs to happen by either changing this RID defaulting logic or something in ResolvePackageAssets needs to change.

[peterhuene]

@nguerrera it looks like you limited the RID defaulting to executable outputs way back in https://github.com/dotnet/sdk/commit/8ad6b9054abd546c932e7b207ad8d4d122d684de.

Since this was before my time, was there a particular reason for this? Removing the restriction on (what is now) HasRuntimeOutput does indeed solve this issue for Roslyn's build.

[nguerrera]

Defaulting the RID in netfx was something I really didn't want to do at all, but was backed into a corner. The RID also will not be set by default if there's an explicit AnyCPU in the project file. The problem with netfx is exactly that it cannot support the runtimes/ folder and therefore can only have portable output if all of its copy-local dependencies are truly portable. I don't think we should change the classlib default, and even if we did, it would not solve this for explicit AnyCPU. I think the fix is to ignore runtimes/ items for netfx. Maybe it should be the other way around and only consider it for netcoreapp. I'm not aware of another TFM that supports runtimes/ at this time.

[nguerrera]

Looking at this now, it looks like we always had this wrong for publish of net472 and now we're seeing the same bug on build due to build getting the copy-local-like-publish behavior. That makes sense. I think in 3.0 we should not have runtimes/ for either case for netfx.

[nguerrera]

This probably was not caught because most publishes are on apps not classlibs.

[peterhuene]

I'll investigate a fix in ResolvePackageAssets then to only output these runtime items when the TFM is netcoreapp* (with an empty RuntimeIdentifier).

[jaredpar]

@peterhuene saw you added partner-blocking label. Sorry if that got miscommunicated. This is not blocking us. I had to add a small work around to one of our tests but nothing problematic. Can be removed any time.

[peterhuene]

Ah, I thought it was preventing your upgrade to 3.0. Regardless, I'll likely have a fix in today.

[dsplaisted]

@peterhuene Are we fixing this in 2.2 or 3.0? Since it seem like this isn't a regression in 2.2, I would expect we would just fix this in 3.0.

[peterhuene]

I believe 3.0 is where it should be fixed, since it only affects publishing class libraries targeting a .NETFramework TFM in 2.2, and that's not commonly done.