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

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

Closed
ViktorHofer opened this issue May 28, 2024 · 10 comments · Fixed by #9914
Closed

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

ViktorHofer opened this issue May 28, 2024 · 10 comments · Fixed by #9914
Labels
🚧 work in progress Investigate Requires further investigation by the WPF team.

Comments

@ViktorHofer
Copy link
Member

ViktorHofer commented May 28, 2024

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
Copy link
Member

jaredpar commented May 28, 2024

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
Copy link
Member Author

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

@singhashish-wpf
Copy link
Member

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

@singhashish-wpf singhashish-wpf added the Investigate Requires further investigation by the WPF team. label May 29, 2024
@ViktorHofer
Copy link
Member Author

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

<NetCoreReference Include="netstandard" />
<NetCoreReference Include="Microsoft.Win32.Registry" />
<NetCoreReference Include="System.Collections" />
<NetCoreReference Include="System.Collections.Generic" />
<NetCoreReference Include="System.Collections.NonGeneric" />
<NetCoreReference Include="System.ComponentModel" />
<NetCoreReference Include="System.ComponentModel.Primitives" />
<NetCoreReference Include="System.Diagnostics.Debug" />
<NetCoreReference Include="System.Diagnostics.Process" />
<NetCoreReference Include="System.Diagnostics.Tools" />
<NetCoreReference Include="System.Diagnostics.TraceSource" />
<NetCoreReference Include="System.Drawing.Common" />
<NetCoreReference Include="System.Drawing.Primitives" />
<NetCoreReference Include="System.IO" />
<NetCoreReference Include="System.IO.FileSystem" />
<NetCoreReference Include="System.IO.FileSystem.AccessControl" />
<NetCoreReference Include="System.Memory" />
<NetCoreReference Include="System.ObjectModel" />
<NetCoreReference Include="System.Resources.ResourceManager" />
<NetCoreReference Include="System.Runtime.Extensions" />
<NetCoreReference Include="System.Runtime.InteropServices" />
<NetCoreReference Include="System.Runtime.Loader" />
<NetCoreReference Include="System.Security.AccessControl" />
<NetCoreReference Include="System.Security.Cryptography" />
<NetCoreReference Include="System.Security.Cryptography.Algorithms" />
<NetCoreReference Include="System.Security.Cryptography.X509Certificates" />
<NetCoreReference Include="System.Threading" />
<NetCoreReference Include="System.Threading.Thread" />
<NetCoreReference Include="System.Threading.ThreadPool" />

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

@singhashish-wpf
Copy link
Member

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

<NetCoreReference Include="netstandard" />
<NetCoreReference Include="Microsoft.Win32.Registry" />
<NetCoreReference Include="System.Collections" />
<NetCoreReference Include="System.Collections.Generic" />
<NetCoreReference Include="System.Collections.NonGeneric" />
<NetCoreReference Include="System.ComponentModel" />
<NetCoreReference Include="System.ComponentModel.Primitives" />
<NetCoreReference Include="System.Diagnostics.Debug" />
<NetCoreReference Include="System.Diagnostics.Process" />
<NetCoreReference Include="System.Diagnostics.Tools" />
<NetCoreReference Include="System.Diagnostics.TraceSource" />
<NetCoreReference Include="System.Drawing.Common" />
<NetCoreReference Include="System.Drawing.Primitives" />
<NetCoreReference Include="System.IO" />
<NetCoreReference Include="System.IO.FileSystem" />
<NetCoreReference Include="System.IO.FileSystem.AccessControl" />
<NetCoreReference Include="System.Memory" />
<NetCoreReference Include="System.ObjectModel" />
<NetCoreReference Include="System.Resources.ResourceManager" />
<NetCoreReference Include="System.Runtime.Extensions" />
<NetCoreReference Include="System.Runtime.InteropServices" />
<NetCoreReference Include="System.Runtime.Loader" />
<NetCoreReference Include="System.Security.AccessControl" />
<NetCoreReference Include="System.Security.Cryptography" />
<NetCoreReference Include="System.Security.Cryptography.Algorithms" />
<NetCoreReference Include="System.Security.Cryptography.X509Certificates" />
<NetCoreReference Include="System.Threading" />
<NetCoreReference Include="System.Threading.Thread" />
<NetCoreReference Include="System.Threading.ThreadPool" />

... 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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor

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 #473, specifically this commit: 083719d 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:

<!--
Internal PBT compilation requires that we use <NetCoreReference>
This is so that the copy of ref\WindowsBase.dll inherited from Microsoft.NetCore.App
does not make its way through to markup-compilation.
In addition to this, our codebase requires that all references to Microsoft.NetCore.App
be explicitly enumerated through the use of <NetCoreReference> to avoid inadvertent additions
to assembly references during code-changes.
-->

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
Copy link
Member Author

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
Copy link
Contributor

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.

JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this issue Oct 8, 2024
This removes the `NetCoreReference` infrastructure and replaces it with `DefaultReferenceExclusion` to remove the one problematic implicit reference to WindowsBase.

Manually picking references was blocking using System.Private.Windows.Core from the WinForms repo. Not having this also greatly simplifies the projects.

This also tweaks the solution to add folders docs and eng items.

Fixes dotnet#9168
JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this issue Oct 8, 2024
This removes the `NetCoreReference` infrastructure and replaces it with `DefaultReferenceExclusion` to remove the one problematic implicit reference to WindowsBase.

Manually picking references was blocking using System.Private.Windows.Core from the WinForms repo. Not having this also greatly simplifies the projects.

This also tweaks the solution to add folders docs and eng items.

Fixes dotnet#9168
JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this issue Oct 31, 2024
This removes the `NetCoreReference` infrastructure and replaces it with `DefaultReferenceExclusion` to remove the one problematic implicit reference to WindowsBase.

Manually picking references was blocking using System.Private.Windows.Core from the WinForms repo. Not having this also greatly simplifies the projects.

This also tweaks the solution to add folders docs and eng items.

Fixes dotnet#9168
JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this issue Nov 1, 2024
This removes the `NetCoreReference` infrastructure and replaces it with `DefaultReferenceExclusion` to remove the one problematic implicit reference to WindowsBase.

Manually picking references was blocking using System.Private.Windows.Core from the WinForms repo. Not having this also greatly simplifies the projects.

This also tweaks the solution to add folders docs and eng items.

Fixes dotnet#9168
JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this issue Nov 4, 2024
This removes the `NetCoreReference` infrastructure and replaces it with `DefaultReferenceExclusion` to remove the one problematic implicit reference to WindowsBase.

Manually picking references was blocking using System.Private.Windows.Core from the WinForms repo. Not having this also greatly simplifies the projects.

This also tweaks the solution to add folders docs and eng items.

Fixes dotnet#9168
JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this issue Nov 6, 2024
This removes the `NetCoreReference` infrastructure and replaces it with `DefaultReferenceExclusion` to remove the one problematic implicit reference to WindowsBase.

Manually picking references was blocking using System.Private.Windows.Core from the WinForms repo. Not having this also greatly simplifies the projects.

This also tweaks the solution to add folders docs and eng items.

Fixes dotnet#9168
JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this issue Nov 10, 2024
This removes the `NetCoreReference` infrastructure and replaces it with `DefaultReferenceExclusion` to remove the one problematic implicit reference to WindowsBase.

Manually picking references was blocking using System.Private.Windows.Core from the WinForms repo. Not having this also greatly simplifies the projects.

This also tweaks the solution to add folders docs and eng items.

Fixes dotnet#9168
JeremyKuhne added a commit to JeremyKuhne/wpf that referenced this issue Nov 11, 2024
This removes the `NetCoreReference` infrastructure and replaces it with `DefaultReferenceExclusion` to remove the one problematic implicit reference to WindowsBase.

Manually picking references was blocking using System.Private.Windows.Core from the WinForms repo. Not having this also greatly simplifies the projects.

This also tweaks the solution to add folders docs and eng items.

Fixes dotnet#9168
Kuldeep-MS pushed a commit that referenced this issue Nov 11, 2024
* Remove NetCoreReference

This removes the `NetCoreReference` infrastructure and replaces it with `DefaultReferenceExclusion` to remove the one problematic implicit reference to WindowsBase.

Manually picking references was blocking using System.Private.Windows.Core from the WinForms repo. Not having this also greatly simplifies the projects.

This also tweaks the solution to add folders docs and eng items.

Fixes #9168

* Put back test projects in solution

* Enable xunit testing in VS with the dispatcher

This change enables writing and debugging unit tests in the Test Explorer in Visual Studio. I added a new test project for WindowsBase that has a unit test that launches the SplashScreen as an example.

In order to make this happen I had to reference the xunit.stafact package which introduced a number of challenges to overcome. The package has a reference to the desktop sdk and needs to be used in a project that targets `net10.0-windows`, not `net10.0`. To make everything work seamlessly I had to:

- Strip the platform reference
- Apply actual versions from the sdk
- Get rid of AnyCPU and explictly use x86 (everything has a native dependency)
- Enable transitive copying of references for the unit test projects
- Target the 10.0 SDK (would get version confusion otherwise)
- Add an override setting for WinForms which still targets the 9.0 SDK
- Remove other targets that were trying to fixup WindowsBase references from the .NET SDK
-

* Make PresentationBuildTasks always AnyCPU

* Update versions and tweak / simplify some version related stuff

* Attempt to turn on the binlog for the CI

* Update binlog names for both build and test run

* Try 17.12 preview

* Move back off of scout image

* Update versions

* Push back to 9.0

* Uncheck D3DCompiler in ConfigurationManager

* Have ref projects only reference ref projects and move ref and cycle breaker projects to AnyCPU.

* Normalize ARM64 to arm64. Having multiple casings was causing projects to be built multiple times to the same folder.

* Revert RuntimeFrameworkReference.targets changes

* Final cleanup pass

* Tweak SdkReferences.targets

* Respond to feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚧 work in progress Investigate Requires further investigation by the WPF team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants