add no_std support #42
Conversation
#2) * Metadata supporting scale-info can be decoded and serialized in no_std (feature flag scale_info) * Changes from review: rename feature flag into full_derive * Changes from review: std implies full-derive * Changes from review: optimization Co-authored-by: echevrier <[email protected]>
|
User @echevrier, please sign the CLA here. |
|
@echevrier what are your thoughts? Are you happy for this to be upstreamed? |
|
|
thinking about renaming |
(drop new Debug impls)
|
Very happy to see this upstream! I can understand that it is tempting to leave out The reasoning behind the Again, I don't mind if it is split up, but I would be very happy to see the optional |
|
I have no objection to having a debug feature (or alternatively just having
them derive debug by default) - if the derives are not used I believe they
won’t get to be in the final binary.
…On Mon, 5 Sept 2022 at 17:15, clangenb ***@***.***> wrote:
Very happy to see this upstream! I can understand that it is tempting to
leave out Debug. However, not having debug limits downstream derives
greatly, I would be very happy to see debug at least as an additional
feature.
The reasoning behind the full_derive was exactly that it includes
serde-codec, scale-decode *and* debug, considering that a crate that uses
one of the features is probably by nature not in a performance critical
environment, so it does not hurt to enable them all at once.
Again, I don't mind if it is split up, but I would be very happy to see
the optional debug feature. :)
—
Reply to this email directly, view it on GitHub
<#42 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGEJCAF7EQUTRLC75XFUK3V4YMDBANCNFSM572XLBVQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
#39 suggests calling |
Are there any downsides? Unused debug impls will just get optimised away won't they?
|
Ah. I see debug was already in std but just not pushed down to non-std. Right, happy now with this. |
|
Moved the debug down for the other versions as well. |
.github/workflows/rust.yml
Outdated
| with: | ||
| command: check | ||
| toolchain: stable | ||
| args: --manifest-path ./frame-metadata/Cargo.toml --no-default-features --features decode,serde_full,v14 |
There was a problem hiding this comment.
Maybe also worth adding a specific check for just serde_full and just decode features being enabled? (it's so easy to miss compile errors that don't occur when both are enabled but do occur when just one is)
There was a problem hiding this comment.
I suggest that we try to use cargo hack check --each-feature here if it's possible instead doing this manually it is so easy to forget some combination of feature flags.
(if this plays well with ci then we should double up, doing the same again but target wasm32)
|
How does this look re cargo hack? Not entirely sure how I coax the ci to run the changes... |
|
Is there a reason that prevents this PR from being merged? Would be nice to see it in the next release. |
|
If @jsdw and @niklasad1 are happy that their suggestions are applied then we can merge |
|
yeah, it looks good to me @jsdw ? :) |
|
I'm good with it! Ultimately we probably want to add a more thorough check that the no_std feature actually works (it's cropped up in other crates that somebody breaks no_std by importing some std thing not behind a feature flag etc, and it is missed) but I'm happy with this merging anyway at the mo; let's just open an issue to do a more thorough test! |
This PR is proposing upstreaming the work done by Integritee parachain and in particular @echevrier to make frame-metadata work for no_std. I have been using this fork myself for a month or two now and can confirm it works fine in browsers.
It's pretty important that we have no_std support as otherwise it's a big blocker for anyone trying to decode scale in wasm (ie. in the browser).
Happy to implement any feedback to get no_std support into main.
Co-authored-by: echevrier [email protected]