chronotope / chrono

Date and time library for Rust
Other
3.3k stars 523 forks source link

Remove rustc-serialize from `chrono = "~0.4.38"` #1548

Closed workingjubilee closed 6 months ago

workingjubilee commented 6 months ago

Per Cargo's documentation on its resolver:

The resolver will skip over versions of packages that are missing required features. For example, if a package depends on version ^1 of regex with the perf feature, then the oldest version it can select is 1.3.0, because versions prior to that did not contain the perf feature. Similarly, if a feature is removed from a new release, then packages that require that feature will be stuck on the older releases that contain that feature.

Thus, while it may be unusual to make this change on a SemVer minor release, it is de facto not a "breaking change" to dependents to remove the rustc-serialize feature. Dependents on it will simply not update to a later version of the chrono crate (i.e. 0.4.38 or later), as Cargo will correctly identify the incompatibility, and will keep giving them a maximum of chrono = { version = "0.4.37", features = ["rustc-serialize"] } instead. This may not be something one should do casually, but as the dependency has been deprecated for almost a year now, and Rust plans to remove the relevant code, it seems preferable.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.79%. Comparing base (7d62045) to head (c4e1957).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1548 +/- ## ========================================== - Coverage 91.82% 91.79% -0.04% ========================================== Files 40 37 -3 Lines 18345 18185 -160 ========================================== - Hits 16846 16693 -153 + Misses 1499 1492 -7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

workingjubilee commented 6 months ago

It's not clear to me if this comment remains actionable: https://github.com/chronotope/chrono/blob/d79a0ee1aa2723dafdf4b8e9eab6a95016eec405/.github/workflows/test.yml#L164-L167

djc commented 6 months ago

We should squash all these commits, since some of them cannot pass CI independently. Can just do it during merge in this csae.

workingjubilee commented 6 months ago

Very nice, and glad to see this gone 😄.

I suppose there is not really a way to test this except by making a release. Since when does cargo's resolver have this ability? Is it supported by our MSRV 1.61? I can't find it, but according to archive.org it is documented this way for at least three years so 👍.

You can experiment with a Cargo project using cargo update --precise and try first releasing 0.4.38-rc.0, if you like? But yes, the feature that will prevent this has essentially "always been there": it was part of the original resolver, so there was probably no release in which it was absent, and it has definitely been there since at least 1.56 (when they started using resolver = "2").

It is old but somewhat true again. Could you replace it with something like "We can't use --all-features because of the conflicting rkyv-* features."?

Done.

pitdicker commented 6 months ago

Thank you!