Diff Microsoft-built vs. source-built 6.0.100 SDK #9

dagood · 2021-10-23T00:09:49Z

Version replacements to minimize diff a little:

/6.0.0-rtm.21522.1/ -> /6.0.0-rtm.21521.10/
/6.0.100-rtm.21521.16/ -> /6.0.100-rtm.21522.10/

Version replacements to minimize diff a little: * /6.0.0-rtm.21522.1/ -> /6.0.0-rtm.21521.10/ * /6.0.100-rtm.21521.16/ -> /6.0.100-rtm.21522.10/

dagood

Adding comments that people started to make on these diffs before I sent this out as a PR.

dagood · 2021-10-23T00:14:38Z

contents.txt

@@ -3848,7 +2985,6 @@
 ./shared/Microsoft.NETCore.App/6.0.0/System.Xml.XmlSerializer.dll
 ./shared/Microsoft.NETCore.App/6.0.0/System.Xml.XPath.dll
 ./shared/Microsoft.NETCore.App/6.0.0/System.Xml.XPath.XDocument.dll
-./shared/Microsoft.NETCore.App/6.0.0/.version


[@omajid]
I can't see anything that uses the runtime's .version file (though I do see some users of the SDK's version file). Still, this seems like something that might break callers?

IIRC this file is intended to be purely informational--it would contain the commit hash on the first line and then the version number on a second line. IMO would be good to put back in place for parity, although I doubt it would cause problems with "ordinary" SDK/sharedfx usage.

Investigate 6.0 SDK missing file: shared/Microsoft.NETCore.App/6.0.0/.version dotnet/source-build#2569

dagood · 2021-10-23T00:15:56Z

contents.txt

+./sdk/6.0.100/testhost.x86.deps.json
+./sdk/6.0.100/testhost.x86.dll
+./sdk/6.0.100/testhost.x86.dll.config
+./sdk/6.0.100/testhost.x86.runtimeconfig.json


[@omajid]
This one looks somewhat interesting:

+./sdk/6.0.100/testhost.deps.json +./sdk/6.0.100/testhost.dll +./sdk/6.0.100/testhost.dll.config +./sdk/6.0.100/testhost.runtimeconfig.json +./sdk/6.0.100/testhost.x86 +./sdk/6.0.100/testhost.x86.deps.json +./sdk/6.0.100/testhost.x86.dll +./sdk/6.0.100/testhost.x86.dll.config +./sdk/6.0.100/testhost.x86.runtimeconfig.json

The name is not what I expected: x86 instead of x64? In RC2,
testhost.x86 was also a native binary that doesn't have debug info, so I am not even sure where that was coming from. That it's also missing in the Microsoft-built SDK is very strange. Is that something you folks are investigating?

[Poison] TestHost files are leaking to source-built SDK dotnet/source-build#2579

dagood · 2021-10-23T00:17:40Z

contents.txt

 ./sdk/6.0.100/de/MSBuild.resources.dll
-./sdk/6.0.100/de/NuGet.Build.Tasks.Console.resources.dll
-./sdk/6.0.100/de/NuGet.Build.Tasks.resources.dll


[@omajid]
For the localization stuff, do you have a plan to "fix" it so that source-build and Microsoft-built SDKs have the same resources (eg -./sdk/6.0.100/de/NuGet.Build.Tasks.resources.dll) at some point?

6.0 SDK diff: Add localization resource DLLs dotnet/source-build#2571

contents.txt

dagood · 2021-10-23T00:19:58Z

contents.txt

@@ -2202,6 +1794,7 @@
 ./sdk/6.0.100/Sdks/FSharp.NET.Sdk/Sdk/Sdk.props
 ./sdk/6.0.100/Sdks/FSharp.NET.Sdk/Sdk/Sdk.targets
 ./sdk/6.0.100/Sdks/Microsoft.Docker.Sdk/
+./sdk/6.0.100/Sdks/Microsoft.Docker.Sdk/microsoft.docker.sdk.1.1.0.csproj


[@omajid]
This file seems like something didn't get filtered out. Could this break any users? It just won't get used, right?

I don't think .csproj would be automatically picked up by anything, so I think it's safe. This issue tracks fixing this kind of file being included from SBRP:

Text-only packages built by SBRP have extra files in them (intermediate output, csproj) dotnet/source-build#1906

It's currently in dotnet-sdk-5.0 in Fedora 34:

/usr/lib64/dotnet/sdk/5.0.206/Sdks/Microsoft.Docker.Sdk/Microsoft.Docker.Sdk.csproj

contents.txt

dagood · 2021-10-23T00:26:03Z

contents.txt

-./sdk/6.0.100/Extensions/ko/Microsoft.TestPlatform.TestHostRuntimeProvider.resources.dll
-./sdk/6.0.100/Extensions/ko/Microsoft.VisualStudio.TestPlatform.Extensions.Html.TestLogger.resources.dll
-./sdk/6.0.100/Extensions/ko/Microsoft.VisualStudio.TestPlatform.Extensions.Trx.TestLogger.resources.dll
-./sdk/6.0.100/Extensions/Microsoft.Diagnostics.NETCore.Client.dll


[@eerhardt]
who knows about vstest stuff? the diffs in that area seem interesting as well

RE: This line, and also the testhost diffs.

Investigate 6.0 SDK diffs: testhost files dotnet/source-build#2570

For diff reduction: 6.0.0-rtm.21525.1 -> 6.0.0-rtm.21521.10 6.0.100-rtm.21523.6 -> 6.0.100-rtm.21522.10

eerhardt · 2021-10-26T20:09:49Z

contents.txt

@@ -5,6 +5,10 @@
 ./host/fxr/6.0.0/
 ./host/fxr/6.0.0/libhostfxr.so
 ./LICENSE.txt
+./metadata/


What are these? Were they here previously?

These are source-build specific changes to enable non-root users to install SDK workloads. See dotnet/installer#12021 and dotnet/installer#12104 for more details. It's safe to ignore them.

contents.txt

eerhardt · 2021-10-26T20:19:09Z

contents.txt

-./sdk/6.0.100/FSharp/runtimes/win/lib/netstandard2.0/System.Security.Cryptography.ProtectedData.dll
-./sdk/6.0.100/FSharp/System.CodeDom.dll
-./sdk/6.0.100/FSharp/System.Configuration.ConfigurationManager.dll
-./sdk/6.0.100/FSharp/System.Drawing.Common.dll


These are .... interesting. Do we have F# smoke tests to ensure F# still works?

I'm giving this a try locally. There are F# smoke-tests, but run is not in the list right now because of some known failures (with the Microsoft build, too).

For followup on the F# uptake not behaving as we normally expect: Investigate F# integration into the .NET SDK: figure out what would make it more source-build and smoke-test compatible dotnet/source-build#2581

I'm able to get F# to run a dotnet new web --language F# project fine, with workaround from the above issue.

I'm not able to find any product problems that happen because of the missing files, but I'm not sure what F# SDK features would be using these assemblies, so I could easily be missing something. 😕

For System.Drawing.Common.dll in particular, there's an existing diff in 5.0 that isn't resolved:

Missing System.Drawing.Common.dll in 5.0 builds dotnet/source-build#2018

For followup on 6.0.0, running more tests, filed:

Run a F# test on the 6.0.0 SDK that targets missing files according to the SDK diff dotnet/source-build#2582

eerhardt · 2021-10-26T20:25:43Z

contents.txt

@@ -1666,7 +1479,6 @@
 ./sdk/6.0.100/Microsoft.DotNet.NativeWrapper.dll
 ./sdk/6.0.100/Microsoft.DotNet.SdkResolver.dll
 ./sdk/6.0.100/Microsoft.DotNet.TemplateLocator.dll
-./sdk/6.0.100/Microsoft.Extensions.CommandLineUtils.dll


FYI - note that this was caused by https://github.com/dotnet/installer/pull/12356/files#diff-df188b804ce1fd62bb0ed1f6bfd73e010e4b6f92295bab371f35e8879783dab5R30.

It took me a while to find it, so I wanted to note it here, in case someone else was interested here.

eerhardt · 2021-10-26T20:36:57Z

contents.txt

-./sdk/6.0.100/Sdks/Microsoft.NET.Sdk.Publish/tools/net472/ko/
-./sdk/6.0.100/Sdks/Microsoft.NET.Sdk.Publish/tools/net472/ko/Microsoft.NET.Sdk.Publish.Tasks.resources.dll
-./sdk/6.0.100/Sdks/Microsoft.NET.Sdk.Publish/tools/net472/Microsoft.Bcl.AsyncInterfaces.dll
-./sdk/6.0.100/Sdks/Microsoft.NET.Sdk.Publish/tools/net472/Microsoft.NET.Sdk.Publish.Tasks.dll


hmmm, this would mean some "publish" things won't work in OmniSharp. Not sure if that is a problem or not.

@vijayrkn - do you know if there is any way to get these "publish" tasks invoked in OmniSharp? If so, we will probably need to update

https://github.com/dotnet/sdk/blob/a30e465a2e2ea4e2550f319a2dc088daaafe5649/src/WebSdk/Publish/Tasks/Microsoft.NET.Sdk.Publish.Tasks.csproj#L14

And start building these tasks for net472 as well.

net472 based assemblies are only used by msbuild invoked within VS. None of the msbuild/sdk core scenarios use these assemblies.

net472 based assemblies are only used by msbuild invoked within VS.

On non-Windows, OmniSharp runs on a mono-based msbuild. This means it acts like a .NET Framework msbuild.exe process, and thus will load the net472 based assemblies.

See https://github.com/OmniSharp/omnisharp-roslyn#introduction

and Mono on OSX/Linux. It targets the net472 target framework. For platforms other than Windows, OmniSharp ships with an embedded Mono which is based on version 6.12.0, includes MSBuild 16.8.0...

There are plans to change this in the future, but for today that's how it works. For the future plans see:

dotnet/vscode-csharp#4843
dotnet/vscode-csharp#4360 (comment)

Chatting offline, @JoeRobich let me know that OmniSharp does a design time build of CoreCompile and Compile targets. So I don't believe these Publish tasks will need to be built for net472 in source-build.

Looking at a diff of 5.0, they weren't built for netfx there either.

eerhardt · 2021-10-26T20:44:17Z

contents.txt

-./sdk/6.0.100/Sdks/NuGet.Build.Tasks.Pack/CoreCLR/zh-Hans/NuGet.Build.Tasks.Pack.resources.dll
-./sdk/6.0.100/Sdks/NuGet.Build.Tasks.Pack/CoreCLR/zh-Hant/
-./sdk/6.0.100/Sdks/NuGet.Build.Tasks.Pack/CoreCLR/zh-Hant/NuGet.Build.Tasks.Pack.resources.dll
+./sdk/6.0.100/Sdks/NuGet.Build.Tasks.Pack/CoreCLR/NuGet.Commands.dll


This is odd. Why would these NuGet assemblies be needed in source-build, but be absent in the MSFT distribution?

cc @nkolev92 @zivkan - do you know why these assemblies are showing up here in the source-built SDK, but not in the MSFT distribution?

We ilmerge into NuGet.Build.Tasks.Pack.dll, and the equivalent is not possible in source build.

Makes sense. Thanks for the info, @nkolev92.

dagood · 2021-10-26T23:16:52Z

contents.txt

+./sdk/6.0.100/DotnetTools/dotnet-format/runtimes/browser/lib/net6.0/System.Text.Encodings.Web.dll
+./sdk/6.0.100/DotnetTools/dotnet-format/runtimes/win/
+./sdk/6.0.100/DotnetTools/dotnet-format/runtimes/win/lib/
+./sdk/6.0.100/DotnetTools/dotnet-format/runtimes/win/lib/net6.0/
+./sdk/6.0.100/DotnetTools/dotnet-format/runtimes/win/lib/net6.0/System.Text.Encoding.CodePages.dll


Investigate 6.0 SDK diff: unnecessary runtime/ folders dotnet/source-build#2573

dagood · 2021-10-27T16:33:20Z

I just tried to update the diff against the official offline build from https://dnceng.visualstudio.com/internal/_build/results?buildId=1442031&view=results ("final" bits), but it's identical to what we have here. 😄 (After ordinary version number replacement.)

6.0.0-rtm.21527.1 -> 6.0.0-rtm.21521.10 6.0.100-rtm.21526.8 -> 6.0.100-rtm.21522.10

dagood · 2021-10-27T19:53:22Z

contents.txt

 ./sdk/6.0.100/DotnetTools/dotnet-watch/6.0.100-rtm.21522.10/tools/net6.0/any/pt-BR/System.CommandLine.resources.dll
-./sdk/6.0.100/DotnetTools/dotnet-watch/6.0.100-rtm.21522.10/tools/net6.0/any/ref/
-./sdk/6.0.100/DotnetTools/dotnet-watch/6.0.100-rtm.21522.10/tools/net6.0/any/ref/dotnet-watch.dll


Does anyone know more about assemblies showing up in build output as ref/foo.dll? I heard it's a new SDK feature, but I don't know what it's for.

It seems hard to imagine a meaning for "ref" that involves someone referencing this DLL... but I want to make sure to point this out.

It looks to be coming from $(ProduceReferenceAssembly) being true. See dotnet/sdk#12720.

Why it is not being produced for source-build I don't know. Can you check a binlog of the sdk and see what value $(ProduceReferenceAssembly) has for dotnet-watch?

In general, I don't think this ref assembly needs to be in the official product. I'm not sure how dotnet-watch is laid into the SDK, but maybe @dsplaisted @sfoslund can help?

Can you check a binlog of the sdk and see what value $(ProduceReferenceAssembly) has for dotnet-watch?

Will do.

I also noticed a lot more instances when I was scanning through a diff again:

./sdk/6.0.100/Microsoft/Microsoft.NET.Build.Extensions/tools/net6.0/ref/Microsoft.NET.Build.Extensions.Tasks.dll ./sdk/6.0.100/Sdks/Microsoft.NET.Sdk.BlazorWebAssembly/tools/net6.0/ref/Microsoft.NET.Sdk.BlazorWebAssembly.Tasks.dll ./sdk/6.0.100/Sdks/Microsoft.NET.Sdk.Publish/tools/net6.0/ref/Microsoft.NET.Sdk.Publish.Tasks.dll ./sdk/6.0.100/Sdks/Microsoft.NET.Sdk/tools/net6.0/ref/Microsoft.NET.Build.Tasks.dll ./sdk/6.0.100/Sdks/Microsoft.NET.Sdk.Web.ProjectSystem/tools/net6.0/ref/Microsoft.NET.Sdk.Web.ProjectSystem.Tasks.dll ./sdk/6.0.100/Sdks/Microsoft.NET.Sdk.Web/tools/net6.0/ref/Microsoft.NET.Sdk.Web.Tasks.dll ./sdk/6.0.100/Sdks/Microsoft.NET.Sdk.Worker/tools/net6.0/ref/Microsoft.NET.Sdk.Worker.Tasks.dll

I'm not sure how dotnet-watch is laid into the SDK

I don't have a great understanding in that area either, @pranavkm would probably know?

Hmm. This was done intentionally in source-build's repos/sdk.proj:

 <BuildCommandArgs>$(BuildCommandArgs) /p:ProduceReferenceAssembly=false</BuildCommandArgs>

I suppose dotnet/sdk updated to a newer version of the .NET SDK as stage 0 since this comment was written, so the Microsoft build started using the new default. Then this line suddenly introduced diffs rather than fixing them. 😄

I'll create a source-build issue to track this and submit a PR to remove those lines, for servicing consideration.

Allow SDK to produce reference assemblies dotnet/installer#12549

6.0 SDK diff: Allow dotnet/sdk to produce reference assemblies dotnet/source-build#2586

Also

The .NET SDK is unnecessarily shipping 'ref' assemblies in the product dotnet/sdk#22359

eerhardt · 2021-10-27T20:10:45Z

contents.txt

@@ -1223,7 +1207,6 @@
 ./sdk/6.0.100/DotnetTools/dotnet-watch/6.0.100-rtm.21522.10/tools/net6.0/any/System.Composition.Hosting.dll
 ./sdk/6.0.100/DotnetTools/dotnet-watch/6.0.100-rtm.21522.10/tools/net6.0/any/System.Composition.Runtime.dll
 ./sdk/6.0.100/DotnetTools/dotnet-watch/6.0.100-rtm.21522.10/tools/net6.0/any/System.Composition.TypedParts.dll
-./sdk/6.0.100/DotnetTools/dotnet-watch/6.0.100-rtm.21522.10/tools/net6.0/any/System.IO.Pipelines.dll


Is this good / expected?

System.IO.Pipelines isn't in the shared fx. So if dotnet-watch needs this assembly, this might break it.

It's not in the Microsoft-built SDK, according to the overall diff. I can try to track down what was bringing this in the non-bootstrapped build.

I can try to track down what was bringing this in the non-bootstrapped build.

Not getting into this for now. This change makes us have the same DLLs as the Microsoft-built SDK, so we'll assume including it in the non-bootstrapped build was just some version-related issue that bootstrapping fixed.

dagood · 2021-10-27T20:56:08Z

I've pushed a few diffs while leaving content.txt alone to avoid potentially wrecking comment context.

I wanted to check the final SDK vs. source-built bootstrap SDK, so I pushed a .diff file that contains that diff. (See final-base-6.0.100...dagood:final-diff-6.0.100 for better colorization.) I also pushed a .diff file for the old Microsoft-built SDK. I diffed those two .diff files against each other to see what changed. It's not fun to read, but I didn't think it was worth spending too much time figuring out a much better way.

dagood · 2021-10-27T21:13:11Z

sdk-old-to-final-diff.diff

+ -./sdk/6.0.100/cs/NuGet.Versioning.resources.dll
+ -./sdk/6.0.100/cs/NuGet.VisualStudio.Contracts.resources.dll
+  ./sdk/6.0.100/cs/System.CommandLine.resources.dll
+--./sdk/6.0.100/cs/Test.Utility.resources.dll


This -- line means that the old diff had -./sdk/6.0.100/cs/Test.Utility.resources.dll, but the new diff doesn't have that line. Since the bootstrap SDK didn't change, this means the Microsoft-built SDK has dropped this file.

tar -tf msft-dotnet-sdk-6.0.100-linux-x64.tar.gz | grep Test.Utility shows no results. So: no Test.Utility.resources.dll, and also no Test.Utility.dll files that would be associated with the resource DLLs.

This brings the source-build and Microsoft-build SDKs closer, so no problem here as far as we're concerned.

(Mentioning it to show an example of what the diff-diff tells us and because I did a grep to make sure.)

Source-built SDK from dotnet-format fix PR local tarball build

9c030bc

Version replacements to minimize diff a little: * /6.0.0-rtm.21522.1/ -> /6.0.0-rtm.21521.10/ * /6.0.100-rtm.21521.16/ -> /6.0.100-rtm.21522.10/

dagood commented Oct 23, 2021

View reviewed changes

Update to newer source-built SDK

c16bba2

For diff reduction: 6.0.0-rtm.21525.1 -> 6.0.0-rtm.21521.10 6.0.100-rtm.21523.6 -> 6.0.100-rtm.21522.10

eerhardt reviewed Oct 26, 2021

View reviewed changes

contents.txt Outdated Show resolved Hide resolved

eerhardt reviewed Oct 26, 2021

View reviewed changes

dagood commented Oct 26, 2021

View reviewed changes

This was referenced Oct 27, 2021

Text-only packages built by SBRP have extra files in them (intermediate output, csproj) dotnet/source-build#1906

Closed

Compare final GA source-build to official GA build dotnet/source-build#2544

Closed

Update to bootstrapped SDK

4976a0b

6.0.0-rtm.21527.1 -> 6.0.0-rtm.21521.10 6.0.100-rtm.21526.8 -> 6.0.100-rtm.21522.10

dagood commented Oct 27, 2021

View reviewed changes

eerhardt reviewed Oct 27, 2021

View reviewed changes

Add diffs and diff diff for final SDK

7cd51a5

dagood commented Oct 27, 2021

View reviewed changes

dagood mentioned this pull request Oct 27, 2021

6.0 SDK diff: Allow dotnet/sdk to produce reference assemblies dotnet/source-build#2586

Closed

eerhardt mentioned this pull request Oct 27, 2021

The .NET SDK is unnecessarily shipping 'ref' assemblies in the product dotnet/sdk#22359

Open

Diff Microsoft-built vs. source-built 6.0.100 SDK #9

Are you sure you want to change the base?

Diff Microsoft-built vs. source-built 6.0.100 SDK #9

Conversation

dagood commented Oct 23, 2021

dagood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagood Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagood Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagood Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagood Oct 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omajid Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagood commented Oct 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagood commented Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

dagood Oct 25, 2021 •

edited

Loading

dagood Oct 25, 2021 •

edited

Loading

dagood Oct 25, 2021 •

edited

Loading

dagood Oct 23, 2021 •

edited

Loading

omajid Oct 26, 2021 •

edited

Loading

dagood commented Oct 27, 2021 •

edited

Loading