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

Make Wasm support dependent on target_arch rather than feature #666

Merged
merged 14 commits into from
Feb 21, 2022

Conversation

PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Feb 19, 2022

Description of change

Essentially, this changes all cfg(feature = "wasm") occurrences to cfg(all(target_arch = "wasm32", not(target_os = "wasi"))) and removes the wasm feature from all crates. Since we only have two occurrences of those in code, most of this applies to various Cargo.tomls. The motivation is to enable Futures that contain an Account to be Send. This is not the case in current dev, because of iota_client::Client. An example that does not compile under current dev:

fn assert_future_send<T: std::future::Future + Send>(_: T) {}

let mut account: Account = AccountBuilder::default()
  .create_identity(IdentitySetup::default())
  .await?;

assert_future_send(account.update_identity().create_method().apply());

which fails with:

error: generator cannot be sent between threads safely
   --> identity-account/src/tests/account.rs:505:22
    |
505 |   assert_future_send(account.update_identity().create_method().apply());
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `apply` is not `Send`
    |
    = help: within `impl futures::Future<Output = std::result::Result<(), error::Error>>`, the trait `std::marker::Send` is not implemented for `std::sync::RwLockWriteGuard<'_, iota_client::builder::NetworkInfo>`
note: required by a bound in `tests::account::_assert_account_send::{closure#0}::assert_future_send`
   --> identity-account/src/tests/account.rs:499:50
    |
499 |   fn assert_future_send<T: std::future::Future + Send>(_: T) {}
    |                                                  ^^^^ required by this bound in `tests::account::_assert_account_send::{closure#0}::assert_future_send`

This compiles under this PR (partly because of changes in iota.rs/804, which this PR depends upon) but subsequent errors are of similar sort. This was added as a static assertion ("compile time test") to prevent a future regression.

Note that this only fails when running cargo check --all-features (which is what CI does), while omitting the wasm feature does not produce this error. The issue is that checking this assertion test under the wasm feature is not what we want, because in Wasm we don't require the future to be Send, but we do on other targets. Thus, moving conditional compilation for Wasm to target_arch rather than feature seems to be more in line with how cfgs are supposed to work.

It's noteworthy that this same type of fix could be used to fix iota.rs, which would not require us to make the change. However, for the just-mentioned reasons, it seems like an improvement to our crates, to prevent future issues target-feature inconsistencies that might not be related to iota.rs.

Important: This points iota.rs to a not-yet-merged commit, so this PR should only be merged after iota.rs/804 has been merged.

This is marked as a breaking change, since the wasm feature was removed from various crates.

This will require #597 to change it's cfg_attrs, and likely introduce a separate feature for non-Send-Storage (FYI @HenriqueNogara).

Unsure if this PR should also make use of the now-runtime configurable POW fallback on iota.rs, which the above mentioned PR changed.

Thanks to @cycraig for suggesting this idea initially!

Added

  • static assertion to ensure Futures containing an Account are Send

Changed

  • multiple_identities example to showcase concurrent updates to two accounts
  • GitHub workflows to only run cargo check --target wasm32-unknown-unknown for the Wasm bindings

Removed

  • Remove the unnecessary cargo test in the Wasm bindings which does not do anything because there are no tests. This still runs the wasm_bindgen_tests in CI through npm run test.

Links to any relevant issues

None

Type of change

Add an x to the boxes that are relevant to your changes.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

Describe the tests that you ran to verify your changes.
Make sure to provide instructions for the maintainer as well as any relevant configurations.

Change checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@PhilippGackstatter PhilippGackstatter added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Feb 19, 2022
@cycraig
Copy link
Contributor

cycraig commented Feb 19, 2022

Unsure if this PR should also make use of the now-runtime configurable POW fallback on iota.rs, which the above mentioned PR changed.

That should be a separate small PR, since it will change the builder options in Rust and Wasm too.

Will review properly once iotaledger/iota.rs#804 is merged. PR was merged, review below.

.github/workflows/clippy.yml Show resolved Hide resolved
bindings/wasm/src/common/timestamp.rs Outdated Show resolved Hide resolved
examples/account/multiple_identities.rs Show resolved Hide resolved
identity-account/src/tests/account.rs Outdated Show resolved Hide resolved
identity-comm/Cargo.toml Outdated Show resolved Hide resolved
identity-iota/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@olivereanderson olivereanderson left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the changes.

Comment on lines +71 to +72
task1.await.expect("task1 failed to execute to completion")?;
task2.await.expect("task2 failed to execute to completion")?;
Copy link
Contributor

@cycraig cycraig Feb 21, 2022

Choose a reason for hiding this comment

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

Apparently join is still suggested to await both tasks but it really doesn't matter since it doesn't affect the parallelism as you demonstrated.

https://docs.rs/tokio/latest/tokio/macro.join.html#runtime-characteristics

If parallelism is required, spawn each async expression using tokio::spawn and pass the join handle to join!.

No changes requested, just wanted to drop a link to the quote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants