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

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

Merged
merged 3 commits into from
Mar 30, 2024

Conversation

workingjubilee
Copy link
Contributor

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.

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

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

@workingjubilee
Copy link
Contributor Author

It's not clear to me if this comment remains actionable:

# We can't use `--all-features` because `rustc-serialize` doesn't support
# `wasm32-wasi`.
- run: cargo wasi test --features=serde,unstable-locales --color=always -- --color=always

Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

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

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 👍.

It's not clear to me if this comment remains actionable:

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."?

@djc
Copy link
Contributor

djc commented Mar 29, 2024

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

@workingjubilee
Copy link
Contributor Author

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

Thank you!

@pitdicker pitdicker merged commit 6c4e735 into chronotope:main Mar 30, 2024
35 checks passed
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