Cleanup of conditional dependencies to support a wider range of compilation targets #138

arnauorriols · 2021-08-14T12:55:32Z

Description

During preparation of arnauorriols/iota-streams-esp32, several changes have had to be done in Streams to be able to run it in the ESP32 target, with and without std feature enabled. This PR includes all such changes. The following is the list of the most relevant:

Make wasm-bindgen feature of rand only enabled when wasm-client feature of app-channels is enabled (it was always enabled)
Derive Debug in a bunch of types, notably BucketTransport, TangleMessage and Keccak1600
Get rid of chrono dependency. I've taken the assumption that the timestamp in TangleMessage was a leftover
from pre-chrysalis, so I've removed it completely. Now TangleMessage<F> is an alias of BinaryMessage<F, TangleAddress>
Make futures dependency pulled only when a *-client feature is enabled (it was always enabled)
Use StdRng::from_entropy() instead of thread_rng when target_os == espidf

Type of change

Enhancement (a non-breaking change which adds functionality)

How the change has been tested

cargo check --no-default-features && cargo check && cargo check --all-features
cargo clippy --no-default-features && cargo clippy && cargo clippy --all-features
run arnauorriols/iota-streams-esp32 example
run examples
run wasm examples
run c examples
run no-std examples

Change checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
New and existing unit tests pass locally with my changes

`chrono` is conflictive with embedded scenarios, as relies on `libc` functions that are not supported on all architectures, even if libstd is available for those architectures. This commit takes the assumption that the timestamp present in the type `TangleMessage` is a leftover from pre-chrysalis. It's the only place where `chrono` is needed. Therefore, the following actions have been taken: - Convert `TangleMessage<F>` into just a type alias of `BinaryMessage<F, TangleAddress>` - Remove `chrono` from `streams-app` dependencies

espidf targets have libstd available through ESP-IDF framework. This framework provides most but not all of the POSIX standard API. In particular, ESP-IDF does not provide a means to fork threads, and cannot implement `pthread_atfork` (see espressif/rust-esp32-example#23). This means `rand::thread_rng()` is not available on those targets, as it is based on storing the rng in the thread-local storage, accessed through a thread handle. For these targets, this commit specializes the random_key and random_nonce generators to use `StdRng::from_entropy()` instead. In practice, given the coldness of these 2 functions, the same level of security and performance is to be expected. Note that currently `getrandom` does not support `espidf` targets and requires a small patch.

sascha1337 · 2021-08-19T16:20:41Z

those 2 commits are causing problems with the node.js bindings -

Derive Clone, Debug, PartialEq and Eq for MessageContent
016f61c

Fix unused warning for prng::rng when no_std in x25519 wrap command
3dffdf6
again the mio that complains ( and its dependency chain if i remember correctly, had the exact same stuff X times )..

Here's some uncompleted fork, that tried to get the lib working with WASI / WASM, but discontinued efforts:
Thomasdezeeuw/mio@2a792af

This might be interesting too, something with the core cargo toml dependencies + features declaration is doing weird things, especially with tokio / mio, maybe some helpful hint inside here:

rust-lang/cargo#9176

I remember actually fixing this issue by tweaking around with the dependencies, overwriting / patching some with that [patch] toml syntax, downgrading a few crates, and using the new feature resolver v2 function on the rust nightly channel ..

https://doc.rust-lang.org/nightly/cargo/reference/features.html#feature-resolver-version-2

But with so many different tries and outdated clones of the streams repo, i cannot remember which exact changes fixed that following build error, that keeps hunting us again and again :D @jlvandenhout

deso@nuc /tmp/foo (master *) $ cargo build --target wasm32-unknown-unknown
   Compiling mio v0.7.7
error[E0432]: unresolved import `crate::sys::IoSourceState`
  --> /tmp/cargo-home/registry/src/github.com-1ecc6299db9ec823/mio-0.7.7/src/io_source.rs:12:5
   |
12 | use crate::sys::IoSourceState;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ no `IoSourceState` in `sys`

Otherwise, +1 for your no-std efforts, can't wait to spin up my xargo / cargo xtensa llvm esp32 custom target ^^
Hella fun to completely use cargo instead of pio / arduino / idf.py / esptool.py to build, flash and serial monitor code on ESPs

cargo espflash --chip esp32 --example esp32 --speed 115200 --release --features="xtensa-lx-rt/lx6,xtensa-lx/lx6,esp32-hal" /dev/tty.usbserial-0001

Ive been using this nice crate for it, ive got the Qemu installed but i tend to run it directly on the hardware

cargo install cargo-espflash

arnauorriols · 2021-10-10T19:43:36Z

Hey @sascha1337 sorry for the late reply! We were focused on releasing 1.2.0 and this PR got backburnered. I've sync'ed this branch with develop, and the wasm build works now.

arnauorriols · 2021-10-19T15:18:32Z

iota-streams-app/Cargo.toml

 anyhow = { version = "1.0.26", default-features = false }
-chrono = { version = "0.4.11", default-features = false, optional = true }


Now the reasons for removing chrono raised to a whole new level: #179
#181

chrono ... wasm... tokio... the never ending loop :(

kwek20

About the timestamp removal. Im all up for it, but when we add it back later (I believe in stardust we get one again?) what is the plan?
Or will we be timestamp agnostic, and let the user use iota.rs to fetch timestamps?

kwek20 · 2022-01-13T01:23:07Z

iota-streams-app/Cargo.toml

@@ -11,14 +11,14 @@ description = "A rust implementation of the IOTA Streams applications"
 [features]
 default = ["std", "client"]
 # Enable `std` feature in dependencies
-std = ["iota-streams-core/std", "iota-streams-core-edsig/std", "iota-streams-ddml/std", "chrono/std", "chrono/clock", "hex/std"]
+std = ["iota-streams-core/std", "iota-streams-core-edsig/std", "iota-streams-ddml/std", "hex/std"]


We should add rand/std as well, otherwise rand doesnt support it with default-features = false. To be correct :)

Well, actually, you helped me realize that rand does not need to be a dependency on iota-streams-app. As per rand/std, in iota-streams-core it is already done as you suggest: https://github.com/iotaledger/streams/pull/138/files#diff-3350ed2612e4263878a1a2e5ba8025625b76c0ec06cc80b7211f586991da47f3R14

arnauorriols · 2022-01-25T09:35:58Z

About the timestamp removal. Im all up for it, but when we add it back later (I believe in stardust we get one again?) what is the plan? Or will we be timestamp agnostic, and let the user use iota.rs to fetch timestamps?

Not sure about Stardust, but definitely will in Coordicide. What I'm not convinced of is if this timestamp will be relevant from Streams point of view... Will Streams use the timestamp of the TX for any logic? what will the semantics of this timestamp be? will it be relevant for the subscriber of the Stream?

In any case, in the present this timestamp is not there, and it is not used, and just adds up to the general confusion. That's why I propose to remove it and deal with it once it makes sense

instead of iota-streams-app/Cargo.toml

DyrellC · 2022-01-25T16:48:58Z

Will Streams use the timestamp of the TX for any logic? what will the semantics of this timestamp be? will it be relevant for the subscriber of the Stream?

I think it's safe to say that as we currently have it implemented it really doesn't provide much benefit. The only way I could see the timestamp being relevant/beneficial is if it was added to the header so that we can sort multiple messages at a given index by this timestamp, which arguably we shouldn't need to do because (I'm not 100% sure on the stardust side specifically but in corrdicide yes) will contain their own timestamping process that we should be able to use since the timestamps will be embedded into the Tangle Message anyways.

DyrellC

I see no issues with this one, I think we're good to merge it.

arnauorriols added 11 commits August 11, 2021 01:03

Use core::fmt instead of std::fmt in certain leftover places

556bd7c

Use map_err explicitly instead of relying on ?

48174bb

use rand/wasm-bindgen only when wasm-client feature is enabled

6e86b24

Derive Debug in Bucket

0575f89

Derive Debug in Message and Keccak types

f0b7116

Make futures dependency part of client features

8867b54

Delegate selection of rng to core::prng::rng() in x25519 DDML command

3ab0cd7

Derive Clone, Debug, PartialEq and Eq for MessageContent

016f61c

Fix unused warning for prng::rng when no_std in x25519 wrap command

3dffdf6

Merge branch 'develop' into no-std

6d1260a

Arnau Orriols added 2 commits October 11, 2021 03:09

import RefCell and PhantomData from core instead of std

df7eb91

Stop reexporting futures from iota-streams-app

806f057

arnauorriols force-pushed the no-std branch from 73052d9 to 806f057 Compare October 11, 2021 01:10

arnauorriols mentioned this pull request Oct 14, 2021

Various cleanups and niceties #150

Merged

3 tasks

arnauorriols commented Oct 19, 2021

View reviewed changes

arnauorriols requested review from kwek20 and DyrellC October 28, 2021 22:32

kwek20 approved these changes Jan 13, 2022

View reviewed changes

Merge branch 'develop' into no-std

7489a03

arnauorriols changed the title ~~Support ESP32 devices with and without std feature~~ Cleanup of conditional dependencies to support a wider range of compilation targets Jan 25, 2022

Move rand/wasm-bindgen feature dependency to bindings/wasm/Cargo.toml

acb97b4

instead of iota-streams-app/Cargo.toml

arnauorriols force-pushed the no-std branch from eff7e4d to acb97b4 Compare January 25, 2022 14:32

Merge branch 'develop' into no-std

23faaef

DyrellC approved these changes Feb 2, 2022

View reviewed changes

DyrellC merged commit 872c370 into iotaledger:develop Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup of conditional dependencies to support a wider range of compilation targets #138

Cleanup of conditional dependencies to support a wider range of compilation targets #138

arnauorriols commented Aug 14, 2021 •

edited

Loading

sascha1337 commented Aug 19, 2021

arnauorriols commented Oct 10, 2021

arnauorriols Oct 19, 2021

sascha1337 Oct 30, 2021

kwek20 left a comment

kwek20 Jan 13, 2022

arnauorriols Jan 25, 2022

arnauorriols commented Jan 25, 2022

DyrellC commented Jan 25, 2022

DyrellC left a comment

		anyhow = { version = "1.0.26", default-features = false }
		chrono = { version = "0.4.11", default-features = false, optional = true }

Cleanup of conditional dependencies to support a wider range of compilation targets #138

Cleanup of conditional dependencies to support a wider range of compilation targets #138

Conversation

arnauorriols commented Aug 14, 2021 • edited Loading

Description

Type of change

How the change has been tested

Change checklist

sascha1337 commented Aug 19, 2021

arnauorriols commented Oct 10, 2021

arnauorriols Oct 19, 2021

Choose a reason for hiding this comment

sascha1337 Oct 30, 2021

Choose a reason for hiding this comment

kwek20 left a comment

Choose a reason for hiding this comment

kwek20 Jan 13, 2022

Choose a reason for hiding this comment

arnauorriols Jan 25, 2022

Choose a reason for hiding this comment

arnauorriols commented Jan 25, 2022

DyrellC commented Jan 25, 2022

DyrellC left a comment

Choose a reason for hiding this comment

arnauorriols commented Aug 14, 2021 •

edited

Loading