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 mutually exclusive rkyv features for size. #637

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gz
Copy link

@gz gz commented Jan 12, 2024

rkyv by default serializes usize as u32. This isn't ideal on most modern platforms and unfortunately is configured by a set of (mutually exclusive) feature flags.

Currently rust-decimal sets it to 32-bit in it's Cargo.toml file. This means downstream apps can't override it (or just enable 64-bit de-serialization) as it will break compilation because these flags are mutually exclusive.

For libraries it's recommended by the rkyv maintainer to just use default-features=false and then "re-export" features so the user can control the right serialization size instead of setting one. So this fix removes the features that were unnecessarily set by the Cargo.toml and adds some new features to activate different rkyv sizes. The approach is similar to what the ordered-float or chrono crate do:

https://github.com/reem/rust-ordered-float/blob/8111b345372632893af0b8aa12152f3dc7278aba/Cargo.toml#L37
https://github.com/chronotope/chrono/blob/6ec8f97d16ce320a6f948b1cd1494ed3a21b251f/Cargo.toml#L32

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

@paupino paupino left a comment

Choose a reason for hiding this comment

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

While this change seems simple, it actually causes a couple of issues:

  • This breaks library upgraders - to avoid this you'd need to keep the default rkyv feature and point it to rkyv-32. To do this you'll also need to use the dep: format.
  • Introducing these features will break documentation generation. We compile documentation using --all-features following the pattern defined here.

As an aside, it looks like the main branch of rkyv recently merged a change that resolves this feature exclusivity here: rkyv/rkyv#461. Should we instead wait for this version to be released and remove the size element altogether?

@gz
Copy link
Author

gz commented Jan 15, 2024

This breaks library upgraders - to avoid this you'd need to keep the default rkyv feature and point it to rkyv-32. To do this you'll also need to use the dep: format.

Yes. Unfortunately, I'm not sure what's worse in this case. Just to make sure everyone is on the same page: The following code will compile (but produce wrong results on 64-bit -- assert triggers) at runtime and there is currently no way to change it for a client if they want to use rust-decimal.

fn main() {
    use rkyv::Deserialize;
    let value = usize::MAX;

    // serialize and deserialize the value
    use rkyv::ser::{Serializer, serializers::AllocSerializer};
    let mut serializer = AllocSerializer::<0>::default();
    serializer.serialize_value(&value).unwrap();
    let bytes = serializer.into_serializer().into_inner();
    let archived = unsafe { rkyv::archived_root::<usize>(&bytes[..]) };
    let deserialized: usize = archived.deserialize(&mut rkyv::Infallible).unwrap();

    assert_eq!(deserialized, value);
}

So assuming most likely the majority of users of this library are on 64-bit architectures, and if there are actually any users of rkyv they might just have a bug that's much harder to catch than getting a one-time compile-time breakage during upgrade... Those were my thoughts about it but I understand if you don't want to break it.

While this change seems simple, it actually causes a couple of issues:

Yep, unfortunately mutually exclusive features aren't really a great idea. I'm glad to see it got fixed in rkyv/rkyv#461. I'll check with the rkyv maintainer when we can expect a release for it.

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.

2 participants