rabbitmq / rabbitmq-dotnet-client

RabbitMQ .NET client for .NET Standard 2.0+ and .NET 4.6.2+
https://www.rabbitmq.com/dotnet.html
Other
2.07k stars 579 forks source link

downgrade dependencies #1594

Closed WeihanLi closed 3 months ago

WeihanLi commented 3 months ago

Proposed Changes

The dependencies seemed no need to use a higher version

https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/1481#discussion_r1469234211 https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/1481#pullrequestreview-1847905299

michaelklishin commented 3 months ago

I vote against this. Using the latest versions is the optimal strategy, otherwise we will be getting PRs asking to upgrade them.

WeihanLi commented 3 months ago

I vote against this. Using the latest versions is the optimal strategy, otherwise we will be getting PRs asking to upgrade them.

Good to me, while from the note and discussion here @lukebakken @paulomorgado and @thompson-tomo seemed to prefer the lower dependencies, thoughts?

And currently, the 6.0.0 version does not have CVEs that have to be upgraded from the nuget.org

lukebakken commented 3 months ago

Thanks @WeihanLi

Keeping dependencies at their minimum version makes sense to me at this time, until someone finds a bug or other serious issue with it.

In this scenario, I like to ask, "What would Npgsql do?" ... and it seems their policy is to use the latest version of deps:

https://github.com/npgsql/npgsql/blob/main/Directory.Packages.props#L3-L15

I'm going to re-tag some people to get them to chime in one last time if they have strong enough feelings about this.

@paulomorgado @thompson-tomo @stebet @danielmarbach

danielmarbach commented 3 months ago

I must say @bording is my goto all around awesome person when it comes to knowing these subtle differences and tradeoffs.

Personally I see little reason to keep a version that provides an API surface that is quite outdated and block adoption of newer APIs just because they are not needed yet. System or Microsoft dependencies tend to be very stable in general and thus I would tend to be slightly more "generous“ to allow future use instead of locking yourself in until you need it and then having to do minors for things that might be purely internal facing changes

thompson-tomo commented 3 months ago

i am also a similar train of thought to @lukebakken where we should be focussed on using the lowest version which is free of CVE's & provides the necessary functionality especially for Microsoft packages as this enables more efficient conditional packaging. It doesn't feel right to me to have an explicit dependency on let's Say syetem.Text.Json 8.0.0 for Net Standard 2.0 but for the NET 6.0 TFM we rely on the framework version which would result in System.Text.Json 6.0.0 being used.

In terms of developers who are wanting to maintain transitive dependencies this possible on a case by case basis by them using transitive version pinning and/or making them explicit dependencies for the key libraries they are wanting to keep up to date,

danielmarbach commented 3 months ago

doesn't feel right to me to have an explicit dependency on let's Say syetem.Text.Json 8.0.0 for Net Standard 2.0 but for the NET 6.0 TFM we rely on the framework version which would result in System.Text.Json 6.0.0 being used.

FYI that's independent from using lowest and I do agree the same package version should be used across the TFMs if there is a package availability for both TFMs. I think that's what I even suggested somewhere (not in the linked comment)

paulomorgado commented 3 months ago

Thanks @WeihanLi

Keeping dependencies at their minimum version makes sense to me at this time, until someone finds a bug or other serious issue with it.

In this scenario, I like to ask, "What would Npgsql do?" ... and it seems their policy is to use the latest version of deps:

https://github.com/npgsql/npgsql/blob/main/Directory.Packages.props#L3-L15

I'm going to re-tag some people to get them to chime in one last time if they have strong enough feelings about this.

@paulomorgado @thompson-tomo @stebet @danielmarbach

Libraries shouldn't constrain the user to upgrade their dependencies if not needed.

Applications are a complex ecosystem and users might be barred from using this version if this version unnecessarily forces them to higher versions.

That might require different versions for different TFMs, but that's the life of owning libraries.

On the other hand, I do not think the libraries should depend on other libraries with known vulnerabilities or known bugs and avoid out of support libraries.

lukebakken commented 3 months ago

I do agree the same package version should be used across the TFMs if there is a package availability for both TFMs

Thanks @danielmarbach for chiming in.

I appreciate everyone's patience with my understanding of this issue... I think it has finally sunk in 😸

We'll continue to use the lowest version practical for dependencies, and ensure that the version used is the same across TFMs.