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

Update the service to std futures #4447

Merged
merged 40 commits into from
Jan 14, 2020
Merged

Update the service to std futures #4447

merged 40 commits into from
Jan 14, 2020

Conversation

expenses
Copy link
Contributor

@expenses expenses commented Dec 18, 2019 •
edited
Loading

Part of #3099.

@expenses expenses changed the title Update sc-service to 0.3 futures Update the service to std futures Dec 18, 2019
@expenses expenses marked this pull request as ready for review December 19, 2019 11:35
@expenses expenses requested a review from kianenigma December 19, 2019 13:57
@expenses expenses added the A0-please_review Pull request needs code review. label Dec 21, 2019
Demi-Marie
Demi-Marie approved these changes Dec 31, 2019
View reviewed changes
Copy link
Contributor

@Demi-Marie Demi-Marie 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

tomaka
tomaka suggested changes Jan 3, 2020
View reviewed changes
client/cli/src/informant.rs Outdated Show resolved Hide resolved
client/consensus/aura/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/chain_ops.rs Outdated Show resolved Hide resolved
client/service/src/chain_ops.rs Outdated Show resolved Hide resolved
client/service/src/chain_ops.rs Outdated Show resolved Hide resolved
client/service/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/lib.rs Show resolved Hide resolved
client/service/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/status_sinks.rs Outdated Show resolved Hide resolved
@expenses expenses requested a review from tomaka January 3, 2020 15:22
tomaka
tomaka suggested changes Jan 3, 2020
View reviewed changes
client/consensus/aura/src/lib.rs Outdated Show resolved Hide resolved
client/consensus/babe/src/tests.rs Outdated Show resolved Hide resolved
@expenses
Copy link
Contributor Author

expenses commented Jan 8, 2020

cargo test -p node-cli --release --verbose --locked -- --ignored does eventually pass on my machine, so I'm not entirely sure what to do here.

@bkchr
Copy link
Member

bkchr commented Jan 9, 2020

cargo test -p node-cli --release --verbose --locked -- --ignored does eventually pass on my machine, so I'm not entirely sure what to do here.

Modify the CI script to print the messages directly.

@gavofyork
Copy link
Member

conflicts and CI failing.

@gavofyork gavofyork added A7-looksgoodcantmerge and removed A0-please_review Pull request needs code review. labels Jan 13, 2020
@expenses expenses merged commit 410ce11 into master Jan 14, 2020
@expenses expenses deleted the ashley-service-futures branch January 14, 2020 14:43
tomaka
tomaka suggested changes Jan 14, 2020
View reviewed changes
client/service/src/lib.rs
@@ -216,11 +210,11 @@ impl<TBl, TBackend, TExec, TRtApi, TSc, TNetSpec, TExPool, TOc> AbstractService
Service<TBl, Client<TBackend, TExec, TBl, TRtApi>, TSc, NetworkStatus<TBl>,
NetworkService<TBl, TNetSpec, TBl::Hash>, TExPool, TOc>
where
TBl: BlockT,
TBl: BlockT + Unpin,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong.

client/service/src/lib.rs
@@ -309,57 +304,56 @@ where
}
}

impl<TBl, TCl, TSc, TNetStatus, TNet, TTxPool, TOc> Future for
impl<TBl: Unpin, TCl, TSc: Unpin, TNetStatus, TNet, TTxPool, TOc> Future for
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
impl<TBl: Unpin, TCl, TSc: Unpin, TNetStatus, TNet, TTxPool, TOc> Future for
impl<TBl, TCl, TSc, TNetStatus, TNet, TTxPool, TOc> Future for

client/service/src/lib.rs
TBackend: 'static + sc_client_api::backend::Backend<TBl>,
TExec: 'static + sc_client::CallExecutor<TBl> + Send + Sync + Clone,
TRtApi: 'static + Send + Sync,
TSc: sp_consensus::SelectChain<TBl> + 'static + Clone + Send,
TSc: sp_consensus::SelectChain<TBl> + 'static + Clone + Send + Unpin,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TSc: sp_consensus::SelectChain<TBl> + 'static + Clone + Send + Unpin,
TSc: sp_consensus::SelectChain<TBl> + 'static + Clone + Send,

client/service/src/lib.rs
}
}

while let Ok(Async::Ready(Some(task_to_spawn))) = self.to_spawn_rx.poll() {
while let Poll::Ready(Some(task_to_spawn)) = Pin::new(&mut this.to_spawn_rx).poll_next(cx) {
// TODO: Update to tokio 0.2 when libp2p get switched to std futures (#4383)
Copy link
Contributor

Choose a reason for hiding this comment

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

Libp2p has been switched to std futures.

@tomaka
Copy link
Contributor

tomaka commented Jan 14, 2020

When you push 17 more commits after a previous review, I'd suggest to ask for a new review instead of just merging.

This was referenced Jan 15, 2020
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tomaka tomaka tomaka requested changes

@Demi-Marie Demi-Marie Demi-Marie approved these changes

@andresilva andresilva Awaiting requested review from andresilva

@kianenigma kianenigma Awaiting requested review from kianenigma

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@expenses @bkchr @gavofyork @tomaka @Demi-Marie @gui1117