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

Set lower minimum dependencies in our Cargo.toml #904

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

rnijveld
Copy link
Member

@rnijveld rnijveld commented Jul 28, 2023

These lower minimum versions were set by lowering the version constraints until either tests started failing or compilation failed. I've also changed dependabot to no longer update our Cargo.toml, although that will also result in dependabot no longer giving us notifications for any semver incompatible changes, but unfortunately the increase-if-necessary dependabot strategy is not supported for cargo. But if we are packaging into OSes then we should treat backwards incompatible upgrades with care anyway. Note that our lockfile still contains the latest versions, and we probably want to add a CI step that runs every once in a while to check that our minimum versions still compile and test correctly.

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #904 (f09559b) into main (a94a972) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #904   +/-   ##
=======================================
  Coverage   82.77%   82.77%           
=======================================
  Files          53       53           
  Lines       13898    13898           
=======================================
  Hits        11504    11504           
  Misses       2394     2394           

Copy link
Member

@davidv1992 davidv1992 left a comment

Choose a reason for hiding this comment

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

Agree with the intent of the changes here. However, this needs quite a bit of testing, especially around the toml and prometheus-client crates where we now support 3 versions that according to semver aren't interchangable. I currently don't have the time to do that properly.

As for CI testing this, I would prefer to have a CI job applying rust-lang/cargo#5657 if possible before merging this (and if that works by now for all the crates we depend on), but it is not a showstopper for me if that turns out to be too much work.

@rnijveld
Copy link
Member Author

rnijveld commented Aug 3, 2023

Both toml and prometheus-client only have backwards incompatible changes that are irrelevant for us right now in the versions specified. Prometheus-client specifically also has updated their semver version when no such increase was necessary. I do want to add a CI job that tests against these minimal dependencies, but I would suggest we do this on a nightly/weekly schedule instead, I'll add that with a separate PR.

@davidv1992 davidv1992 merged commit 6ad3b74 into main Aug 9, 2023
16 checks passed
@davidv1992 davidv1992 deleted the lower-minimum-deps branch August 9, 2023 08:31
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.

2 participants