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

Expose rkyv features as features for chrono users. #1368

Merged
merged 2 commits into from
Dec 29, 2023

Conversation

gz
Copy link
Contributor

@gz gz commented Dec 12, 2023

rkyv by default serializes usize as u32. This isn't ideal on most modern platforms and unfortunately is configured through a feature flag.

If we just set default-features = false in the rkyv Cargo dependency, the crate fails to compile because all the size features are mutually exclusive. On the other hand if we want to e.g., change the serialization of usize to 64-bit and we also want to use chrono this currently fails to compile because chrono always enables rkyv/size_32.

This re-exports the relevant rkyv features so users can choose which serialization to enable. The approach is similar to what the ordered-float crate does:

https://github.com/reem/rust-ordered-float/blob/8111b345372632893af0b8aa12152f3dc7278aba/Cargo.toml#L37

Note aside: rkyv with `default-features = false is what was recommended on the rkyv discord by the maintainer (see screenshot below). But it seems that the approach ordered-float took is better as it doesn't break backward compatibility (and compilation of chrono library without specifying any features).

image

Thanks for contributing to chrono!

If your feature is semver-compatible, please target the 0.4.x branch;
the main branch will be used for 0.5.0 development going forward.

Please consider adding a test to ensure your bug fix/feature will not break in the future.

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

This makes sense to me, thanks!

If you want to use this any time soon, I recommend retargeting the PR/rebasing the branch on 0.4.x.

Cargo.toml Outdated Show resolved Hide resolved
src/datetime/mod.rs Outdated Show resolved Hide resolved
@gz
Copy link
Contributor Author

gz commented Dec 14, 2023

Thanks for the review, I updated this and also made some changes to CI scripts to account for the fact that --all-features no longer works out of the box in a new commit.

@gz
Copy link
Contributor Author

gz commented Dec 14, 2023

If you want to use this any time soon, I recommend retargeting the PR/rebasing the branch on 0.4.x.

Is there a timeline for the 0.5 release? I'm happy to send a PR for 0.4 if you think the 0.5 release is still a few months out.

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d7b4a82) 91.64% compared to head (70dfc07) 91.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##            0.4.x    #1368      +/-   ##
==========================================
+ Coverage   91.64%   91.67%   +0.02%     
==========================================
  Files          38       38              
  Lines       17554    17577      +23     
==========================================
+ Hits        16088    16113      +25     
+ Misses       1466     1464       -2     

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

@gz gz force-pushed the rkyv-features branch 3 times, most recently from 12ea985 to bbfdc20 Compare December 14, 2023 23:04
@gz
Copy link
Contributor Author

gz commented Dec 14, 2023

Looks like the last test-failure I get is

Run cargo test --lib \
error: failed to parse manifest at `/home/runner/work/chrono/chrono/Cargo.toml`

Caused by:
  namespaced features with the `dep:` prefix are only allowed on the nightly channel and requires the `-Z namespaced-features` flag on the command-line
Error: Process completed with exit code 101.

and it's due to https://github.com/chronotope/chrono/pull/1368/files#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R37

I added it due to the suggestion here:

I propose that we instead use an rkyv-any (open to better naming suggestions) feature that is enabled by all the size features (add a comment that it is for internal use only, maybe?).

I'm not quite sure if it's possible to specify the feature with the same intent without using dep:? The alternatives are to go back to the uglier any(f1, f2, f3) approach or raise msrv to 1.60 (which is when this was stabilized). Any preferences on how to proceed?

@djc
Copy link
Contributor

djc commented Dec 15, 2023

Is there a timeline for the 0.5 release? I'm happy to send a PR for 0.4 if you think the 0.5 release is still a few months out.

There is not, you definitely want to be on 0.4.x for now (and we do regular merges from 0.4.x -> main), so please just retarget this PR.

@djc
Copy link
Contributor

djc commented Dec 15, 2023

I'm not quite sure if it's possible to specify the feature with the same intent without using dep:? The alternatives are to go back to the uglier any(f1, f2, f3) approach or raise msrv to 1.60 (which is when this was stabilized). Any preferences on how to proceed?

I think it is possible, just replace dep:rkyv with rkyv.

rkyv by default serializes usize as u32. This isn't ideal
on most modern platforms and unfortunately is configured through
a feature flag.

If we just set `default-features = false` in the rkyv Cargo
dependency, the crate fails to compile because all the size features
are mutually exclusive. On the other hand if we want to
e.g., change the serialization of usize to 64-bit and
we also want to use chrono this currently fails to compile
because chrono always enables rkyv/size_32.

This re-exports the relevant rkyv features so users can
choose which serialization to enable. The approach
is similar to what the ordered-float crate does:

https://github.com/reem/rust-ordered-float/blob/8111b345372632893af0b8aa12152f3dc7278aba/Cargo.toml#L37

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
@gz gz changed the base branch from main to 0.4.x December 27, 2023 01:38
@gz gz force-pushed the rkyv-features branch 3 times, most recently from fbf5136 to 8e373df Compare December 27, 2023 01:52
This is due to the mutually exclusive features in rkyv which
we expose now. `--all-features` will now activate them and the crate
will fail to compile rkyv. We work around this by defining
an explicit list of all mutually exclusive features to.

Unfortunately there isn't an easy way to share env variables
among different YAML files
(actions/runner#655).

There also isn't a good way to specify `--all-features` minus
"just a few" (rust-lang/cargo#3126)
aside from giving the complete list.

Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
@gz
Copy link
Contributor Author

gz commented Dec 27, 2023

@djc I rebased it on the 0.4 branch

@djc djc merged commit 849932b into chronotope:0.4.x Dec 29, 2023
37 checks passed
@djc
Copy link
Contributor

djc commented Dec 29, 2023

Thanks!

@pitdicker
Copy link
Collaborator

This PR was a breaking change, chrono no longer builds with just features = ["rkyv"] as reported in #1380.

This breaks our build on docs.rs for 0.4.32, although that is easy to fix.

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