At the time of writing this (early 2023), if you download recent versions of .NET tools (Visual Studio 2022, the .NET 7 SDK, and any Windows SDK supported in Visual Studio 2022) it is not possible to get a warning-free build of Rx.NET. This ADR proposes a set of changes to fix this.
Draft.
@idg10 (Ian Griffiths).
There are four main problems preventing Rx.NET building on current tooling:
- Out-of-support TFMs
- Use of a Windows SDK version not supported by Visual Studio 2022
- New analyzer diagnostics
- The UWP test runner project (
Tests.System.Reactive.Uwp.DeviceRunner.csproj) fails to build due to an incompatibility with Coverlet, and once that is fixed, xUnit seems unable to run tests
The following sections describe each are in more detail.
The Target Framework Monikers (TFMs) in the following table are all out of support. The table shows which projects use them.
| Out-of-support TFM | Projects |
|---|---|
netcoreapp2.1 |
Tests.System.Reactive |
netcoreapp3.1 |
Microsoft.Reactive.Testing, System.Reactive, Tests.System.Reactive |
net5.0 |
Microsoft.Reactive.Testing, System.Reactive, System.Reactive.Observable.Aliases |
net5.0-windows10.0.19041 |
System.Reactive |
There is also a more subtle problem: the netstandard2.0 TFM is of course still very much supported, but in its current form, the System.Reactive DLL built for that target won't run correctly on all .NET runtimes that support netstandard2.0. This is because it has a dependency on System.Runtime.InteropServices.WindowsRuntime. The netstandard2.0 TFM is nominally compatible with .net6.0-windows style TFMs, but that library won't work on that runtime because it was written for the old WinRT interop mechanism in which the CLR handled .winmd files directly, whereas .NET 6.0 uses the newer C#/WinRT mechanism to interoperate with WinRT APIs.
In theory that shouldn't be a problem because applications using .NET 6.0 or later should end up with either the net6.0 or net6.0-windows10.0.19041 version of System.Reactive. However, it's conceivable that certain plug-in scenarios could end up binding prematurely netstandard2.0. (The old dotnet#97 problem was caused by a similar situation.) It would be better if the netstandard2.0 build did actually work if it happened to be loaded into a net6.0 runtime. (Also, one possible future direction will be for System.Reactive to support .NET 6 and .NET 7 via the netstandard2.0 target, in which case it would become absolutely necessary to remove this dependency.)
We have been asked to enable trimming, and it has been suggested that on .NET 6.0 this is a just a matter of setting the IsTrimmable build variable. If this is true, then since moving to in-support TFMs necessarily means targeting net6.0 we should see if this does work.
Rx.NET currently uses two Windows SDK versions. The UWP targets all use a TFM of uap10.0.16299, which corresponds to a Windows SDK version of 10.0.16299, while the various Windows-specific .NET 5.0 targets are all on net5.0-windows10.0.19041, which corresponds to a Windows SDK version of 10.0.19041.
Visual Studio 2022 does not support the use of Windows SDK 10.0.16299 (which was introduced in 2017). The earliest SDK version it supports is Windows SDK 10.0.18362, which is a fairly old SDK—the last version of Windows that required anything that old shipped in May 2019 and went out of support in November 2020. Applications targeting 10.0.20348 will work with all versions of Windows 10 and 11 still in support.
Each new .NET SDK release provides a more extensive set of code quality and style analyzers. One effect of this is that when a project upgrades to a new SDK, it immediately produces a large number of new warnings and messages. Rx.NET is no exception.
The Coverlet library does not seem to work with UWP test projects in Visual Studio 2022. Visual Studio refuses to load the solution, reporting this error:
C:\repos\reactive\Rx.NET\Source\tests\Tests.System.Reactive.Uwp.DeviceRunner\Tests.System.Reactive.Uwp.DeviceRunner.csproj : error : The expression "[System.Version]::Parse('')" cannot be evaluated. Version string portion was too short or too long. C:\Users\fquimby\.nuget\packages\coverlet.collector\3.1.1\build\netstandard1.0\coverlet.collector.targets
Once this has been worked around, a new problem becomes apparent: the xUnit test runner does not seem to work on Visual Studio 2022. Although it successfully discovers tests and reports them in Visual Studio's Test Explorer, it is unable to execute them. The https://github.com/xunit/devices.xunit project has had no meaningful updates in years. (Its logo was updated in July 2021, but the last release was in January 2019.) Updated support does not look to be immediately forthcoming.
We have done an experimental spike to demonstrate that moving to the MSTest framework is relatively straightforward. A handful of tests fail because they turned out to be dependent on xUnit supplying a non-null SynchronizationContext. These tests would need to be examined—it is possible the failures represent a real problem that needs addressing, but if not we either need to modify the tests so that their expectations are in line with the absence of a SynchronizationContext, or to arrange for a SynchronizationContext to be set up when those tests run (or possibly do both, so that the relevant tests run both with and without a SynchronizationContext).
There's one frustrating issue with this change to MSTest: it results in some very long path lengths during builds. If you clone the repository into a folder whose path is longer than 26 characters (e.g., C:\Users\jdoe\source\repos\reactive) you will run into this problem: microsoft/testfx#1607
This is unfortunate because it means that the default location Visual Studio chooses for cloning repositories will trigger this problem.
The following sections describe how we will deal with each of the issues raised above.
In all projects that used to target net5.0, change this to net6.0. Likewise, in any projects that target net5.0-windows10.0.19041, change that to net6.0-windows10.0.19041.
Modify the entries in Directory.build.targets
Remove the entries in Directory.build.targets that refer to the out-of-support TFMs. This means that several preprocessor constants are no longer used. We should scour the codebase and remove all conditionally compiled sections of code that will no longer be used because the target frameworks that used to bring them in no longer exist. The constants no longer in use are:
NETSTANDARD1_0NETSTANDARD1_3NETCOREAPP1_0NETCOREAPP1_1NETCOREAPP2_1NETCOREAPP3_1WP8NO_CODE_COVERAGE_ATTRIBUTENO_SERIALIZABLECRIPPLED_REFLECTIONNO_THREADNO_TRACENO_REMOTING(availability of remoting is still issue, but this symbol is not used, because all relevant sections useHAS_REMOTINGinstead)
Find all Condition attributes across all project files that refer to TFMs no longer in use, and remove the relevant TFMs. Elements which would only come into play for TFMs no longer in use should be removed entirely.
In all test projects that used to target net5.0, in addition to changing this to net6.0 also add net7.0. Likewise, for test projects that used to target net5.0-windows10.0.19041, in addition to changing that to net6.0-windows10.0.19041, also add net7.0-windows10.0.19041.
Remove Rx.NET/Source/src/System.Reactive/build/System.Reactive.targets since it is concerned only with .NET Core 3.1
Update the build pipelines to install the .NET 7 SDK and .NET 6 runtime; remove the tasks that installed .NET Core 2.2 and 3.1. Modify the steps that currently run tests on .NET Core 2.1, .NET Core 3.1 and .NET 5.0 to run them on .NET 6.0 and .NET 7.0.
Replace the code that depends on System.Runtime.InteropServices.WindowsRuntime with code that achieves the same effect without it. (This can be done by replacing direct comparisons with typeof(EventRegistrationToken) with code that checks for the relevant type by inspecting the type name; it is possible to write code that works on both the old and new WinRT interop mechanisms.) The code in EventRegistrationToken currently made conditional on HAS_WINRT can then be made unconditional.
In the net6.0 and net6.0-windows10.0.19041 targets, we will try setting <IsTrimmable>true</IsTrimmable>. We will need to verify that this has the intended effect before doing a non-preview release.
In any projects that target uap10.0.16299, change that to uap10.0.18362. (The goal with this particular set of changes is to get everything building on modern tooling while changing as little as possible. This is the smallest version change that meets those goals, and that's why we're not updating to a more recent version.)
Change the Tests.System.Reactive.Uwp.DeviceRunner test runner project to target SDK 10.0.18362.
New diagnostic messages caused by more aggressive analyzers in the current SDK will be dealt with in two phases
- to enable preview builds of the next version to be made available as early as possible, we will initially disable the new diagnostics
- once preview builds have been released, we will update the codebase in cases where the new warnings make suggestions we want to take, and add suitable configuration to prevent warnings suggesting changes we do not want
It Coverlet-related build problems can be dealt with by setting the NETCoreSdkVersion build variable in the UWP test project.
As for the lack of current xUnit support, it looks like the only realistic option at this point is not to use xUnit. An experimental spike has been done to demonstrate that moving to the MSTest framework is relatively straightforward. A handful of tests fail because they turned out to be dependent on xUnit supplying a non-null SynchronizationContext. These tests would need to be examined—it is possible the failures represent a real problem that needs addressing, but if not we either need to modify the tests so that their expectations are in line with the absence of a SynchronizationContext, or to arrange for a SynchronizationContext to be set up when those tests run (or possibly do both, so that the relevant tests run both with and without a SynchronizationContext).
These changes will have the following positive effects:
- it will be possible to download the repository and, using current tooling (VS 2022 and .NET 7.0 SDK), open, build, and test
System.Reactive.slnwithout errors, warnings or other diagnostic messages - automated builds will now be running tests on .NET 6.0 and .NET 7.0 (in addition to .NET Framework)
- when we release components based on these changes to NuGet, they will no longer target frameworks that are out of support, and will include
net6.0targets - if setting
<IsTrimmable>true</IsTrimmable>turns out to be all that is required for trimmability, this will address that requirement
It will impose the following new requirements for anyone wanting to build the solution:
- the repository must be cloned into a location where the path length is no longer than 25 characters
- Visual Studio 2022 must be installed
- .NET SDK 7.0 must be installed
- Windows SDK 10.0.18362 must be installed