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 #108

Merged
merged 22 commits into from
Dec 4, 2020
Merged

Add no_std support #108

merged 22 commits into from
Dec 4, 2020

Conversation

snjax
Copy link
Contributor

@snjax snjax commented Nov 27, 2020

Add no_std support for borsh.

Polyfill is implemented at borsh::lib::* and borsh::error (same as in serde).

For no_std borsh HashMap is imported from hashbrown and Error and Write imported from bare_io.

@MaksymZavershynskyi
Copy link
Contributor

@ailisp , @willemneal could you take a look?

@codecov-io
Copy link

codecov-io commented Nov 28, 2020

Codecov Report

Merging #108 (3768ab8) into master (4c2f1a0) will decrease coverage by 0.21%.
The diff coverage is 89.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
- Coverage   93.43%   93.21%   -0.22%     
==========================================
  Files          21       21              
  Lines        1219     1224       +5     
==========================================
+ Hits         1139     1141       +2     
- Misses         80       83       +3     
Impacted Files Coverage Δ
borsh-rs/borsh/src/schema.rs 100.00% <ø> (ø)
borsh-rs/borsh/tests/test_schema_enums.rs 86.55% <ø> (ø)
borsh-rs/borsh/tests/test_schema_nested.rs 100.00% <ø> (ø)
borsh-rs/borsh/tests/test_schema_structs.rs 100.00% <ø> (ø)
borsh-rs/borsh/tests/test_simple_structs.rs 92.63% <ø> (ø)
borsh-rs/borsh/src/schema_helpers.rs 84.61% <50.00%> (ø)
borsh-rs/borsh/src/de/mod.rs 90.96% <81.03%> (-0.29%) ⬇️
borsh-rs/borsh-derive-internal/src/struct_ser.rs 83.67% <100.00%> (ø)
...rs/borsh-schema-derive-internal/src/enum_schema.rs 95.28% <100.00%> (ø)
...rsh-rs/borsh-schema-derive-internal/src/helpers.rs 92.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c2f1a0...3768ab8. Read the comment docs.

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

Re: "Replace bare-io with semi-custom Error and Write". If the only reason for this is the support for the String error messages, I vote for dropping format! and using a static string message (&'static str). I don't want us to vendor the fork of std library.

borsh-rs/borsh/tests/test_simple_structs.rs Outdated Show resolved Hide resolved
borsh-rs/borsh/src/ser/mod.rs Outdated Show resolved Hide resolved
borsh-rs/borsh/src/ser/mod.rs Outdated Show resolved Hide resolved
borsh-rs/borsh/src/lib.rs Outdated Show resolved Hide resolved
@snjax snjax requested a review from frol November 28, 2020 14:31
@snjax
Copy link
Contributor Author

snjax commented Nov 28, 2020

@frol We updated the PR

@snjax
Copy link
Contributor Author

snjax commented Nov 28, 2020

Re: "Replace bare-io with semi-custom Error and Write". If the only reason for this is the support for the String error messages, I vote for dropping format! and using a static string message (&'static str). I don't want us to vendor the fork of std library.

bare-io is incomplete: there are no hashmaps and hashsets. That's why anyway we should use std polyfill (like serde).

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

It seems that the test suite is not running under no-std mode. I have just tested cargo test -Zpackage-features --package borsh --no-default-features and it failed to compile for obvious reasons of using std contexts. Please, add the relevant cargo test commands to .travis.yml.

P.S. cargo test -Zpackage-features --workspace --no-default-features is not working properly rust-lang/cargo#7160 See next comment

borsh-rs/borsh/src/de/mod.rs Outdated Show resolved Hide resolved
@frol
Copy link
Collaborator

frol commented Nov 30, 2020

Oh, well, cargo test -Zpackage-features --workspace --no-default-features works fine if you edit benchmarks/Cargo.toml to disable the default features:

borsh = { path = "../borsh", default-features = false }

@snjax snjax requested a review from frol December 1, 2020 22:36
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

In order to test no-std, we need to have #![cfg_attr(not(feature = "std"), no_std)] in the test files. Once I add this line to our tests, if fails to compile now. Please, address the compilation errors and make sure that no-std is indeed tested.

P.S. There seems to be a conflict in .travis.yml

P.S.2. Ideally, we need to test that the data structures from no-std are deserializable in std context and wise-versa, but we can work on this under a separate issue.

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

On the other hand, it would fail due to unsupported std types anyway, so it is fine as is, I guess.

@snjax @voidxnull Great work! It is good to be merged. Please, resolve the merge conflict on .travis.yml (I don't have permissions to push to your PR branch to fix it myself)

@snjax
Copy link
Contributor Author

snjax commented Dec 2, 2020

Please, resolve the merge conflict on .travis.yml (I don't have permissions to push to your PR branch to fix it myself)

@frol Done.

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

I am sorry to get back and forth here, but CI is completely broken now.

  1. stable and beta Rust complain about the -Zpackage-features flag being unstable. Let's replace the command with cd borsh && cargo test --no-default-features
  2. nightly-2019-05-22 is too old, replace it with nightly-2020-05-15, which we use for nearcore

@snjax snjax force-pushed the feature/no-std branch 2 times, most recently from 584cbbc to 7879fe8 Compare December 3, 2020 19:42
@snjax
Copy link
Contributor Author

snjax commented Dec 3, 2020

@frol done

@frol frol merged commit 949f0da into near:master Dec 4, 2020
@frol
Copy link
Collaborator

frol commented Dec 4, 2020

Thank you!

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.

5 participants