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

Rust: add support for no_std env #205

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

overcat
Copy link
Contributor

@overcat overcat commented Sep 12, 2024

Enable rs-stellar-xdr to encode/decode XDR in a no_std environment.

See stellar/rs-stellar-xdr#392

@overcat overcat marked this pull request as ready for review September 12, 2024 08:18
@@ -76,7 +76,7 @@ def render_top_matter(out)
// #{@output.relative_source_paths.join("\n// ")}
EOS
out.break
out.puts "#![allow(clippy::missing_errors_doc, clippy::unreadable_literal)]"
out.puts "#![allow(clippy::missing_errors_doc, clippy::unreadable_literal, clippy::needless_question_mark)]"
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 did not modify the relevant code, so I am not sure why clippy is giving a warning. And I encountered some difficulties in fixing this warning, so let's add it first.

@overcat overcat changed the title Rust: add support for no_std env Rust: add support for no_std env Sep 12, 2024
@leighmcculloch
Copy link
Member

Thanks for looking into this.

Why embedded-io over core_io? Are there other crates to consider? I haven't looked into this enough yet to know myself, but core_io is more of a mirror of std::io, and it has a Cursor type.

I noticed that we're using embedded_io_extras to get a Cursor type. Do you think that could be contributed upstream to reduce the nested dependencies we're adding to the xdr lib? I also wonder if embedded-hal has an equivalent type we're missing.

Rather than add a generic sounding 'alloc' feature to the stellar-xdr crate, I'm inclined to instead add a feature named after the optional dependency. So if you enable the embedded-io feature, you get embedded-io capability.

We need to think about how to test this for consistency with the std::io behavior. The xdr lib depends on the std::io::Read::read_exact function, and it's default_read_exact implementation, and it has some subtle interactions with reading EOF safeties in the xdr lib. But I notice that the embedded-io read_exact function isn't identical, and so I think we'll be hesitant to merge changes unless we can either avoid the difference, or do some differential fuzzing to ensure there's no breakage or observable difference in behavior.

@overcat
Copy link
Contributor Author

overcat commented Sep 13, 2024

Hi @leighmcculloch, thank you for taking the time to review the PR.

Why embedded-io over core_io? Are there other crates to consider? I haven't looked into this enough yet to know myself, but core_io is more of a mirror of std::io, and it has a Cursor type.

I researched the following libraries:

  • ciborium_io: Only provides Read/Write traits and differs significantly from std::io::Read/Write.
  • core2: Offers an API almost identical to std::io, but hasn't been maintained for over two years. I'm concerned about the project's future. [WIP] Rust: add support for no_std env #204 includes an implementation using core2.
  • core_io: Last released in 2021 with very few downloads.
  • embedded-io: Maintained by rust-embedded, actively developed. I believe it's currently the best choice.

I noticed that we're using embedded_io_extras to get a Cursor type. Do you think that could be contributed upstream to reduce the nested dependencies we're adding to the xdr lib? I also wonder if embedded-hal has an equivalent type we're missing.

I will create an issue to ask if they would consider adding Cursor to embedded-io. Another option is to implement Cursor in rs-stellar-xdr, allowing us to depend directly on embedded-io. I think Cursor does not exist in embedded-hal.

Rather than add a generic sounding 'alloc' feature to the stellar-xdr crate, I'm inclined to instead add a feature named after the optional dependency. So if you enable the embedded-io feature, you get embedded-io capability.

I like this idea, so that embedded-io will only be used when the user explicitly enables it.

We need to think about how to test this for consistency with the std::io behavior. The xdr lib depends on the std::io::Read::read_exact function, and it's default_read_exact implementation, and it has some subtle interactions with reading EOF safeties in the xdr lib. But I notice that the embedded-io read_exact function isn't identical, and so I think we'll be hesitant to merge changes unless we can either avoid the difference, or do some differential fuzzing to ensure there's no breakage or observable difference in behavior.

I need to do a bit more research. If I have any questions, can I reach out to you on Discord?

@overcat
Copy link
Contributor Author

overcat commented Sep 21, 2024

@@ -2767,3 +2816,335 @@ mod test {
assert_eq!(v.to_option(), Some(1));
}
}

#[cfg(all(test, any(feature = "std", feature = "embedded_io")))]
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 added some tests to verify whether the behavior of embedde io meets expectations.

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