Double write during publish

[sbomer]

Publishing a self-contained WPF template app writes some files twice because ResolvedFileToPublish has the same dll from different runtime packs (for example, Microsoft.Win32.Registry.dll comes from both netcoreapp and the windowsdesktop pack). This causes crossgen failures when used together with PublishReadyToRun=true.

Looks very similar to https://github.com/dotnet/sdk/issues/3035 which was addressed in https://github.com/dotnet/sdk/pull/3021, but I'm still seeing this with version 3.0.100-preview6-012031.

/cc @fadimounir @nguerrera @peterhuene

edit: it fails during PublishReadyToRun=true only when used together with the linker. Without linking, the duplicates are already crossgen'd, and so they don't hit the failure. In any case, ResolvedFileToPublish has duplicates that shouldn't be there, and we end up with DoubleWrites of some files.

[nguerrera]

cc @dsplaisted

[peterhuene]

This differs from #3035 because that bug was caused by having an item being a member of two intermediate groups and sourcing the final group from both of those intermediate groups, thereby causing the duplicate.

This bug, however, has ResolveRuntimePackAssets outputting the duplicate items because the item shows up in multiple runtime packs.

Other than ideally having composite runtime packs that don't contain duplicates, how should we solve this? ResolvePackageFileConflicts only handles conflicts between the runtime pack assets and package assets, it won't dedup the runtime pack assets itself.

Should ResolveRuntimePackAssets handle conflicts between the packs and only output unique items?

[sbomer]

Not sure if it's relevant, but I remember seeing WindowsBase.dll in both the netcoreapp and windowsdesktop runtime pack, but that assembly seems to be correctly dedup'd. I'm wondering what mechanism does that, and why it's not doing the same for these other assemblies?

[sbomer]

We now have folks hitting this when trying to trim+R2R their WPF apps, FYI:

"D:\dev\NuGetPackageExplorer\PackageExplorer\NuGetPackageExplorer.csproj" (publish target) (1) ->
(_CreateR2RImages target) ->
  C:\Program Files\dotnet\sdk\3.0.100-preview6-012151\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Publish.targets(284,28): error MSB4094: "obj\Debug\ netcoreapp3.0\win-x86\linked\Microsoft.Win32.Registry.dll;obj\Debug\netcoreapp3.0\win-x86\linked\Microsoft.Win32.Registry.dll" is an invalid value for  the "CompilationEntry" parameter of the "RunReadyToRunCompiler" task. Multiple items cannot be passed into a parameter of type "Microsoft.Build.Frame work.ITaskItem". [D:\dev\NuGetPackageExplorer\PackageExplorer\NuGetPackageExplorer.csproj]

@nguerrera

[nguerrera]

I'll take a look.

[nguerrera]

@swaroop-sridhar You are seeing more than Microsoft.Win32.Registry, can you provide more info on that here too.

[nguerrera]

I think the difference between Microsoft.Win32.Registry and WindowsBase is that the latter have different versions between the runtime packs, engineered to make WindowsBase from WindowsDesktop pack win. Whereas registry is the same, but excluded from compile in base targeting pack, IIRC. cc @ericstj

If that's right, (haven't proven yet), then we need to pick a deterministic strategy like first runtime pack wins.

[nguerrera]

Yeah, they have the same version.

Thinking about this, I think these shouldn't be in the runtime pack for windows desktop. It could be a flaw in how that is produced.

[swaroop-sridhar]

Yes, I tried publishing a template Winforms app dotnet publish -c release -r win10-x64 We see several files (including satellite assemblies) coming from multiple targets.

_ResolvedFileToPublishPreserveNewest=

C:\Users\name\.nuget\packages\runtime.win-x64.microsoft.netcore.app\3.0.0-preview6-27804-01\runtimes\win-x64\lib\netcoreapp3.0\Microsoft.Win32.Registry.dll
                                   AssetType=runtime
                                   CopyLocal=true
                                   CopyToPublishDirectory=PreserveNewest
                                   DestinationSubPath=Microsoft.Win32.Registry.dll
                                   IsTrimmable=true
                                   PackageName=runtime.win-x64.Microsoft.NETCore.App
                                   PackageVersion=3.0.0-preview6-27804-01
                                   RelativePath=Microsoft.Win32.Registry.dll
                                   RuntimeIdentifier=win-x64

C:\Users\name\.nuget\packages\runtime.win-x64.microsoft.windowsdesktop.app\3.0.0-preview6-27804-01\runtimes\win-x64\lib\netcoreapp3.0\Microsoft.Win32.Registry.dll
                                   AssetType=runtime
                                   CopyLocal=true
                                   CopyToPublishDirectory=PreserveNewest
                                   DestinationSubPath=Microsoft.Win32.Registry.dll
                                   IsTrimmable=
                                   PackageName=runtime.win-x64.Microsoft.WindowsDesktop.App
                                   PackageVersion=3.0.0-preview6-27804-01
                                   RelativePath=Microsoft.Win32.Registry.dll
                                   RuntimeIdentifier=win-x64

The files that are duplicated are:

Microsoft.Win32.Registry.dll
System.IO.FileSystem.AccessControl.dll
System.Security.AccessControl.dll
System.Security.Cryptography.Cng.dll
System.Security.Principal.Windows.dll

cs/PresentationCore.resources.dll
cs/PresentationFramework.resources.dll
cs/PresentationUI.resources.dll
cs/ReachFramework.resources.dll
cs/System.Printing.resources.dll
cs/System.Windows.Controls.Ribbon.resources.dll
cs/System.Windows.Forms.Design.Editors.resources.dll
cs/System.Windows.Forms.Design.resources.dll
cs/System.Windows.Forms.resources.dll
cs/System.Windows.Input.Manipulations.resources.dll
cs/System.Xaml.resources.dll
cs/UIAutomationClient.resources.dll
cs/UIAutomationClientSideProviders.resources.dll
cs/UIAutomationProvider.resources.dll
cs/UIAutomationTypes.resources.dll
cs/WindowsBase.resources.dll
cs/WindowsFormsIntegration.resources.dll

Similarly for languages: de, es, fr, it, ja, ko, pl, pt-BR, ru, tr, zh-Hans, zh-Hant

[swaroop-sridhar]

This issue did not exist in Preview 5, it is a new regression.

[nguerrera]

Ok, looks like two issues here, but I'll fix them together. The resource assembly is being put twice in the resolved files with the same source, whereas the others are duplicated between runtime packs

[peterhuene]

I have a test case that catches duplicates for publish, but it's for a console app and not WPF (we should probably change it to a theory that tests the supported framework references).

Regarding the satellite assemblies for WPF, I recently implemented copying those locally, so not a regression, but a bug in the implementation I didn't catch.

[ericstj]

So today the fact that Registry isn’t available for reference from NETCore.App is intentional. You could solve this with conflict resolution if you make it behave the same as the host. This means providing correct package ranks for the layered shared frameworks.

It makes me nervous to have this excluded from ref for NETCore.App and excluded for runtime in Window.Desktop, but I guess that’s not much different than having an assembly in WindowsDesktop use all the API exposed... I guess we could do that @DaGood what do you think?

[nguerrera]

I'll look into matching the host policy.

[nguerrera]

https://github.com/dotnet/corefx/issues/34906#issuecomment-458459882

I think understanding the framework layers is going to be painful. Can we say that equal versions means the choice is arbitrary. We'd then just pick something deterministic? @vitek-karas

[dagood]

It makes me nervous to have this excluded from ref for NETCore.App and excluded for runtime in Window.Desktop, but I guess that’s not much different than having an assembly in WindowsDesktop use all the API exposed... I guess we could do that @dagood what do you think?

I probably don't know the full range of things to be nervous about with this--the concrete thing I can think of is the files disappearing from NETCoreApp's runtime, breaking WindowsDesktop runtime. I'm pretty sure VerifyClosure wouldn't catch that since it only looks at NETCoreApp's References, but maybe it could be enhanced? Then we could make some change (not necessarily in VerifyClosure) to catch duplicate assemblies with the same versions to change to ref-only. Filed https://github.com/dotnet/core-setup/issues/6727.

But that just solves duplicates between NETCoreApp and WindowsDesktop, not sibling frameworks like WindowsDesktop and AspNetCore, so making publish nicely handle duplicates still seems necessary. Removing duplicates would just be to reduce FDD and dev-time disk usage.

[vitek-karas]

Without running experiments (just looking at the code) the host will:

• If the two assemblies are of different assembly or file versions, then we take the higher version (assembly version wins over file version)
• If the two assemblies are of the same version, then the lower-level framework will win (honestly kind of weird, but my guess would be it's a side effect of the implementation, although it seems a little bit intentional). So in the above case of WindowsBase being in both WindowsDesktop and NETCore, the one in NETCore would be picked.

We've had this discussion already in some other context (can't find it now) and the outcome was basically: If the two files have the same assembly and file version, then we assume they are identical and thus it should not matter which one we pick.

So if the SDK follows that logic as well, I think we're OK. It should definitely work for our assemblies as we would change file version on rebuilds, even if the assembly version stays the same.

Note: For the sibling case (same assembly in WindowsDesktop and AspNetCore) the outcome is basically random - we will pick one of them at random (I mean I could give you the exact order, it's deterministic, but it depends on order of records in .runtimeconfig.json and possibly even on file locations on disk).

[ericstj]

If the two assemblies are of the same version, then the lower-level framework will win (honestly kind of weird, but my guess would be it's a side effect of the implementation, although it seems a little bit intentional).

I am pretty sure this was intentional. @steveharter and I agreed upon this behavior when this feature was added to the host. I believe there is a pretty big spec on this. (edit, I don't trust my memory here, consult spec)

So in the above case of WindowsBase being in both WindowsDesktop and NETCore, the one in NETCore would be picked.

No, we intentionally give WPF's dll a higher file version. In time it will have a higher assembly version as well: we were careful early in 3.0 not to change assembly versions so that some design time components could limp along using desktop as a proxy. Soon that will no longer be the case: corefx will stay at the desktop assembly version (since its a compat ship) and WPF will increment (since its a product assembly).

[ericstj]

Update: here's the discussion I was remembering. https://github.com/dotnet/core-setup/issues/4047

[nguerrera]

I have a fix locally for the satellite duplicates. It comes down to an msbuild batching behavior that I don't actually understand yet. 😊 I will add some test coverage and also file a discussion issue on the msbuild repo so I can solidify my understanding and help future me and others with similar issues.

[nguerrera]

The satellite issue is entirely separate from the conflict resolution we're talking about.

[nguerrera]

also file a discussion issue on the msbuild repo so I can solidify my understanding and help future me and others with similar issues.

https://github.com/microsoft/msbuild/issues/4429