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

Revisit .NET targets #2251

Closed
dennisdoomen opened this issue Aug 13, 2023 · 26 comments · Fixed by #2302
Closed

Revisit .NET targets #2251

dennisdoomen opened this issue Aug 13, 2023 · 26 comments · Fixed by #2302
Assignees
Milestone

Comments

@dennisdoomen
Copy link
Member

No description provided.

@dennisdoomen dennisdoomen changed the title Revisit .NET target Revisit .NET targets Aug 13, 2023
@dennisdoomen
Copy link
Member Author

Started a thread on Twitter to get some feedback.

@dennisdoomen dennisdoomen added this to the 7.0 milestone Aug 13, 2023
@Corniel
Copy link
Contributor

Corniel commented Aug 15, 2023

I would argue that you can drop .NET 4.7, as .NET standard 2.0 covers all their is to cover (at least to my knowledge). And .NET 8.0 should probably be added?!

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Aug 15, 2023

Maybe remove .NETCoreApp3.0 and 3.1, because of EOL?

@lg2de
Copy link
Contributor

lg2de commented Aug 15, 2023

I would argue that you can drop .NET 4.7, as .NET standard 2.0 covers all their is to cover (at least to my knowledge).

Currently there are FA options (e.g. Event Monitoring) which is not available in the netstandard target. So, net47 should not be removed.

net47 has currently no support end date. So, it should be kept, I guess.

And .NET 8.0 should probably be added?!

I think, net8 should be added as target for library when using new features from net8. There is no need to add it otherwise.
It should be added to the tests as soon as possible to check the compatibility.

@dennisdoomen
Copy link
Member Author

net47 has currently no support end date. So, it should be kept, I guess.

Correct. Or at least 4.8, depending on the support lifecycle

I think, net8 should be added as target for library when using new features from net8. There is no need to add it otherwise.
It should be added to the tests as soon as possible to check the compatibility.

That is also correct.

@jnyrup
Copy link
Member

jnyrup commented Aug 16, 2023

How about we target this from a feature perspective instead?

What new features do we want that requires new TFMs?

  • Can these features be enabled only for newer TFMs? (Think DateOnly/TimeOnly, BufferedStream, EventMonitor, etc)

Is there any existing TFMs that cause unbearable maintenance?

  • E.g. do we feel restrained by missing BCL APIs that we cannot easily polyfill?

I think we should bump net47 to at least net472.

I have regularly tested FA on .NET 8 previews.
The two blockers I recall were BinaryFormatter (#1779) and SerializationFormat.Binary (#2002).

@dennisdoomen
Copy link
Member Author

dennisdoomen commented Sep 2, 2023

.NET Framework 4.7 is still supported, so that brings the final list to:

  • .NET Framework 4.7
  • .NET Standard 2.0
  • .NET 6

And if we need to support anything specific for .NET 7 or 8, we'll include those as well. But not before that.

Right @jnyrup ?

@jnyrup
Copy link
Member

jnyrup commented Sep 3, 2023

I'll refer to my comment above.
What do we want for Fluent Assertions?

Do we want to:

  • have the latest and greatest language/BCL features available and no need to polyfill older frameworks,
  • support the most frameworks
  • something in between

Some notes:
.netstandard2.0 is needed if we want to support UWP https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0

net462 is also still supported by MS, but they have this quote on https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0

2 The versions listed here represent the rules that NuGet uses to determine whether a given .NET Standard library is applicable. While NuGet considers .NET Framework 4.6.1 as supporting .NET Standard 1.5 through 2.0, there are several issues with consuming .NET Standard libraries that were built for those versions from .NET Framework 4.6.1 projects. For .NET Framework projects that need to use such libraries, we recommend that you upgrade the project to target .NET Framework 4.7.2 or higher.

@dennisdoomen
Copy link
Member Author

dennisdoomen commented Sep 3, 2023

Although NET 4.6.2 is still supported, we didn't support it explicitly. And I'm fine with .NET Standard 2.0. We don't need to support .NET 7 or 8 just yet, but can if we need to. What else is there to discuss?

@Corniel
Copy link
Contributor

Corniel commented Sep 4, 2023

We can consider to move the .NET framework related assertions in a separate (legacy) package. This is only beneficial when the the .NET framework related assertions are not used that much (anymore). Do we have any insights in the usage?

@dennisdoomen
Copy link
Member Author

We can consider to move the .NET framework related assertions in a separate (legacy) package. This is only beneficial when the the .NET framework related assertions are not used that much (anymore). Do we have any insights in the usage?

I see no issues to keep supporting .NET 4.7. And there's not much that is 4.7-specific?

@Corniel
Copy link
Contributor

Corniel commented Sep 4, 2023

I see no issues to keep supporting .NET 4.7. And there's not much that is 4.7-specific?

@dennisdoomen I have no issues with supporting that either. But it would simplify the build and reduces the the amount of targets. (.NET 4.7 is already 6,5 years old)

@jnyrup
Copy link
Member

jnyrup commented Sep 9, 2023

I tried trimming the TFMs to <TargetFrameworks>net47;net6.0;netstandard2.0</TargetFrameworks> and not touching our test suite to see what the implications would be.

The only thing I noticed was that EventAssertionSpecs and BufferedStreamAssertionSpecs no longer works for netcoreapp21 and netcoreapp31 and I'm fine with that.

I can't find a link right now, but I recall around the release of v6 we discussed to targeting net472 instead of net47 because it solved some problems when multi-targeting net47+netstandard20 and packages.config.
Probably related to #2122
But since we haven't heard about other problems with net47 I'm fine not bumping it.
Or am I confusing it with net46/net461 being the problematic ones and upgrading to net462 being the solution?

We added netstandard21 in #1158 to support ValueTask.
By removing that again I think would cause Mono, Xamarin and Unity no longer being able to use ValueTask.
So we might not want to remove netstandard21.

@jnyrup
Copy link
Member

jnyrup commented Sep 10, 2023

As a note: if we keep netstandard2.0 keeping netcoreapp31 (and netcoreapp21) and shouldn't put any burden on us as they are supersets of netstandard20.
But again, I'm fine dropping support for Event and BufferedStream to trim off those two targets.

@dennisdoomen
Copy link
Member Author

Or am I confusing it with net46/net461 being the problematic ones and upgrading to net462 being the solution?

I think it was something with 4.6 indeed. Can't be bother to look it up though.

As a note: if we keep netstandard2.0 keeping netcoreapp31 (and netcoreapp21) and shouldn't put any burden on us as they are supersets of netstandard20.

Are you suggesting keeping support for .NET Core?

So we might not want to remove netstandard21.

We should not remove it.

So that means <TargetFrameworks>net47;net6.0;netstandard2.1</TargetFrameworks>

@jnyrup
Copy link
Member

jnyrup commented Sep 11, 2023

As a note: if we keep netstandard2.0 keeping netcoreapp31 (and netcoreapp21) and shouldn't put any burden on us as they are supersets of netstandard20.

Are you suggesting keeping support for .NET Core?

Nah... I'm mostly just trying to add some extra perspectives besides looking at currently supported frameworks by MS.
Sentry and
Datadog both use FA and targets netcoreapp31 and netcoreapp21, respectively.

So we might not want to remove netstandard21.

We should not remove it.

So that means <TargetFrameworks>net47;net6.0;netstandard2.1</TargetFrameworks>

Given we don't want to shrink the API surface for frameworks supported by MS.
<TargetFrameworks>net47;net6.0;netstandard2.0;netstandard2.1</TargetFrameworks>

netstandard2.0 because of UWP (this will also include net462)
netstandard2.1 because of Mono, Xamarin and Unity

@dennisdoomen
Copy link
Member Author

Given we don't want to shrink the API surface for frameworks supported by MS.
net47;net6.0;netstandard2.0;netstandard2.1

netstandard2.0 because of UWP (this will also include net462)
netstandard2.1 because of Mono, Xamarin and Unity

Agreed.

@dennisdoomen dennisdoomen self-assigned this Sep 17, 2023
@dennisdoomen
Copy link
Member Author

So apparently we were not really testing .NET Standard, except for a single UWP test.

@jnyrup
Copy link
Member

jnyrup commented Sep 17, 2023

So apparently we were not really testing .NET Standard, except for a single UWP test.

We're testing the netstandard20 through the netcoreapp20 target.

@dennisdoomen
Copy link
Member Author

Yes, and that one is not supported anymore. You're suggesting to keep doing that and ignore the dotnet warnings?

@jnyrup
Copy link
Member

jnyrup commented Sep 17, 2023

I'm fine with whatever test target that consumes the netstandard20 and netstandard21 TFMs of FA.
netcoreapp20 and netcoreapp31 just seems easier than UWP and Mono/Xamarin/Unity.

@dennisdoomen
Copy link
Member Author

dennisdoomen commented Sep 17, 2023

Then you're essentially testing against the .NET Core 3.1 runtime. If we really want to ensure our .NET Standard 2.0 library works as expected, we would need to target UWP, Mono, Xamarin or Unity, which I don't want either. I would be fine only testing .NET 4.7 and 6.0. Alternatively, we can add all the tests to the UWP.Specs project and accept that this only gets tested on GHA

@jnyrup
Copy link
Member

jnyrup commented Sep 17, 2023

One cannot test netstandard as it's not an implementation, you'll have to run test on a runtime which implements that specification.

Of course different runtimes can have different implementations of netstandard, but shouldn't we at least test the all TFMS we compile to against some runtime?

@dennisdoomen
Copy link
Member Author

dennisdoomen commented Sep 17, 2023

The compiler will help us protect against using members that aren't available, but we know that certain runtimes pretend to implement .NET Standard, but then throw at runtime. To really catch those cases, you'll need to target the real runtimes. As .NET Standard is being deprecated in the community, But I'm willing to bring that risk to the folks that need .NET Standard.

If we don't want that, then we need to either use a deprecated runtime in our tests or start properly supporting all these other frameworks. I don't want to put that burden on ourselves and our contributors.

@dennisdoomen
Copy link
Member Author

To conclude, we have a couple of options:

  1. Keep using .NET Core to test .NET Standard. My biggest concern is that we're using a deprecated framework and it doesn't help prove that target works for Unity, UWP and such.
  2. Don't test .NET Standard. The biggest risk is that we use something that compiles but doesn't work in Unity, UWP and such. But I'm confident we'll get bug reports if that really happens.
  3. We fully embrace Unity, UWP and such. The big concern is that it requires quite some infrastructure, not just for us, but also for our contributors. I'm convinced it will slow down contributions.

Since 1 doesn't really solve the real problem and 3 is going to be very costly, I vote for 2.

@jnyrup
Copy link
Member

jnyrup commented Sep 17, 2023

I have no concerns also testing with deprecated frameworks, they can only add confidence not take it away.
I had a look at the #ifs and and the CC report from #2302 and all paths should be covered by either net47 or net6.
So let's go with 2, learn and adapt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

5 participants