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

Include Tokio version in "no such executor" for Tokio 0.2.x and 0.3.x #3442

Closed
Darksonn opened this issue Jan 17, 2021 · 9 comments
Closed
Labels
A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime T-v0.2.x Topic: tokio 0.2.x T-v0.3.x Topic: tokio 0.3.x

Comments

@Darksonn
Copy link
Contributor

Darksonn commented Jan 17, 2021

A very common question is why they get an error saying there is no Tokio runtime when people use libraries that depend on old Tokio versions. We should update the 0.2 and 0.3 error message to include the version number.

@Darksonn Darksonn added E-help-wanted Call for participation: Help is requested to fix this issue. E-easy Call for participation: Experience needed to fix: Easy / not much A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime T-v0.2.x Topic: tokio 0.2.x T-v0.3.x Topic: tokio 0.3.x labels Jan 17, 2021
@dekellum
Copy link
Contributor

dekellum commented Jan 22, 2021

Seems like this can be done by backporting most of #3441 that is relevant to the respective branches? Perhaps, the error message is improved by taking advantage of CARGO_PKG_VERSION (the exact version in tokio/Cargo.toml):

 /// Error string explaining that the Tokio context hasn't been instantiated.
-pub(crate) const CONTEXT_MISSING_ERROR: &str =
-    "there is no reactor running, must be called from the context of a Tokio 1.x runtime";
+pub(crate) const CONTEXT_MISSING_ERROR: &str = concat!(
+    "there is no reactor running, must be called from the context of the Tokio ",
+    env!("CARGO_PKG_VERSION"),
+    " runtime!"
+);

Note: "1.x" does imply the appropriate semver compatible range [1.0.0, 2), but mentioning the exact version might give users a better clue?

Or is it better for 0.2.x to keep its existing shorter ""not currently running on the Tokio runtime." style message (with only the version added)?

If we agree, I can PR the above (with some slightly tricky associated test changes) against master/1.1.

Then the bulk of the work (for which I'm not sure qualifies as label: E-easy) becomes reviving CI on these branches, 0.2.x in particular made more difficult without a checked in Cargo.lock (that I can find).

@dekellum
Copy link
Contributor

cc @aknuds1 the author of #3441.

@Darksonn
Copy link
Contributor Author

Darksonn commented Jan 22, 2021

Using 1.x rather than CARGO_PKG_VERSION was chosen specifically to make the semver compatibility range clear. I think that for 0.2.x and 0.3.x it is fine to just add the version to the existing panic messages.

The CI setup on 0.2.x and 0.3.x should work now without changes unless it has been broken by an upgraded non-pinned clippy version. In case this happens, you can just pin the clippy version used by CI. We do not check in Cargo.lock because Tokio is a library, not a binary, and Tokio is supposed to work with any semver-compatible version of its dependencies.

@dekellum
Copy link
Contributor

Okay, I won't PR that. 🙂

In looking at 0.2.x very briefly, it looked like there was an MSRV bump in a tokio (all features) dependency beyond its CI'd MSRV. If there was a Cargo.lock that could be avoided, while an explicit cargo update step could still test full semver dep exposure.

But I don't think I'll PR that either. Thanks for your comment and consideration.

@Darksonn
Copy link
Contributor Author

In looking at 0.2.x very briefly, it looked like there was an MSRV bump in a tokio (all features) dependency beyond its CI'd MSRV.

Which dependency are you referring to? Both PRs passed the minrust check.

@dekellum
Copy link
Contributor

dekellum commented Jan 22, 2021

Congrats! I'm happy it wasn't as bad as I feared it would be.

Like I said, I only looked at it briefly before commenting here, but I had to:

cargo update -p tracing-subscriber --precise 0.2.12` 

in order to get:

cargo +1.39.0 test --all-feature

to succeed. Guessing your current CI just does cargo check on the MSRV? At least that's another alternative workaround I've seen, like Cargo.lock, or explicitly downgrading in CI, but at the expense of not actually testing.

@Darksonn
Copy link
Contributor Author

The test suite isn't subject to the MSRV.

@dekellum
Copy link
Contributor

Well then, obviously, you can't succeed at testing it on MSRV, then, can you. You reap what you sow.

(No offense intended, I'm only explaining why some like myself choose to check in the Cargo.lock to libraries and cargo test on MSRV. Since you asked.)

@Darksonn Darksonn removed E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue. labels Jan 25, 2021
@taiki-e
Copy link
Member

taiki-e commented Mar 27, 2021

done in #3460 & #3461

@taiki-e taiki-e closed this as completed Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime T-v0.2.x Topic: tokio 0.2.x T-v0.3.x Topic: tokio 0.3.x
Projects
None yet
Development

No branches or pull requests

3 participants