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 requirement on bitvec for std feature and constructing TypeDefBitSequence. #168

Merged
merged 7 commits into from Sep 13, 2022

Conversation

jsdw
Copy link
Contributor

@jsdw jsdw commented Sep 12, 2022

Requires Rust 1.60 for the optional dependency feature (? in Cargo.toml).

  • Use the "?" feature in cargo.toml features to avoid bringing bitvec in when the "std" feature is enabled, unless it's been specifically asked for. I don't believe that this is a breaking change, since while the bitvec crate would be included if just the "std" feature was enabled and isn't now, it looks like we hide that stuff behind an explicit "bit-vec" feature anyway, so hopefully it just avoids pulling in a crate we don't actually use in that case.
  • Allow TypeDefBitSequence::new() to be called without the bit-vec feature being enabled. This relaxes the constraints on it slightly, making "incorrect" uses more possible, but allowing us to create the type info without expecting any specific underlying library. Again, since this just relaxes/adds, it's not a breaking change.

I also updated the UI test output just because it had changed a touch. There are a couple of unused lifetime warnings that can't be silenced but will be silencable in a future rust update (see rust-lang/rust#96956)

parity-scale-codec PR to help with this: paritytech/parity-scale-codec#375

Closes #167

@jsdw jsdw changed the title Don't force bitvec to be included when "std" is enabled. [WIP] Don't force bitvec to be included when "std" is enabled. Sep 12, 2022
@jsdw jsdw changed the title [WIP] Don't force bitvec to be included when "std" is enabled. Don't force bitvec to be included when "std" is enabled. Sep 12, 2022
@jsdw jsdw changed the title Don't force bitvec to be included when "std" is enabled. Remove requirement on bitvec for std feature and constructure TypeDefBitSequence. Sep 12, 2022
@jsdw jsdw changed the title Remove requirement on bitvec for std feature and constructure TypeDefBitSequence. Remove requirement on bitvec for std feature and constructing TypeDefBitSequence. Sep 12, 2022
@@ -25,7 +25,7 @@ scale = { package = "parity-scale-codec", version = "3", default-features = fals
[features]
default = ["std"]
std = [
"bitvec/std",
"bitvec?/std",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bitvec crate will no longer be pulled in when "std" is enabled, but this I think makes no difference to the external interface since bitvec additions are behind the separate "bit-vec" feature anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh ok; I'll do a quick follow PR there then to prevent that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR for parity-scale-codec thing: paritytech/parity-scale-codec#375

@jsdw jsdw requested a review from ascjones September 12, 2022 13:13
/// With the `bit-vec` feature enabled, the expected usage is to provide either
/// `bitvec::order::Lsb0` or `bitvec::order::Msb0` as the order type, and then something
/// like u8, u8, or u32 as the store type. Without the `bit-vec` feature enabled, it's
/// recommended that your types have identical `TypeInfo` to those.
Copy link
Contributor

Choose a reason for hiding this comment

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

…or else? :)
Like, is "recommended" the right word here? Isn't it more "has to be"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, in theory it can be whatever it wants; it's just that our current tooling won't know how to decode the thing if the types don't line up with what we expect (no reason somebody else can't make tooling that can support more exotic types though if they want to go that route)

@@ -25,7 +25,7 @@ scale = { package = "parity-scale-codec", version = "3", default-features = fals
[features]
default = ["std"]
std = [
"bitvec/std",
"bitvec?/std",
Copy link
Member

Choose a reason for hiding this comment

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

@jsdw
Copy link
Contributor Author

jsdw commented Sep 13, 2022

I'll merge this for now but ideally would make sure we bump the parity-scale-codec version (if needed) when that PR lands before releasing. Actually.. it doesn't matter.. we can do another release if needed.

@jsdw jsdw merged commit 429a388 into master Sep 13, 2022
@jsdw jsdw deleted the jsdw-bitvec-optional branch September 13, 2022 10:06
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.

Don't enable bitvec when "std" feature is enabled
4 participants