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

no-std support for hickory-proto #2104

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

domenukk
Copy link

@domenukk domenukk commented Dec 5, 2023

This is a rather big pull request that allows the use of hickory-proto in no_std environments.

It has been tested on an embedded system and works, however there are certain gotchas, listed below.
Hence, this PR is marked as draft and we hope to hear some maintainer feedback on it.

Some gotchas are as follows:

  • So far, no-std support requires nightly features. We have feature-gated those behind an unstable feature. The std feature doesn't require nightly - everything works as normal here. The nightly features we need are:
  • This PR also depends on an open pull request to the rust-url crate. Either it gets merged, or we will have to upload a no-std compatible for to crates for the time being, see no_std support for the url crate servo/rust-url#831.
  • Similarly, it uses thiserror_core, a no-std compatible version of thiserror on cargo.
  • For lack of better randomness, all users should manually seed the RNG using seed_rng

One possible way forward, instead of going all-in on, could be to merge the unstable as well as std features from this PR and the no_std header (including all vec and string imports), but continue to depend on the upstream url and thiserror crates, so that a no_std forks is easier to maintain (by changing the imports in Cargo.toml)

Please let us know what you think :)

@djc
Copy link
Collaborator

djc commented Dec 5, 2023

Who is us? What is your use case for this? If we're going to review this, it would be nice to get it split into smaller commits that do one thing instead of one big wad of code -- would you be up for that?

@domenukk
Copy link
Author

domenukk commented Dec 5, 2023

Thank you for your prompt response!

The use-case is parsing DNS responses in an RTOS (so no rust stdlib support).
It's just a proof-of-concept at this point, but it works, so I figured a PR could already help other people looking for a no_std solution.

I'll be glad to work on this / split it up into chunks, if there is interest to land (parts of) this PR from your end
However, most of the changes are simply sprinkling things like use alloc::vec::Vec throughout the codebase, and they will all have to be in a single PR to make sense.

@bluejekyll
Copy link
Member

This is a really big change right now. I think having no-std makes sense, but is definitely going to be challenging to land this in its current form.

I wonder if there is a simpler pass that could be taken, where the initial start is in the Cargo.toml, and then focusing a series of PRs on allowing different modules to be ok without the std feature (essentially start with all modules needed std). Maybe you started there and it spiderwebs out quickly. But focusing on perhaps just making resource::Record no-std compatible seems like it would be a good first PR to land?

It sounds like you're open to doing that work, and it might get us to a better spot with less overhead from a review prespective? Also, it would probably help with passing all the different test cases.

@domenukk
Copy link
Author

domenukk commented Dec 5, 2023

As far as I can see, the testcases fail right now because of unstable features in use.
I don't think there is a good solution for no_std support without said unstable features, namely support for net and error in core.

We can totally move to no_std module by module, but there might be a bunch of cross-module dependencies as well.
I can try resource::Record and see where it leads us?

@bluejekyll
Copy link
Member

yeah, Error is still going to be needed, as well as some portion of net...

@djc
Copy link
Collaborator

djc commented Dec 6, 2023

I think the path here is smaller "horizontal" changes, like:

  • One commit/PR to import alloc::Vec everywhere that it is necessary (I think there's a clippy lint to help enforce this?)
  • One commit/PR to convert to thiserror_core
  • One commit/PR to make Cargo features minimal/optional

These help us move to no_std incrementally while making it easier to digest the smaller reviews.

(We've been going through a similar strategy in the rustls project.)

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.

None yet

3 participants