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

Bump dependencies #65

Merged
merged 3 commits into from
Dec 14, 2023
Merged

Bump dependencies #65

merged 3 commits into from
Dec 14, 2023

Conversation

rubdos
Copy link
Member

@rubdos rubdos commented Dec 14, 2023

Follow-up from #60, such that #60 can focus on the lazy_static -> OnceCell transition.

@rubdos rubdos requested a review from gferon December 14, 2023 12:55
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (226b6fb) 71.39% compared to head (d70a9d4) 71.18%.

Files Patch % Lines
src/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
- Coverage   71.39%   71.18%   -0.22%     
==========================================
  Files          19       19              
  Lines        1930     1933       +3     
==========================================
- Hits         1378     1376       -2     
- Misses        552      557       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rubdos
Copy link
Member Author

rubdos commented Dec 14, 2023

@djc, let's take the dependency and CI discussion here, this is a more isolated pull request.

@djc
Copy link

djc commented Dec 14, 2023

FWIW, unless you put in the work to have CI jobs for each semver-incompatible branch (or have CI testing minimal-versions), I'm weary of >=0.10, <=0.12 style dependencies. I think this means Cargo will generally select 0.12.x in CI, so you're not actually testing whether 0.10 and 0.11 still work over time.

I just tested, and apparently since the rust-version statements appeared in Cargo.toml, cargo is smart enough to check out the lower versions when necessary for lower rustc. Your point still stands though, there's no formal test around. Are you aware of any formal tests for these situations?

I basically never do range dependencies like this -- the complexity isn't worth it to me. The canonical way to test this stuff would be with -Z minimal-versions but even then you'd only be testing 0.10, not 0.11, so there's no good guarantees.

(Also, while I consider myself fairly conservative on MSRV issues, I think 1.6x is a pretty reasonable target at this point, especially for low x.)

I would still consider 1.61 at this point, since that's what SailfishOS will soon be on (if not 1.72). More out of personal "greed" or interest than anything else, for which I'm sorry.

1.61 makes sense to me, that's what we've been using for rustls.

@rubdos
Copy link
Member Author

rubdos commented Dec 14, 2023

I basically never do range dependencies like this -- the complexity isn't worth it to me. The canonical way to test this stuff would be with -Z minimal-versions but even then you'd only be testing 0.10, not 0.11, so there's no good guarantees.

Interesting, thanks for sharing. To me, testing MSRV and latest stable is good enough for now. I suppose the real fix would be to have the upstream deps release a 1.x.

1.61 makes sense to me, that's what we've been using for rustls.

I'll consider 1.61 next. There is currently nothing really pushing us there as far as I can tell. Only the derive(Default), but that is 1.62 material anyway.

Thanks for your valuable input! I have implemented your suggestion for CI :)

@rubdos rubdos removed the request for review from gferon December 14, 2023 13:44
@rubdos
Copy link
Member Author

rubdos commented Dec 14, 2023

@ds-cbo / @djc, does either of you want to do a review on this, before I merge? Gabriel is currently ill, so I'd rather spare him a bit :-)

regex = "1.7"
regex-cache = "0.2"
serde = "1.0"
serde_derive = "1.0"
strum = { version = "0.24", features = ["derive"] }
strum = { version = ">=0.24, <=0.25", features = ["derive"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning behind <= 0.25 ? 0.25 is currently the latest, and afaik there's no "watch out, next release will be breaking" coming up

Copy link
Member Author

Choose a reason for hiding this comment

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

Because 0.26 is semver-breaking according to how cargo defines version... There's no real way to tell Cargo that we'd like any future compatible version, because all future compatible versions are by definition 0.25.x for any x

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh fair enough, I forgot about semver (and its 0.x specialties) for a second

@rubdos rubdos merged commit c25b37f into main Dec 14, 2023
8 checks passed
@rubdos rubdos deleted the bump-deps-2 branch December 14, 2023 13:59
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.

3 participants