Skip to content

Support stable async-std #7

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

Merged
merged 2 commits into from
Feb 5, 2021
Merged

Support stable async-std #7

merged 2 commits into from
Feb 5, 2021

Conversation

flub
Copy link
Contributor

@flub flub commented Jan 24, 2021

Since async-std 1.8 the API has moved and is considered stable. Since
async-std 1.9 the old API has been removed. This allows both versions
to still work, depending on whether this crate is built with the
"unstable" feature or not.

Unfortunately I'm not sure how to run the test for both cases. There might be some cargo gymnastics that I'm not aware off but to test the unstable feature you need to limit async-std to < 1.9 somehow, but not do that otherwise.

@flub flub mentioned this pull request Jan 24, 2021
@yoshuawuyts
Copy link
Collaborator

Hey @flub, thanks for opening this! -- I think we can safely drop the pre-1.9 API and only add support for the new stable API. Once we have that we can release [email protected] which will only work with the new, stabilized API.

Could you update this PR to reflect that? -- thanks!

Since async-std 1.8 the API has moved and is considered stable.  Since
async-std 1.9 the old API has been removed.  This adapts stop-token to
this new API and drops support for async-std <1.8.
@flub
Copy link
Contributor Author

flub commented Jan 29, 2021

Seems very reasonable, I updated the PR and squashed it back into just a single commit since the history didn't add much value.

Now that the async-std dependency is newer it makes sense to match the
pin-project-lite version of it, so that a duplicate copy is does not
need to be compiled in.
Copy link
Collaborator

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This looks great; thanks heaps!

@yoshuawuyts yoshuawuyts merged commit 7e42721 into async-rs:master Feb 5, 2021
@flub
Copy link
Contributor Author

flub commented Feb 5, 2021

thanks for pushing a new release!

@@ -1,36 +1,40 @@
use std::time::Duration;

use async_std::{prelude::*, task, sync::channel};
use async_std::prelude::*;

Choose a reason for hiding this comment

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

@flub I was wondering why the change in use syntax here? Is it not better to use {} syntax to make all these imports on 1 line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mixing multiple levels of of imports quickly becomes confusing to me, that is i avoid having to use :: inside { } and nesting braces for imports. fewer lines isn't always more readable. but this doesn't matter much does it?

Choose a reason for hiding this comment

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

No, just a style question and I was simply curious.

Choose a reason for hiding this comment

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

the downside to this however is that it makes the diff harder to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you stick to that style all following diffs are more readable. If you don't every single non-trivial diff becomes harder to read. this is all bikeshedding of course and depends on personal taste :) i wish cargo-fmt had much stricter formatting rules around imports instead of allowing so many variations, it would avoid this bikeshedding.

[features]
unstable = ["async-std/unstable"]
pin-project-lite = "0.2.0"
async-std = "1.8"

Choose a reason for hiding this comment

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

I'm curious about this change. If I understand cargo dependency resolution, async-std = "1.0" is short for async-std="^1.0", which in this case would resolve to async-std="1.9" since: ^1 := >=1.0.0, <2.0.0. So I think this version bump is superfluous so semver will evaluate to the latest 1.x release, namely 1.9.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.8 is the earliest version this can support. setting it like this is equivalent to ^1.8 or >=1.8.0 <2.0.0. As this is a library that means users can themselves say for example async-std = "^1.18" in their own cargo.toml and this library will not force another duplicate version of async-std to be compiled in as that version is still compatible here.
If we would set this to ^1 then someone depending on this could say = 1.3.0 and we'd get that version here and it would break.

Choose a reason for hiding this comment

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

I think i'm missing something about dependency resolution here. When I set async-std="1.0" cargo downloads 1.9. I guess I don't understand how to tell cargo to use less than 1.9.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

your binary says async-std >= 1.0.0, < 2.0.0 and you also depend on stop-token which says async-std >= 1.8.0, < 2.0.0. As that's the only input to cargo's dependency resolver it decides it also prefers the most recent version and it resolves this to async-std 1.9.0 which satisfies all those constraints.

If you want cargo to use something older than 1.9 you need to tell it to do that in your Cargo.toml. Set async-std: <1.9 probably with some other constraints added. If you are asking for async-std < 1.8 but do ask for stop-token >= 0.2 then cargo will build 2 versions of async-std into your binary.

yancyribbens referenced this pull request in chatmail/async-imap Mar 22, 2021
Update async-std and stop-token dependencies, migrate to stable channels
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.

3 participants