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

chore(common): bump MSRV to 1.56 #866

Merged
merged 4 commits into from Sep 1, 2022
Merged

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp commented Aug 22, 2022

  • Bump MSRV to 1.56
  • Bump the edition to 2021
  • Set thr rust-version in Cargo.tomls
  • Add patch_dependencies to lower dashmap and time version. dashmap latest version requires 1.59, time latest version requires 1.57
  • Unpin indexmap from 1.8

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #866 (1609a42) into main (c92a1ad) will decrease coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            main    #866     +/-   ##
=======================================
- Coverage   66.6%   66.6%   -0.1%     
=======================================
  Files        113     113             
  Lines       8727    8726      -1     
=======================================
- Hits        5820    5813      -7     
- Misses      2907    2913      +6     
Impacted Files Coverage Δ
opentelemetry-api/src/lib.rs 100.0% <ø> (ø)
...aeger/src/exporter/config/collector/http_client.rs 22.5% <ø> (ø)
opentelemetry-jaeger/src/lib.rs 93.3% <ø> (+0.3%) ⬆️
opentelemetry-zipkin/src/lib.rs 100.0% <ø> (ø)
opentelemetry/src/lib.rs 100.0% <ø> (ø)
opentelemetry-api/src/context.rs 82.9% <0.0%> (-6.0%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@djc
Copy link
Contributor

djc commented Aug 22, 2022

I'm confused about the dashmap change (the repository seems to be testing 1.49). In general, in the crates ecosystem bumping the MSRV without bumping the feature version seems to be the de facto standard so calling it out doesn't seem helpful. I would probably just go with 1.57 if that's what's required for time.

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Aug 23, 2022

I'm confused about the dashmap change (the repository seems to be testing 1.49)

Their rust-version in Cargo.toml is 1.59.

In general, in the crates ecosystem bumping the MSRV without bumping the feature version seems to be the de facto standard so calling it out doesn't seem helpful

dashmap made a commitment to bump MSRV with a major version bump so I was confused. Revised PR description

I would probably just go with 1.57 if that's what's required for time.

Emm I think we should try to keep MSRV as low as possible. I intend to replace the patch_dependencies.sh with cargo-minimal-versions and run MSRV with minimal compatible version. So our MSRV checks won't break whenever one of dependencies bumps MSRV

@TommyCpp TommyCpp marked this pull request as ready for review August 23, 2022 05:37
@TommyCpp TommyCpp requested a review from a team as a code owner August 23, 2022 05:37
@djc
Copy link
Contributor

djc commented Aug 23, 2022

Emm I think we should try to keep MSRV as low as possible. I intend to replace the patch_dependencies.sh with cargo-minimal-versions and run MSRV with minimal compatible version. So our MSRV checks won't break whenever one of dependencies bumps MSRV

I wonder how useful this form of MSRV support is. For any downstream users that actually want to keep their dependencies up to date, it doesn't seem very meaningful. If our dependencies require a higher MSRV we should just adopt their MSRV and/or push back on their MSRV bumps. Otherwise the result is that MSRV xor bug fixes, which doesn't seem great either.

@TommyCpp
Copy link
Contributor Author

I wonder how useful this form of MSRV support is

I think one use case is users that who are using 1.56 can pin time and dashmap dependency in their application to "force" resolve dependencies that work with 1.56. If we bump the MSRV to 1.59 then they won't be able to use the create at all.

tracing also runs their MSRV check against the minimal version of dependencies(See here). That's why they depended on time and still have an MSRV at 1.49.

I am also concerned that simply keeping our MSRV equal to the max(MSRV of all our dependencies) will break our CI pipeline and force us to release versions more often. Also, it's possible we won't be keep up with our MSRV promise as we now have less control of MSRV.

Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

This looks good to me. I agree that this isn't a perfect solution but I don't have a better one at the moment.

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Sep 1, 2022

Gonna merge it as it is. Feel free to reopen the discussion if needed.

@TommyCpp TommyCpp merged commit 9ea1ee7 into open-telemetry:main Sep 1, 2022
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.

None yet

3 participants