search

dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
6.92k stars 1.15k forks source link

WPF build projects hand pick the Microsoft.NETCore.App references #9168

Open ViktorHofer opened 1 month ago

ViktorHofer commented 1 month ago

Projects in this repository hand pick the targeting pack references that come from the Microsoft.NETCore.App.Ref targeting pack which recently caused issues that required actions:

I would like to understand why wpf projects hand pick references. @jaredpar mentioned that there isn't really any benefit in doing. Libraries in runtime that are out-of-band did this as well in the past but we changed that a few years ago.

cc @ericstj

jaredpar commented 1 month ago

The core problem is that we don't put any effort into testing subsets of our reference sets. There isn't really any effort into verifying a particular feature works with a subset of references, instead we put the effort into making sure that features work with the .NET X reference set.

When you subset references you risk running into a number of problems:

  1. Using a non-transitively complete reference graph. This means that bug fixes in the compiler, or new features, could cause errors when moving to new versions of the compiler (example) These will always be resolved as "by design" and customers will be asked to have a transitively complete reference set.
  2. Getting errors when moving to new compiler features that require references that you don't have. New compiler features are tested against the net9.0 reference set, not subsets of the assemblies.

There is also no meaningful perf benefit to referencing less assemblies. The compiler aggressively caches references, particularly ones in the core .NET reference set.

ViktorHofer commented 1 month ago

@singhashish-wpf who is the right person on your team to chat about this?

singhashish-wpf commented 1 month ago

@rchauhan18 and I will look into this, Let us know if there is something which should be changed.

ViktorHofer commented 1 month ago

We basically want to understand why you hand pick references, i.e. in https://github.com/dotnet/wpf/blob/47efe4600a00ac059aeca109459e9a4e8132d907/src/Microsoft.DotNet.Wpf/src/PresentationUI/PresentationUI.csproj#L229-L257

... instead of just letting the TFM + FrameworkReference bring everything in automatically.

singhashish-wpf commented 1 month ago

We basically want to understand why you hand pick references, i.e. in

https://github.com/dotnet/wpf/blob/47efe4600a00ac059aeca109459e9a4e8132d907/src/Microsoft.DotNet.Wpf/src/PresentationUI/PresentationUI.csproj#L229-L257

... instead of just letting the TFM + FrameworkReference bring everything in automatically.

This has been the same since WPF was open sourced. So basically this has origins in .NET FX, same code.

jaredpar commented 1 month ago

@singhashish-wpf sure but why do we do this? As I mentioned above it's a recipe for hitting a number of problems because it's explicitly not tested. For WPF this turned into friction shipping .NET 9 features where as if WPF used the full reference set this wouldn't have happened. It's reasonable to expect that this will happen again in the future.

Is there a reason you all can't use the normal full reference set here?

singhashish-wpf commented 1 month ago

@jaredpar We are going run the experiment, to remove these references and use TFM + FrameworkReference thing, validate that first. Will update the thread with the findings.

ThomasGoulet73 commented 1 month ago

I did some snooping around to try to find if there was a reason for NetCoreReference.

I looked at the System.Xaml project because it was the first project open sourced in this repo and I saw that it hasn't always used NetCoreReference. It was added in dotnet/wpf#473, specifically this commit: 083719d2cc7e169148b94a148363584095bb0f6f which talks about "Integrate changes needed to use Microsoft.Dotnet.Arcade.Wpf.Sdk" so this might give some clue about the reasoning. @vatsan-madhavan who made the changes might have some context, AFAIK he isn't on the WPF team anymore but still works at Microsoft and answers questions in this repo from time to time when pinged.

I also saw this comment in the build files that might give some clues about the use of NetCoreReference: https://github.com/dotnet/wpf/blob/675f95f766b75c6a4f35d7d76c159320d138375c/eng/WpfArcadeSdk/tools/Pbt.targets#L29-L37

An explanation could be that it is to require explicit reference to new assemblies to avoid loading too many assemblies at runtime. WPF has a bad rep about slow startup and it employs a couple of "tricks" to reduce the need to load assemblies so this might be one of them. I don't know if those "tricks" are still needed or if this particular issue is one of those "tricks".

ViktorHofer commented 1 month ago

An explanation could be that it is to require explicit reference to new assemblies to avoid loading too many assemblies at runtime. WPF has a bad rep about slow startup and it employs a couple of "tricks" to reduce the need to load assemblies so this might be one of them. I don't know if those "tricks" are still needed or if this particular issue is one of those "tricks".

The number of (reference) assemblies that are passed to the compiler don't impact the number of assemblies that are loaded at run time. The runtime probes for and loads an assembly (with non-deterministic ordering) when it finds a type that needs to be resolved.

ThomasGoulet73 commented 1 month ago

An explanation could be that it is to require explicit reference to new assemblies to avoid loading too many assemblies at runtime. WPF has a bad rep about slow startup and it employs a couple of "tricks" to reduce the need to load assemblies so this might be one of them. I don't know if those "tricks" are still needed or if this particular issue is one of those "tricks".

The number of (reference) assemblies that are passed to the compiler don't impact the number of assemblies that are loaded at run time. The runtime probes for and loads an assembly (with non-deterministic ordering) when it finds a type that needs to be resolved.

That's true but what I meant was that with explicit references it means that you cannot use types from an assembly not explicitly referenced so it won't trigger the load of the assembly at runtime.