Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double write during publish #3257

Closed
sbomer opened this issue May 22, 2019 · 22 comments · Fixed by #3320
Closed

Double write during publish #3257

sbomer opened this issue May 22, 2019 · 22 comments · Fixed by #3320
Assignees
Milestone

Comments

@sbomer
Copy link
Member

@sbomer sbomer commented May 22, 2019

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 #3035 which was addressed in #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.

image

@nguerrera nguerrera added the Bug label May 22, 2019
@nguerrera nguerrera added this to the 3.0.1xx milestone May 22, 2019
@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented May 22, 2019

@peterhuene

This comment has been minimized.

Copy link
Contributor

@peterhuene peterhuene commented May 23, 2019

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

This comment has been minimized.

Copy link
Member Author

@sbomer sbomer commented May 23, 2019

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

This comment has been minimized.

Copy link
Member Author

@sbomer sbomer commented Jun 3, 2019

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 nguerrera self-assigned this Jun 3, 2019
@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 3, 2019

I'll take a look.

@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 7, 2019

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

@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Contributor

@swaroop-sridhar swaroop-sridhar commented Jun 7, 2019

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 comment has been minimized.

Copy link
Contributor

@swaroop-sridhar swaroop-sridhar commented Jun 7, 2019

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

@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Contributor

@peterhuene peterhuene commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Member

@ericstj ericstj commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 7, 2019

I'll look into matching the host policy.

@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 7, 2019

dotnet/corefx#34906 (comment)

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

This comment has been minimized.

Copy link
Member

@dagood dagood commented Jun 7, 2019

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 dotnet/core-setup#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

This comment has been minimized.

Copy link
Member

@vitek-karas vitek-karas commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Member

@ericstj ericstj commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Member

@ericstj ericstj commented Jun 7, 2019

Update: here's the discussion I was remembering. dotnet/core-setup#4047

@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 7, 2019

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

@nguerrera

This comment has been minimized.

Copy link
Member

@nguerrera nguerrera commented Jun 10, 2019

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

microsoft/msbuild#4429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.