Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Upgrade from futures-preview to futures 0.3.1, and remove futures 0.1 where currently possible #4083

Merged
merged 23 commits into from
Nov 22, 2019

Conversation

expenses
Copy link
Contributor

As #3099 mentions, we want to switch to stable futures. While we are still blocked in several places (tokio 0.2, jsonrpc, libp2p etc), we can switch from the preview branch of futures to the proper 0.3.1, and get away from futures 0.1 in some places.

Note that in order to stabilise futures-timer, the Interval stream was moved into async-std under the unstable feature (as the stream api hasn't been stabilised yet). This isn't great, as it adds more dependencies than we really need. Perhaps it would be best to move Interval into our own lib.

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm

@tomaka
Copy link
Contributor

tomaka commented Nov 11, 2019

This isn't great, as it adds more dependencies than we really need.

I'm not so worried about adding dependencies, but more about the fact that it's behind the unstable feature flag, meaning that it can break at any time.

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

I'm kind of undecided about async-std. Maybe we should depend on =0.99.12 rather than just 0.99.12.
In practice, though, the chances of Interval breaking are fairly low.

core/rpc/Cargo.toml Outdated Show resolved Hide resolved
@@ -102,7 +101,7 @@ pub struct VersionInfo {
/// Something that can be converted into an exit signal.
pub trait IntoExit {
/// Exit signal type.
type Exit: Future<Item=(),Error=()> + Send + 'static;
type Exit: Future<Output=()> + Unpin + Send + 'static;
Copy link
Contributor

Choose a reason for hiding this comment

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

As a heads up, this breaks Polkadot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I'll make a PR for that then.

@tomaka
Copy link
Contributor

tomaka commented Nov 11, 2019

You might have to replace all the cargo web build -p ... in .gitlab-ci.yml with cargo build -p ... --target=wasm32-unknown-unknown in order to make it work 😬

@tomaka
Copy link
Contributor

tomaka commented Nov 13, 2019

async-std unfortunately doesn't compile for wasm32-unknown-unknown 😬
cc async-rs/async-std#220

@expenses
Copy link
Contributor Author

@tomaka ah ok. What should I do here?

core/network/src/debug_info.rs Outdated Show resolved Hide resolved
core/network/src/protocol.rs Outdated Show resolved Hide resolved
@gavofyork
Copy link
Member

@tomaka is the webwasm stuff something that can/should be fixed in this PR?

@tomaka
Copy link
Contributor

tomaka commented Nov 19, 2019

The WASM CI is probably failing because there's still a dependency on async-std which can be removed by using stream::unfold rather than async_std::Interval (as explained above).

@gavofyork
Copy link
Member

ok - @expenses you ok doing that?

@expenses
Copy link
Contributor Author

Yep!

@@ -68,6 +67,8 @@ use prost::Message;
use sr_primitives::generic::BlockId;
use sr_primitives::traits::{Block as BlockT, ProvideRuntimeApi};

type Interval = Box<dyn Stream<Item = ()> + Unpin>;
Copy link
Contributor Author

@expenses expenses Nov 19, 2019

Choose a reason for hiding this comment

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

I think this is preferable to stream::Unfold<(), ...>, but not ideal

@expenses
Copy link
Contributor Author

@tomaka this should be ready now

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Approving for the third time :)

@gavofyork gavofyork merged commit aee12ee into paritytech:master Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants