Source-build: ASP.NET Core shared framework file differences

[omajid]

I did a diff of building commit 25deb0c043ab69bae6610196297e82d49871c343 with the Microsoft-published .NET 5 SDK. There are some differences in the ASP.NET Core shared framework. Are these expected?

diff -u <(tar tf ./artifacts/x64/Release/dotnet-sdk-5.0.100-fedora.32-x64.tar.gz | sort -u ) <(tar tf dotnet-sdk-microsoft-5.0.100-linux-x64.tar.gz | sort -u)

 ./shared/Microsoft.AspNetCore.App/5.0.0/Microsoft.Extensions.WebEncoders.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/Microsoft.JSInterop.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/Microsoft.Net.Http.Headers.dll
-./shared/Microsoft.AspNetCore.App/5.0.0/Microsoft.Win32.Registry.dll
+./shared/Microsoft.AspNetCore.App/5.0.0/Microsoft.Win32.SystemEvents.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/RuntimeList.xml
 ./shared/Microsoft.AspNetCore.App/5.0.0/System.Diagnostics.EventLog.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/System.Drawing.Common.dll
-./shared/Microsoft.AspNetCore.App/5.0.0/System.Formats.Asn1.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/System.IO.Pipelines.dll
-./shared/Microsoft.AspNetCore.App/5.0.0/System.Security.AccessControl.dll
-./shared/Microsoft.AspNetCore.App/5.0.0/System.Security.Cryptography.Cng.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/System.Security.Cryptography.Pkcs.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/System.Security.Cryptography.Xml.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/System.Security.Permissions.dll
-./shared/Microsoft.AspNetCore.App/5.0.0/System.Security.Principal.Windows.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/System.Windows.Extensions.dll
 ./shared/Microsoft.AspNetCore.App/5.0.0/THIRD-PARTY-NOTICES.txt
 ./shared/Microsoft.AspNetCore.App/5.0.0/.version

[dagood]

The extra DLLs in the source-built SDK are expected, tracked by https://github.com/dotnet/source-build/issues/1779 and not harmful.

Missing Microsoft.Win32.SystemEvents.dll isn't something I've seen before. @dseefeld @crummel ?

[dagood]

Microsoft.Win32.SystemEvents.dll

Strange, we do have a prebuilt usage here that I've been planning to look at. Seems unlikely to just be coincidence...

Id, Version | Category | Owner
-- | -- | --
Microsoft.Win32.SystemEvents,4.7.0 | Should accept dotnet/runtime Microsoft.Win32.SystemEvents 5.0: included in SDK | dagood

[dseefeld]

The extra DLLs in the source-built SDK are expected, tracked by #1779 and not harmful.

Missing Microsoft.Win32.SystemEvents.dll isn't something I've seen before. @dseefeld Dan Seefeldt FTE @crummel Chris Rummel FTE ?

I haven't seen this one before, either.

[dagood]

Looking at a standalone vs. source-build of aspnetcore, at artifacts/obj/Microsoft.AspNetCore.App.Runtime/project.assets.json, I see that System.Drawing.Common/5.0.0 is missing its dependency on SystemEvents and has some other differences that look more expected. This dependency is normally the (only) way it's normally brought in by the standalone build:

--- "a/.\\bad.txt"
+++ "b/.\\good.txt"
       "System.Drawing.Common/5.0.0": {
         "type": "package",
+        "dependencies": {
+          "Microsoft.Win32.SystemEvents": "5.0.0"
+        },
         "compile": {
-          "ref/net5.0/_._": {}
+          "ref/netcoreapp3.0/_._": {}
         },
         "runtime": {
-          "lib/net5.0/_._": {}
+          "lib/netcoreapp3.0/System.Drawing.Common.dll": {}
         },
         "runtimeTargets": {
-          "runtimes/unix/lib/net5.0/System.Drawing.Common.dll": {
+          "runtimes/unix/lib/netcoreapp3.0/System.Drawing.Common.dll": {
             "assetType": "runtime",
             "rid": "unix"
           },
-          "runtimes/win/lib/net5.0/_._": {
+          "runtimes/win/lib/netcoreapp3.0/System.Drawing.Common.dll": {
             "assetType": "runtime",
             "rid": "win"
           }

(Looking at dotnet/runtime for cause... but it actually might be unrelated to the prebuilt, not seeing anything yet that would be in common.)

[dagood]

My guess so far is that it's because the reference is only added when TargetsWindows is true in dotnet/runtime. I imagine we don't build with that configuration in source-build, but I'm not 100% sure what we expect to build right now. (Haven't caught up with the dotnet/runtime source-build efforts yet.) https://github.com/dotnet/runtime/blob/v5.0.0-rtm.20519.4/src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj#L357-L359 :

  <ItemGroup Condition="'$(TargetsWindows)' == 'true'">
    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Win32.SystemEvents\src\Microsoft.Win32.SystemEvents.csproj" />  
  </ItemGroup>

@JunTaoLuo, @ericstj, do you know if missing Microsoft.Win32.SystemEvents.dll from a linux-x64 ASP.NET Core shared framework will cause bad behavior? (Maybe our users will see type load exceptions when referencing types, where normally they'd only get PlatformNotSupportedException if they try to use them?)

Would it be better to directly reference Microsoft.Win32.SystemEvents in aspnetcore to bring it in (looks easy enough in aspnetcore /eng/SharedFramework.External.props), or should we force dotnet/runtime's System.Drawing.Common proj to reference Microsoft.Win32.SystemEvents so it works normally from aspnetcore's perspective?

/cc @jaredpar (dotnet/runtime sb champ)

[dagood]

Triage notes: @JunTaoLuo, @ericstj, @jaredpar, any comments on this? We'd like to figure out if this will be a big issue with source-built 5.0.0.

[ericstj]

My guess so far is that it's because the reference is only added when TargetsWindows is true in dotnet/runtime.

You're building packages in dotnet/runtime, so those packages should include all builds, right? The method of inclusion is through the package reference in ASP.NETCore. I'd expect the System.Drawing.Common package to still have this package reference/

Since it's not part of the refpack to begin with folks would need to reference a package to use it. You could test what an ASP.NET app that references this package gets in its output folder. If it doesn't get this DLL then its expecting it from the framework. If it's missing from the framework that means FileLoadExceptions.

That said: I'm not sure how common it is for folks use use SystemEvents from an ASPNETCore app.

[dagood]

You could test what an ASP.NET app that references this package gets in its output folder.

Gave this a try with the dotnet new web template and this proj:

<Project Sdk="Microsoft.NET.Sdk.Web">
  <PropertyGroup>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Microsoft.Win32.SystemEvents" Version="5.0.0" />
  </ItemGroup>
</Project>

With a recent local source-build, dotnet build puts it in this output folder: bin/Debug/net5.0/Microsoft.Win32.SystemEvents.dll.

But... when I build the same thing with the Microsoft-built 5.0 SDK, Microsoft.Win32.SystemEvents.dll doesn't show up.

This seems to me to mean that if we don't fix this, apps that are built using the Microsoft-built SDK and deployed to a machine that has a source-built Runtime will fail to run if they use this dependency.

/cc @dleeapho

I'd expect the System.Drawing.Common package to still have this package reference

I guess this is where we should fix it then. I'm not sure what we expect the dotnet/runtime build to produce and how that interacts with TargetsWindows. Maybe @dseefeld can shed some light? (Does allconfigurations mean OS configurations as well, or something? Are we even doing allconfigurations right now?)

Maybe we can just remove Condition="'$(TargetsWindows)' == 'true'" with a patch (for now)?

[dagood]

I put up https://github.com/dotnet/source-build/pull/1912 to try adding or '$(DotNetBuildFromSource)' == 'true' to that condition. We should be able to see in CI if that worked based on what Microsoft.AspNetCore.App.Runtime.linux-x64.5.0.0.nupkg ends up containing. (I'd expect it to have the same effect there as what happens for the sharedfx, and we have better access to this than the source-built SDK/Runtime.)

[dagood]

That didn't work--seemed to have no effect at all. 😕

I tried to get at the 5.0.0 dotnet/runtime official build to see where these dependencies come from, but it looks like it's been deleted. Filed https://github.com/dotnet/arcade/issues/6615 for that. Trying a local build.

[dagood]

It looks like the GetFilePackageReferences target is run on each assembly that is built. (Per TFM and platform.) It extracts the assembly references from each one, and matches those up against packages. That ends up being the package's dependencies.

With a standalone build, it finds a artifacts/bin/System.Drawing.Common/netcoreapp3.0-Windows_NT-Debug/System.Drawing.Common.dll file that references Microsoft.Win32.SystemEvents.

In source-build, that DLL doesn't exist (in the binlog it helpfully points this out with a _missingHarvestFile item group). The one that does exist, artifacts/bin/System.Drawing.Common/net5.0-Unix-Release/System.Drawing.Common.dll, doesn't have a reference on Microsoft.Win32.SystemEvents, according to the binlog. Csc does show Microsoft.Win32.SystemEvents as a Reference, but I guess it doesn't make it further. Maybe if it's unused, Csc doesn't put it in the assembly?

We can still try patching to have dotnet/aspnetcore include the package, rather than fixing dotnet/runtime, if we can't get any help on that. I'll try that now.

[ericstj]

I see, that makes sense. The source-build isn't building all configurations and the package dependencies are harvested from the assembly references.

One thing that comes to mind here : in dotnet/runtime we force all runtimes to have the exact same file set. It seems like ASPNet.Core should do that to since the SDK depends on it. Simple fix here is to add a packagereference for every binary which makes up shared framework across any runtime and never rely on a transitive reference.

[dagood]

in dotnet/runtime we force all runtimes to have the exact same file set.

Just to confirm, you mean the proper .NET Runtime / Shared Framework here, not something else like the runtimes/ dirs in these nupkgs?

Simple fix here is to add a packagereference for every binary which makes up shared framework across any runtime and never rely on a transitive reference.

That sounds like what I'm ending up doing with the dotnet/aspnetcore patch I was thinking about. 😄 I have https://github.com/dotnet/source-build/pull/1915 on that to try it out blindly. (Although only for this particular package for now.)

---

This file in aspnetcore has some words about what they have set up currently, but I don't see a why: https://github.com/dotnet/aspnetcore/blob/045882af9d80bebf581b4f1786ad4f4b4f9448d9/eng/SharedFramework.External.props#L46-L54

Transitive dependencies of other assemblies in the shared framework. These are listed separately and should not be included directly when setting <Reference>. These are listed for the purpose of tests and servicing builds only.

If implementation details change and these assemblies are no longer showing up in the shared framework as a result of that,
it is okay to remove these transitive dependencies.

If these are needed as direct dependencies, it is okay to change them to ExternalAspNetCoreAppReference and move up into sections above.

The why I'm looking for is for this: These are listed separately and should not be included directly when setting <Reference>.
@JunTaoLuo @Pilchie why is that? (Or who can answer this?)

Looking a bit further down in the file, it looks like we may not have run into this in 5.0.1 (once IsServicingBuild get turned on) since that adds all the transitive references into the direct reference item list. That implies to me that this is some kind of way to allow transitive dependencies to dissolve themselves during day-to-day dev up until the RTM build, but I don't know.

[dagood]

I merged https://github.com/dotnet/source-build/pull/1915 that fixed the missing DLL in Microsoft.AspNetCore.App.Runtime.linux-x64.5.0.0.nupkg by forcing a direct dependency. I'm going to leave this open until someone has a new SDK source-built that confirms the real sharedfx/Runtime is also fixed.

[Pilchie]

Tagging the rest of @dotnet/aspnet-build, though it's unlikely you'll get a response before Monday.

[dagood]

Confirmed that ./shared/Microsoft.AspNetCore.App/5.0.0/Microsoft.Win32.SystemEvents.dll is present in the SDK's sharedfx with a local build I did today, with the patch. Closing the source-build issue.

Open question/suggestion for ASP.NET Core infrastructure improvement:

[ericstj]
One thing that comes to mind here : in dotnet/runtime we force all runtimes to have the exact same file set. It seems like ASPNet.Core should do that to since the SDK depends on it. Simple fix here is to add a packagereference for every binary which makes up shared framework across any runtime and never rely on a transitive reference.