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

Serde no_std compatibility #288

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

semenov-vladyslav
Copy link
Contributor

This PR addresses issue #287. Tests pass with different combinations of features: "serde", "serde alloc", "serde std", "alloc".

@tarcieri
Copy link
Contributor

tarcieri commented Mar 7, 2023

Weird failure in the MSRV test. Wonder if it's a nightly bug in the unused import diagnostic.

@semenov-vladyslav maybe try pinning a slightly older nightly?

@semenov-vladyslav
Copy link
Contributor Author

Looks like clippy failed on unused imports. Forgot to update cfg pragma to a few other imports. Essentially, "serde" feature requires one of "std" or "alloc". Fixed that, now it should work.

@pinkforest
Copy link
Contributor

pinkforest commented Mar 12, 2023

Looks like we didn't use thumbv7em-none-eabihf in CI that tests the target with no std avail.

We should probably add that:
https://github.com/dalek-cryptography/curve25519-dalek/blob/main/.github/workflows/rust.yml#L45-56

Adding a test here to CI: #289 - checking that this fails as expected.

@pinkforest pinkforest mentioned this pull request Mar 12, 2023
@pinkforest
Copy link
Contributor

Confirming no_std build works in main without serde:
https://github.com/dalek-cryptography/ed25519-dalek/actions/runs/4394998670/jobs/7696444822

However with serde it fails as expected:
https://github.com/dalek-cryptography/ed25519-dalek/actions/runs/4395018249/jobs/7696481185

Can we re-base this when the test PR in CI so we ensure in CI it's fixed.

ryankurte added a commit to ryankurte/ed25519-dalek that referenced this pull request Mar 20, 2023
as an alternative to dalek-cryptography#288 this updates serde `Serialize` and `Deserialize` implementations to use a
custom visitor, removing the need for `alloc` or `std` for embedded use, and making this
consistent with implementations in
[curve25519-dalek](https://github.com/dalek-cryptography/curve25519-dalek/blob/a63e14f4ded078d6bf262ba0b3f47026bdd7f7c0/src/edwards.rs#L269).

@pinkforest seems like it'd be good to have some serde tests / this should go over dalek-cryptography#289?

Co-Authored-By: Vlad Semenov <semenov.vlad.by@gmail.com>
ryankurte added a commit to ryankurte/ed25519-dalek that referenced this pull request Mar 21, 2023
…lize` and `Deserialize` implementations to use a

custom visitor, removing the need for `alloc` or `std` for embedded use, and making this
consistent with implementations in
[curve25519-dalek](https://github.com/dalek-cryptography/curve25519-dalek/blob/a63e14f4ded078d6bf262ba0b3f47026bdd7f7c0/src/edwards.rs#L269).

@pinkforest seems like it'd be good to have some serde tests / this should go over dalek-cryptography#289?

Co-Authored-By: Vlad Semenov <semenov.vlad.by@gmail.com>
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.

3 participants