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

Upgrade to tokio 1.0 #142

Merged
merged 5 commits into from
Jan 9, 2021
Merged

Upgrade to tokio 1.0 #142

merged 5 commits into from
Jan 9, 2021

Conversation

dnaka91
Copy link
Contributor

@dnaka91 dnaka91 commented Dec 24, 2020

Tokio 1.0 has just been released and PR updates to the latest release.

This depends on:

I will update this PR as the dependencies become ready.

Currently waiting on new release of the following crates to mark this PR as ready:

@hansihe
Copy link

hansihe commented Dec 30, 2020

Now that tokio has reached 1.0, I think it would make sense to specify the dependency with a caret ^1.0.0 instead of exact like done in the PR?

This is especially important since 1.0.1 has already been released.

Also, is there any timetime on a release, and is there anything I can do to help out?

@paolobarbolini
Copy link

Now that tokio has reached 1.0, I think it would make sense to specify the dependency with a caret ^1.0.0 instead of exact like done in the PR?

It's the same thing

@hansihe
Copy link

hansihe commented Dec 30, 2020

It's the same thing

Ah, TIL, thanks!

Copy link

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

The tungstenite-rs dependency needs updating, since nickelc's branch was merged into snapview and then removed.

Cargo.toml Outdated Show resolved Hide resolved
gakonst added a commit to gakonst/ethers-rs that referenced this pull request Dec 31, 2020
gakonst added a commit to gakonst/ethers-rs that referenced this pull request Dec 31, 2020
gakonst added a commit to gakonst/ethers-rs that referenced this pull request Dec 31, 2020
gakonst added a commit to gakonst/ethers-rs that referenced this pull request Dec 31, 2020
* feat(providers): tokio 1.0

BREAKING: This removes async-std as a compatibility option

* feat: tokio 1.0 in rest of crates

* fix: patch Cargo.toml until deps are released

* fix(contract): load ws deps

* feat: bytes 1.0 (#121)

* feat(core): move to bytes::Bytes

* feat: adjust rest of crates to Bytes

* chore: bump deps

CI fails due to:
snapview/tokio-tungstenite#142 (comment)

* chore: use latest tokio-tungstenite

* ci: split tests into jobs (#129)

* Switch to `hex` (#128)

* fix(core): replace rustc_hex with hex

* fix(providers): replace rustc_hex with hex

* chore: replace rustc-hex with hex

* chore: cargo fmt

* fix(ledger): copy address from string correctly

* chore: fix flaky tests

Fixes #105
@coolreader18
Copy link

Is there anything that's still blocking this?

@dnaka91
Copy link
Contributor Author

dnaka91 commented Jan 4, 2021

@coolreader18 currently just waiting for the tungstenite and input_buffer crates to release a new version.

This is due to input_buffer having a dependency on bytes 0.5 and the new tokio depends on 1.0.

Technically this could be merged and released (removing the current git reference for the tungstenite dependency). That would cause crates to build with bytes 0.5 AND 1.0 as this crate would still use the current input_buffee with 0.5 and tokio would use 1.0. that is okay because input_buffer only uses it internally but bloats the library and it's always good to avoid compiling in multiple versions of the same crate.

In the end it's up to the guys from snapview. Either release new versions of input_buffer and tungstenite first and then merge this, or merge now and do the upgrade of tungstenite later (eventually even before the next release).

So far I was just planning to keep this as draft until the new versions are out.

@strohel
Copy link
Contributor

strohel commented Jan 4, 2021

Hi @dnaka91 and others, thanks for all the work!

Let's see if we can release input_buffer and tungstenite soon so that this can reference released versions.

@ry
Copy link

ry commented Jan 7, 2021

Thanks for this work!

In Deno, tokio-tungstenite is the last dependency we are waiting on to upgrade to Tokio 1.0. Happy to lend a hand if there's any specific work needed but it sounds like you all are just waiting on input_buffer tho?

@dnaka91
Copy link
Contributor Author

dnaka91 commented Jan 7, 2021

Hi @ry, yeah just waiting for the releases of input_buffer and tungstenite, that's all. I added them to the PR description so people can see what is currently missing for this to be ready for merging.

@extrawurst
Copy link

input_buffer changed its dependency already 9days ago, it is just not released yet: snapview/input_buffer#5

@Owez
Copy link

Owez commented Jan 8, 2021

I've asked about tungstenite-rs as there currently aren't any issues/prs I can see unless they where hidden in a unreleased commit somewhere: snapview/tungstenite-rs#169 (not a contrib to tokio-tungstenite or tungstenite-rs myself, just a user of warp)

@dnaka91
Copy link
Contributor Author

dnaka91 commented Jan 8, 2021

inbut_buffer 0.4 was released today (https://crates.io/crates/input_buffer/0.4.0). Now just waiting on the tungstenite-rs release. Thanks for opening an issue to push things @Owez 🙇

@daniel-abramov daniel-abramov marked this pull request as ready for review January 8, 2021 12:27
@dnaka91
Copy link
Contributor Author

dnaka91 commented Jan 8, 2021

Thank you for the release of tungstenite 0.12 and quick update of the dependency here @application-developer-DA 🙇
Guess we're ready for merge then.

Copy link
Contributor

@strohel strohel left a comment

Choose a reason for hiding this comment

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

The code looks good to me. I'd appreciate somebody else also has a look at the changes in examples/interval-server.rs as I'm not proficient with https://docs.rs/tokio/1.0.1/tokio/macro.select.html

@strohel strohel requested review from daniel-abramov and jxs January 8, 2021 14:22
@udoprog
Copy link
Contributor

udoprog commented Jan 8, 2021

The code looks good to me. I'd appreciate somebody else also has a look at the changes in examples/interval-server.rs as I'm not proficient with https://docs.rs/tokio/1.0.1/tokio/macro.select.html

FWIW. I can confirm that the use of tokio::select! in interval-server.rs looks correct.

@@ -21,10 +21,10 @@ stream = []
log = "0.4"
futures-util = { version = "0.3", default-features = false, features = ["async-await", "sink", "std"] }
pin-project = "1.0"
tokio = { version = "0.3", default-features = false, features = ["io-util"] }
tokio = { version = "1.0.0", default-features = false, features = ["io-util"] }
Copy link

Choose a reason for hiding this comment

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

Is there any reason for pinning to 1.0.0. Why can't it just be 1.0?

Copy link

Choose a reason for hiding this comment

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

If semantic versioning is used properly for this, which it should be, couldn't this even be "1" as tokio is stable on 1.x.x?

Copy link

Choose a reason for hiding this comment

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

That's a valid point. I mostly also specify the minor version in my apps so i didn't think of that. So, the tokio version here should just be 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, per https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#caret-requirements 1, 1.0 and 1.0.0 are all equivalent.

Then it is a question of style/consistency. 1.0 would be most consistent locally (see line above). Is anybody aware of an ecosystem-wide style recommendation for equivalent version declarations in Cargo.toml?

Copy link

Choose a reason for hiding this comment

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

All I remember is Cargo endorses semantic versioning and the major version thats 1+.x.x won't have any breaking changes. It's fine to use 1.0 as well for consistency and in case of local bugfixes that get added/removed due to tokio in this repo as tokio is developed/patched

Copy link
Contributor Author

@dnaka91 dnaka91 Jan 8, 2021

Choose a reason for hiding this comment

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

As @strohel mentioned 1.0.0 is equivalent to 1.x.x. the only exception to this rule are 0.x.x releases. For example 0.1.0 would be equivalent to 0.1.x.

I personally prefer the full version as it kind of is a statement to what version the project was tested with by the authors and considered to be working. So that later when an error happens only on the user side and the user states that he is using for example the auto upgraded 1.5.3 instead of 1.0.0, then it's easy to derive that the error lies in some changes of the said dependency. Or at least would be likely and a good thing to look at first.

In the end it's up to you folks from snapview. I just wanted to explain my reasoning behind putting the full version in the Cargo.toml and don't mind changing to just 1.0 or 1.

Choose a reason for hiding this comment

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

It's not pinned, this is equivalent to ^1.0.0 (docs) and will allow updates to any 1.x.x.

Copy link

Choose a reason for hiding this comment

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

It's not pinned, this is equivalent to ^1.0.0 (docs) and will allow updates to any 1.x.x.

TIL. Thanks for informing me

@daniel-abramov daniel-abramov merged commit d3ae6c6 into snapview:master Jan 9, 2021
@dnaka91 dnaka91 deleted the tokio-1.0 branch January 9, 2021 16:21
jsLover1117 added a commit to jsLover1117/ethers-rs that referenced this pull request Jul 17, 2022
* feat(providers): tokio 1.0

BREAKING: This removes async-std as a compatibility option

* feat: tokio 1.0 in rest of crates

* fix: patch Cargo.toml until deps are released

* fix(contract): load ws deps

* feat: bytes 1.0 (#121)

* feat(core): move to bytes::Bytes

* feat: adjust rest of crates to Bytes

* chore: bump deps

CI fails due to:
snapview/tokio-tungstenite#142 (comment)

* chore: use latest tokio-tungstenite

* ci: split tests into jobs (#129)

* Switch to `hex` (#128)

* fix(core): replace rustc_hex with hex

* fix(providers): replace rustc_hex with hex

* chore: replace rustc-hex with hex

* chore: cargo fmt

* fix(ledger): copy address from string correctly

* chore: fix flaky tests

Fixes #105
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.