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

Add no_std support #25

Closed
haerdib opened this issue Jun 20, 2023 · 10 comments · Fixed by #26
Closed

Add no_std support #25

haerdib opened this issue Jun 20, 2023 · 10 comments · Fixed by #26

Comments

@haerdib
Copy link
Contributor

haerdib commented Jun 20, 2023

Thank you very much for this crate, it really simplifies the usage for decoding / encoding of Substrate types from metadata. We'd like to use scale-decode (and encode) for the substrate-api-client, but for this we need no-std support.

I'd happily provide the PR for this, but there are two caveats:

  • thiserror is not no-std compatible. AFAIK, there's no simple workaround
  • currently, error_in_core is a nightly feature only.

So I'm not sure how open you are to a PR in this regard. If you're still interested, I did some work in a fork:

Happy to share and adapt if you see a solution for the caveats above.

@jsdw
Copy link
Collaborator

jsdw commented Jun 20, 2023

Ooh thankyou for your branches!

I would actually like this, scale-encode (and scale-bits and scale-value) to all be no-std compatible eventually (alloc being allowed of course). And also we'd want a CI check to ensure that no future updates break the no-std support too.

Aah I see, the Error trait is not exposed in core yet. thiserror can always be made optional and used only when "std" is enabled I guess :)

I'll take a look at those branches!

@haerdib
Copy link
Contributor Author

haerdib commented Jun 20, 2023

About thiserror: Any idea how to make it optional without duplicating all error types or cluttering the file with #[cfg(..)] ?

@jsdw
Copy link
Collaborator

jsdw commented Jun 20, 2023

Maybe thiserror can just be removed and replaced with a manual Display impl (I see you used a crate for that; likely I'd just do it manually to avoid the extra dependency) and manual From impls as you did. And then a single cfg to enable impl std::error::Error for Error {...} with relevant source impl etc when std is enabled?

@jsdw
Copy link
Collaborator

jsdw commented Jun 20, 2023

Your branches are promising; there's not much to do there I can see!

So if you were up for opening a PR, I'd basically love something that did the following:

  • adds an "std" feature, and is no_std compatible without it, so alloc crate etc as you've done.
  • remove thiserror, replacing with manual From, Display impls and Error impls behind a #[cfg(feature = "std")] flag.
  • adds a CI job to try compiling with --no-default-features with a target eg --target thumbv6m-none-eabi (iirc this one only supports no-std so it's a good way to check that no std importst exist etc).

I'd keep the PR to those things and avoid the rustfmt and other CI things you did in your branches etc offhand so that it's pretty focused, but otherwise that'd be amazing :)

@jsdw
Copy link
Collaborator

jsdw commented Jun 20, 2023

(and I think this is basically all true for scale-encode too!)

@haerdib
Copy link
Contributor Author

haerdib commented Jun 20, 2023

Sure thing. Let me clean up a little on my end.

One last thing: scale-bits currently depends on my GitHub fork due to its no-std compatibility. There's already an open PR for scale-bits (paritytech/scale-bits#2) . If you accept this PR and could publish it on crates.io there'd be no need to change the import to GitHub.

@jsdw
Copy link
Collaborator

jsdw commented Jun 20, 2023

Ah yes; we can't have any github dependencies in the crate (they prevent publishing it), so we'd def need to get scale-bits to no-std first then; I'll have a look at that!

@jsdw
Copy link
Collaborator

jsdw commented Jun 20, 2023

@haerdib scale-bits 0.4.0 is now published fyi; thanks for your efforts there :)

@haerdib
Copy link
Contributor Author

haerdib commented Jun 20, 2023

That was fast, thank you very much!

@haerdib
Copy link
Contributor Author

haerdib commented Jun 22, 2023

Opened a PR on scale-encode: paritytech/scale-encode#11. Let me know what you think of it, I will adapt on both encode and decode branches, so you don't have to review twice.

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 a pull request may close this issue.

2 participants