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

downgrade dependencies #1594

Merged
merged 2 commits into from
Jun 7, 2024
Merged

Conversation

WeihanLi
Copy link
Contributor

@WeihanLi WeihanLi commented Jun 6, 2024

Proposed Changes

The dependencies seemed no need to use a higher version

#1481 (comment)
#1481 (review)

@michaelklishin
Copy link
Member

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

@WeihanLi
Copy link
Contributor Author

WeihanLi commented Jun 6, 2024

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
Copy link
Contributor

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

@lukebakken lukebakken self-assigned this Jun 6, 2024
@lukebakken lukebakken added this to the 7.0.0 milestone Jun 6, 2024
@danielmarbach
Copy link
Collaborator

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
Copy link
Contributor

thompson-tomo commented Jun 6, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

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 lukebakken force-pushed the downgrade-dependencies branch from 1c4d15e to 528092a Compare June 7, 2024 13:08
@lukebakken
Copy link
Contributor

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.

@lukebakken lukebakken merged commit db48736 into rabbitmq:main Jun 7, 2024
11 checks passed
@WeihanLi WeihanLi deleted the downgrade-dependencies branch June 7, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants