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

Support for generic -windows targets in ReactiveUI.WPF (#3558) #3559

Closed
wants to merge 1 commit into from

Conversation

the-black-wolf
Copy link

@the-black-wolf the-black-wolf commented Jun 21, 2023

What kind of change does this PR introduce?

Feature: Adding support for generic -windows targets

What is the current behavior?

WPF applications require an OS version specific target. This forces the hand of develoeprs and 3rd party library makers.

#3557
#3558

What is the new behavior?

WPF application targeting net6.0-windows and net7.0-windows can use ReactiveUI.WPF. Also 3rd party libraries for WPF using ReactiveUI.WPF internally can expose the target frameworks.

What might this PR break?

Nothing really, it just adds more targets, existing targets are unaffected.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

The integration tests do not contain any dotnet builds as it is, so nothing to expand upon, we can upgrade those tests if you like. I did test the fix in my own WPF app.

Other information:

Copy link
Member

@ChrisPulman ChrisPulman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Target Frameworks supported by ReativeUI are starting from net6.0-windows10.0.17763, we have base libraries that use windows10.0.19041 such as System.Reative, we have created some patches that allow us to go back to windows10.0.17763, this target has been there for some considerable time in these base libraries and actually caused quite a few issues when using net5.0-windows as an example where it failed over to netstandard code instead of the required target leading to things not working quite as expected but also unknown at compile time.

Recent versions of Visual Studio 2022 have been far better at giving notifications and telling us about these underlying issues, but it would have been pure luck if people managed to run an application with a netx.x-windows target and not have cross threading issues for example.

You are more than welcome to target a lower Target version in your own solutions such as net6.0-windows which is actually net6.0-windows7.0 (Windows 7) however you will import either netstandard or net462 versions of the ReactiveUI libray and sub libraries which may not be 100% compatible with your application/ library or tool, you will then need to create the relevant patches required to make it work as expected.

Unfortunately we cannot keep targeting windows versions that are no longer supported or we will not be able to develop ReactiveUI or the related set of tools that we have.

@the-black-wolf
Copy link
Author

the-black-wolf commented Jun 22, 2023

@ChrisPulman thanks for the reply. So let me address your concerns.

Naturally, you cannot target windows10 version target from a generic moniker, that is he core of the problem, it binds the net462 version which of course does not work from the dotnet context. Hence the PR.
However, there are no more issues with net6.0 and forward, whatever was blocking you (and them) on net5.0-windows does not appear to be there any more. If you look at the WPF project, and what you did for the 17763 version, you effectively copied over the missing class and built the observable features on it. The same trick applies to generic moniker. This issue is not caused by you, or the OS, its caused by a faulty HAS_WPF condition in System.Reactive.

So what happens after this PR, when you bind to the ReactiveUI.WPF from a net6.0-windows (or net7.0-windows)?

  1. It loads the ReactiveUI.WPF, finds a net6.0-windows and binds to it
  2. This package is dependent on System.Reactive, it loads it and binds to net6.0 version, in the absence of net6.0-windows and failing to match net6.0-windows10.0.19041.0.
  3. System.Reactive binds to its own net6.0 dependencies

It does not fall back to netstandard, so there is nothing missing, except the HAS_WPF code, which you compensated for in 17763 workaround. its really that simple, if -windows does not work, neither does 17763, and they both work. They both use the same code from System.Reactive (and all the threading stuff from net6.0/7.0 itself) and your stand-in class. None of that is Windows 10.19041 dependent.

I have tested this quite a lot since implementing the fix, I have not had a single issue with it. Nothing really changes for your users who already have 17763 or 19041 targets, they will still use what they use today. If you want, I can work on tests for WPF for scenarios you think are problematic. If you know the issues reported for net5.0-windows, we can test them directly on this build.

As for supporting deprecated OS comment, this is not really about that (although it is a valid concern, especially for maintaining closed systems with difficult upgrade process, such as ATMs et al, some of which are still running even on XP, examples are many, especially in embedded and industrial sector). Real life is not really what Microsoft Windows sales imagines it to be. As you know WPF is not UWP, it is not windows version dependent. Even Microsofts own WPF libraries (and all the 3rd party libraries we worked with except ReactiveUI) have the generic -windows targets. Even you yourself have a mixed support for windows 10 (you lack 14393, which is still live in LTSC, and if you only want to support GAC, then you should scrap everything and only support 19045). I understand, its a nightmare caused by MS and their update policies. The bizzare thing being that 19041 build of ReactiveUI.WPF app will work just fine even on 10240, most probably even on Windows 7. Which is why having a -windows bypasses all of that. You should not care what OS is supported, unless you specifically use the OS features of that version (which again only matters for UWP). You should only care what tooling is supported (so net6.0 and net7.0 at the moment).

BTW, unrelated to this, your tests are failing on the WinForms tests.

Thanks for your time.

@reactiveui reactiveui locked and limited conversation to collaborators Jun 22, 2023
@glennawatson
Copy link
Contributor

Please stop opening the same issue over and over again. You been given your resolution go to dotnet/reactive and ask them to remove the constraint.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants