Ensure target frameworks make sense for 7.0 #1189

lukebakken · 2022-04-05T21:43:25Z

Update packages to latest versions

michaelklishin · 2022-04-06T13:21:37Z

@danielmarbach @stebet @bollhals @bording can we really move this fast and require .NET 6.0 for RabbitMQ .NET client 7.0? It seems to be very new.

projects/RabbitMQ.Client/RabbitMQ.Client.csproj

lukebakken · 2022-04-06T15:13:18Z

Thanks @danielmarbach. This is the statement that lead me to believe net6.0 would be the right target:

.NET 6 is currently the primary implementation, and the one that's the focus of ongoing development.

https://docs.microsoft.com/en-us/dotnet/fundamentals/implementations

danielmarbach · 2022-04-06T16:01:24Z

How many of your users would actually depend on the red marked targets?

Do you know that?

projects/Benchmarks/Benchmarks.csproj

projects/Unit/Unit.csproj

projects/RabbitMQ.Client/RabbitMQ.Client.csproj

.github/workflows/main.yaml

lukebakken · 2022-04-06T16:46:05Z

How many of your users would actually depend on the red marked targets?

AFAIK we don't have that information - @michaelklishin

However, if we target netstandard2.0 that should suffice, correct?

If you haven't guessed already it has been a long time since I've had to think about framework targets. I appreciate your patience @danielmarbach

projects/RabbitMQ.Client/RabbitMQ.Client.csproj

Update packages to latest versions Fixes #1172

bollhals · 2022-04-06T20:39:44Z

https://docs.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0#net-5-and-net-standard

Reusable libraries

If you're building reusable libraries that you plan to ship on NuGet, consider the trade-off between reach and available feature set. .NET Standard 2.0 is the latest version that's supported by .NET Framework, so it gives good reach with a fairly large feature set. We don't recommend targeting .NET Standard 1.x, as you'd limit the available feature set for a minimal increase in reach.

If you don't need to support .NET Framework, you could go with .NET Standard 2.1 or .NET 5/6. We recommend you skip .NET Standard 2.1 and go straight to .NET 6. Most widely used libraries will multi-target for both .NET Standard 2.0 and .NET 5+. Supporting .NET Standard 2.0 gives you the most reach, while supporting .NET 5+ ensures you can leverage the latest platform features for customers that are already on .NET 5+.

So I'd probably do .NET Standard 2.0 and then .NET Core 3.1 and .NET 6. Why?
=> Standard for the masses, covers most relevant cases
=> 3.1 as it was used in the past and it hasn't reached end of life quite yet.
=> 6.0 as it is the latest LTS release of .NET

danielmarbach · 2022-04-06T20:47:09Z

@bollhals supporting netcoreapp3.1 doesn't make sense. Even if you'd release tomorrow you'd soon have to release another major because you'd be dropping netcoreapp3.1 support when it is out of support. The customers that are on the netcoreapp3.1 have moved or are in the process of moving to Net6 since that is how net works these days since it is no longer coupled to windows updates

danielmarbach · 2022-04-06T20:48:56Z

@bording thoughts? I know you have a lot of insights into this whole TFM horror show 👻😂

lukebakken · 2022-04-06T20:55:47Z

Thanks again for everyone's input. I'm going to review what other .NET client library projects are doing, like npgsql.

bording · 2022-04-06T21:31:11Z

@bording thoughts? I know you have a lot of insights into this whole TFM horror show 👻😂

Targeting net6.0 only is a noble goal, but likely too aggressive because there are plenty of people that haven't moved to .NET Core/.NET yet, so they'd be completely unable to use the library.

What is the support policy for older major versions of the library? If 6.0 is going to be supported for some amount of time after 7.0 is out, then maybe net6.0 for 7.0 could be okay.

Otherwise, if you want to support "older" frameworks in some capacity, then you should definitely be multi-targeting in some way.

The main question to answer if you care about supporting the more obscure platforms that work with .NET Standard 2.0 or not.

If you do, then going with netstandard2.0;net6.0 makes sense. If not, I'd go with net472;net6.0.

While it is true that .NET Core 3.1 is still supported right now, it goes out of support in December, so I don't think it makes sense to keep a 3.1 target in the new major version.

michaelklishin · 2022-04-06T23:46:42Z

I suggest that we ship another 6.x release and reduce the number of .NET platforms we support.

stebet · 2022-04-08T10:11:35Z

If 7.0 gives an opportunity to move completely over to just supporting the latest LTS of .NET Core, my vote would be to go for it, but that kind of depends on if there are big features coming up that people expect to be available on older frameworks.

@bording Is there a reason you'd want to target .NET Standard 2.0 instead of 2.1, since I think .NET Standard 2.0 for .NET Framework projects tends to not always work correctly For .NET Framework.

lukebakken · 2022-04-08T15:53:04Z

The main question to answer if you care about supporting the more obscure platforms that work with .NET Standard 2.0 or not. If you do, then going with netstandard2.0;net6.0 makes sense. If not, I'd go with net472;net6.0.

Thanks @bording. Yes we do have people using this client on Xamarin.

I'm still a bit mystified about why you'd include net6.0 if we can't really use any net6.0 specific features without ifdefs, which sounds like a maintenance headache.

Obviously there's a reason I don't understand, because check out these targets - https://github.com/npgsql/npgsql/blob/main/src/Npgsql/Npgsql.csproj

There is no special-casing in the code, either 🤔

stebet · 2022-04-08T15:59:10Z

I'm still a bit mystified about why you'd include net6.0 if we can't really use any net6.0 specific features without ifdefs, which sounds like a maintenance headache.

Obviously there's a reason I don't understand, because check out these targets - https://github.com/npgsql/npgsql/blob/main/src/Npgsql/Npgsql.csproj

The API difference between .NET Standard 2.1 and .NET 6.0 is not a lot, but there might be a difference in the supported C# language version, although I'm not 100% on that.

The main benefit in targeting .NET 6.0 is there are high-performance/low-allocation APIs available there that aren't available o previous versions, but those can be easily ifdef without a lot of noise. There shouldn't be huge code blocks of those

Taken from: https://docs.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-1#when-to-target-net50-or-net60-vs-netstandard

If you're building reusable libraries that you plan to ship on NuGet, consider the trade-off between reach and available feature set. .NET Standard 2.0 is the latest version that's supported by .NET Framework, so it gives good reach with a fairly large feature set. We don't recommend targeting .NET Standard 1.x, as you'd limit the available feature set for a minimal increase in reach.

If you don't need to support .NET Framework, you could go with .NET Standard 2.1 or .NET 5/6. We recommend you skip .NET Standard 2.1 and go straight to .NET 6. Most widely used libraries will multi-target for both .NET Standard 2.0 and .NET 5+. Supporting .NET Standard 2.0 gives you the most reach, while supporting .NET 5+ ensures you can leverage the latest platform features for customers that are already on .NET 5+.

stebet · 2022-04-08T16:03:29Z

Here is also a table for the C# language version support: https://docs.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-1#when-to-target-net50-or-net60-vs-netstandard

.NET Standard 2.1 only supports C# 8 by default so there are definitely more recent language features that would be nice to be able to use such as localsinit, source generators, records, const interpolated strings etc.

C# 9: https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-9
C# 10: https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-10

bording · 2022-04-08T16:03:37Z

@bording Is there a reason you'd want to target .NET Standard 2.0 instead of 2.1, since I think .NET Standard 2.0 for .NET Framework projects tends to not always work correctly For .NET Framework.

@stebet .NET Standard 2.1 is even more of a dead end than .NET Standard 2.0 is. The only thing that supports it is .NET Core 3.1+. My intention of having a netstandard2.0 target is to keep having some sort of .NET Framework support, which you don't get with netstandard2.1.

There's basically no reason to ever use .NET Standard 2.1 and you should pretend it doesn't exist.

bording · 2022-04-08T16:07:59Z

I'm still a bit mystified about why you'd include net6.0 if we can't really use any net6.0 specific features without ifdefs, which sounds like a maintenance headache.

@lukebakken Being able to access new APIs via #ifdefs is a primary benefit, and in some cases it's absolutely worth doing. You can also potentially avoid needing some package dependencies if you are targeting net6.0 instead of netstandard2.0.

Obviously there's a reason I don't understand, because check out these targets - https://github.com/npgsql/npgsql/blob/main/src/Npgsql/Npgsql.csproj

I'm not really sure what they are doing there, either. That seems a bit excessive to me.

…nfirms

``` [xUnit.net 00:00:41.19] RabbitMQ.Client.Unit.TestBlockingCell.TestBackgroundUpdateFails [FAIL] [xUnit.net 00:00:41.19] Assert.Throws() Failure [xUnit.net 00:00:41.19] Expected: typeof(System.TimeoutException) [xUnit.net 00:00:41.19] Actual: (No exception was thrown) [xUnit.net 00:00:41.19] Stack Trace: [xUnit.net 00:00:41.19] D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\Unit\TestBlockingCell.cs(170,0): at RabbitMQ.Client.Unit.TestBlockingCell.TestBackgroundUpdateFails() Passed RabbitMQ.Client.Unit.TestBlockingCell.TestGetValueWhichDoesTimeOut [469 ms] Failed RabbitMQ.Client.Unit.TestBlockingCell.TestBackgroundUpdateFails [739 ms] Error Message: Assert.Throws() Failure Expected: typeof(System.TimeoutException) Actual: (No exception was thrown) Stack Trace: at RabbitMQ.Client.Unit.TestBlockingCell.TestBackgroundUpdateFails() in D:\a\rabbitmq-dotnet-client\rabbitmq-dotnet-client\projects\Unit\TestBlockingCell.cs:line 170 ```

lukebakken · 2022-04-08T17:43:36Z

Thanks everyone. I think what I have in this PR is good for now, and I will add a "7.0 Release Checklist" issue which will include an item to re-visit target frameworks if necessary.

The RabbitMQ team appreciates the input! Have a great weekend!

danielmarbach · 2022-04-08T17:48:58Z

@stebet many language features can be shimmed into the code. For example LocalsInit is just a compiler feature. You can define the attribute as internal in your code and it will magically work. I've done that in the azure sdk.

michaelklishin · 2022-05-07T12:00:38Z

One PR that specifically adopts a C# 10 feature: #1202.

lukebakken added this to the 7.0.0 milestone Apr 5, 2022

lukebakken self-assigned this Apr 5, 2022

lukebakken marked this pull request as ready for review April 6, 2022 12:25

lukebakken force-pushed the rabbitmq-dotnet-client-1172 branch from ff1f501 to dc80d6a Compare April 6, 2022 12:26

lukebakken requested review from michaelklishin and stebet April 6, 2022 12:27

danielmarbach reviewed Apr 6, 2022

View reviewed changes

projects/RabbitMQ.Client/RabbitMQ.Client.csproj Show resolved Hide resolved

lukebakken changed the title ~~Update target framework to net6.0~~ Ensure target frameworks make sense for 7.0 Apr 6, 2022

lukebakken force-pushed the rabbitmq-dotnet-client-1172 branch from dc80d6a to 60f02ce Compare April 6, 2022 15:12

lukebakken requested review from danielmarbach and removed request for stebet April 6, 2022 15:13

lukebakken force-pushed the rabbitmq-dotnet-client-1172 branch from 60f02ce to f5136de Compare April 6, 2022 15:17

danielmarbach reviewed Apr 6, 2022

View reviewed changes

projects/Benchmarks/Benchmarks.csproj Outdated Show resolved Hide resolved

projects/Unit/Unit.csproj Outdated Show resolved Hide resolved

projects/RabbitMQ.Client/RabbitMQ.Client.csproj Show resolved Hide resolved

.github/workflows/main.yaml Outdated Show resolved Hide resolved

lukebakken force-pushed the rabbitmq-dotnet-client-1172 branch from f5136de to fba3869 Compare April 6, 2022 16:47

lukebakken commented Apr 6, 2022

View reviewed changes

projects/RabbitMQ.Client/RabbitMQ.Client.csproj Outdated Show resolved Hide resolved

Update target frameworks to recommended versions

9e76201

Update packages to latest versions Fixes #1172

lukebakken force-pushed the rabbitmq-dotnet-client-1172 branch from fba3869 to 9e76201 Compare April 6, 2022 20:33

lukebakken added 3 commits April 8, 2022 09:45

Fix flaky test by publishing a lot more messages before waiting on co…

d4562da

…nfirms

Add net6.0 to targets

687a6f2

lukebakken mentioned this pull request Apr 8, 2022

7.0 Release Checklist #1191

Closed

3 tasks

lukebakken merged commit b6884c5 into main Apr 8, 2022

lukebakken deleted the rabbitmq-dotnet-client-1172 branch April 8, 2022 20:32

michaelklishin mentioned this pull request May 7, 2022

[REVERTED] Use file-scoped namesapces in RabbitMQ.Client project #1202

Merged

11 tasks

lukebakken mentioned this pull request Mar 21, 2023

Target netstandard2.1 #1320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure target frameworks make sense for 7.0 #1189

Ensure target frameworks make sense for 7.0 #1189

lukebakken commented Apr 5, 2022

michaelklishin commented Apr 6, 2022

lukebakken commented Apr 6, 2022 •

edited

Loading

danielmarbach commented Apr 6, 2022

lukebakken commented Apr 6, 2022

bollhals commented Apr 6, 2022

danielmarbach commented Apr 6, 2022

danielmarbach commented Apr 6, 2022

lukebakken commented Apr 6, 2022

bording commented Apr 6, 2022

michaelklishin commented Apr 6, 2022

stebet commented Apr 8, 2022 •

edited

Loading

lukebakken commented Apr 8, 2022 •

edited

Loading

stebet commented Apr 8, 2022 •

edited

Loading

stebet commented Apr 8, 2022

bording commented Apr 8, 2022 •

edited

Loading

bording commented Apr 8, 2022 •

edited

Loading

lukebakken commented Apr 8, 2022

danielmarbach commented Apr 8, 2022

michaelklishin commented May 7, 2022

Ensure target frameworks make sense for 7.0 #1189

Ensure target frameworks make sense for 7.0 #1189

Conversation

lukebakken commented Apr 5, 2022

michaelklishin commented Apr 6, 2022

lukebakken commented Apr 6, 2022 • edited Loading

danielmarbach commented Apr 6, 2022

lukebakken commented Apr 6, 2022

bollhals commented Apr 6, 2022

danielmarbach commented Apr 6, 2022

danielmarbach commented Apr 6, 2022

lukebakken commented Apr 6, 2022

bording commented Apr 6, 2022

michaelklishin commented Apr 6, 2022

stebet commented Apr 8, 2022 • edited Loading

lukebakken commented Apr 8, 2022 • edited Loading

stebet commented Apr 8, 2022 • edited Loading

stebet commented Apr 8, 2022

bording commented Apr 8, 2022 • edited Loading

bording commented Apr 8, 2022 • edited Loading

lukebakken commented Apr 8, 2022

danielmarbach commented Apr 8, 2022

michaelklishin commented May 7, 2022

lukebakken commented Apr 6, 2022 •

edited

Loading

stebet commented Apr 8, 2022 •

edited

Loading

lukebakken commented Apr 8, 2022 •

edited

Loading

stebet commented Apr 8, 2022 •

edited

Loading

bording commented Apr 8, 2022 •

edited

Loading

bording commented Apr 8, 2022 •

edited

Loading