# Updating Rx.NET Build for .NET 7.0 era Tooling

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.

## Status

Draft.


## Authors

@idg10 ([Ian Griffiths](https://endjin.com/who-we-are/our-people/ian-griffiths/)).


## Context

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.

### Out-of-support TFMs

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 https://github.com/dotnet/reactive/issues/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](https://github.com/dotnet/reactive/issues/1683), and it has been [suggested](https://github.com/dotnet/reactive/discussions/1868#discussioncomment-5018844) 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.

### Windows SDK version

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.

### New analyzer diagnostics

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.

### UWP test runner issues

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](https://github.com/xunit/devices.xunit/issues/171). 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](https://github.com/idg10/reactive/commit/a6d39f6dabf7d6b342f552831c7413d54f3bc32b). 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: https://github.com/microsoft/testfx/issues/1607

This is unfortunate because it means that the default location Visual Studio chooses for cloning repositories will trigger this problem.

## Decision

The following sections describe how we will deal with each of the issues raised above.

### Out-of-support TFMs

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_0`
* `NETSTANDARD1_3`
* `NETCOREAPP1_0`
* `NETCOREAPP1_1`
* `NETCOREAPP2_1`
* `NETCOREAPP3_1`
* `WP8`
* `NO_CODE_COVERAGE_ATTRIBUTE`
* `NO_SERIALIZABLE`
* `CRIPPLED_REFLECTION`
* `NO_THREAD`
* `NO_TRACE`
* `NO_REMOTING` (availability of remoting is still issue, but this symbol is not used, because all relevant sections use `HAS_REMOTING` instead)

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.

### Windows SDK version

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 analyzer diagnostics

New diagnostic messages caused by more aggressive analyzers in the current SDK will be dealt with in two phases

1. to enable preview builds of the next version to be made available as early as possible, we will initially disable the new diagnostics
2. 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

### UWP test runner issues

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`).

## Consequences

These changes will have the following positive effects:

1. 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.sln` without errors, warnings or other diagnostic messages
2. automated builds will now be running tests on .NET 6.0 and .NET 7.0 (in addition to .NET Framework)
3. 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.0` targets
4. 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:

1. the repository must be cloned into a location where the path length is no longer than 25 characters
1. Visual Studio 2022 must be installed
1. .NET SDK 7.0 must be installed
1. Windows SDK 10.0.18362 must be installed