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

Stage 0.3.0 release #1954

Merged
merged 1 commit into from Nov 6, 2019
Merged

Stage 0.3.0 release #1954

merged 1 commit into from Nov 6, 2019

Conversation

cramertj
Copy link
Member

@cramertj cramertj commented Nov 5, 2019

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Nov 5, 2019

Running a test now on async-std to check if anything is broken. Will update this comment to report back.

edit: yes everything seems to pass locally 🎉. Filed async-rs/async-std#460 to test the latest master on CI once too.

@cramertj
Copy link
Member Author

cramertj commented Nov 5, 2019

@yoshuawuyts Thanks for helping review! <3

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Nov 6, 2019

async-rs/async-std#460 passed on CI, which means that as far as async-std is concerned this patch should be good to go!

@cramertj @taiki-e @Nemo157 thanks so much for the work you've put in to get this release out!

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Could you replace all "=0.3.0" with "0.3.0"? (I think "=" requirements are no longer necessary.)

@taiki-e
Copy link
Member

taiki-e commented Nov 6, 2019

There is a problem trying to update in tokio.
commit: tokio-rs/tokio@fa71a58

error: implementation of `core::future::future::Future` is not general enough
   --> examples/connect.rs:31:5
    |
31  |       tokio::spawn(async move {
    |       ^^^^^^^^^^^^ implementation of `core::future::future::Future` is not general enough
    |
    = note: `futures_util::sink::send_all::SendAll<'0, tokio_util::codec::framed_write::FramedWrite<tokio::io::stdout::Stdout, codec::Bytes>, futures_util::stream::stream::map::Map<futures_util::stream::stream::filter_map::FilterMap<tokio_util::codec::framed_read::FramedRead<tokio::net::tcp::split::ReadHalf<'1>, codec::Bytes>, futures_util::future::ready::Ready<std::option::Option<std::vec::Vec<u8>>>, [closure@examples/connect.rs:99:25: 105:14]>, fn(std::vec::Vec<u8>) -> std::result::Result<std::vec::Vec<u8>, std::io::Error> {std::result::Result::<std::vec::Vec<u8>, std::io::Error>::Ok}>>` must implement `core::future::future::Future`, for any two lifetimes `'0` and `'1`...
    = note: ...but `core::future::future::Future` is actually implemented for the type `futures_util::sink::send_all::SendAll<'_, tokio_util::codec::framed_write::FramedWrite<tokio::io::stdout::Stdout, codec::Bytes>, futures_util::stream::stream::map::Map<futures_util::stream::stream::filter_map::FilterMap<tokio_util::codec::framed_read::FramedRead<tokio::net::tcp::split::ReadHalf<'_>, codec::Bytes>, futures_util::future::ready::Ready<std::option::Option<std::vec::Vec<u8>>>, [closure@examples/connect.rs:99:25: 105:14]>, fn(std::vec::Vec<u8>) -> std::result::Result<std::vec::Vec<u8>, std::io::Error> {std::result::Result::<std::vec::Vec<u8>, std::io::Error>::Ok}>>`

I will investigate later, but it may be #1946 (comment).

@taiki-e
Copy link
Member

taiki-e commented Nov 6, 2019

@cramertj I filed #1955 and confirmed that the above problem can be fixed once #1955 is merged.

@Nemo157
Copy link
Member

Nemo157 commented Nov 6, 2019

+1 to removing the = requirements. It might pay to have another CI job/steps that verify the internal minimum versions are correct, but that seems like it might be complicated enough that it's not worth it (I believe that properly testing it would require a bunch of Cargo.toml rewriting to add and remove path dependencies as needed to test against the released versions, although maybe cargo package could be leveraged to do this somehow).

@cramertj
Copy link
Member Author

cramertj commented Nov 6, 2019

I've landed #1955, rebased this change, and amended the versions to remove =. PTAL.

@LucioFranco
Copy link
Member

@taiki-e thank you for checking! <3

I say lets ship! Thank you for all your hard work @cramertj and others!

@Nemo157
Copy link
Member

Nemo157 commented Nov 6, 2019

CI failure is unrelated, the job itself is wrong and needs fixing (it has been wrong all along, but Cargo is now erroring out in this situation).

Copy link
Contributor

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thanks for coordinating this Taylor!

@theduke
Copy link
Contributor

theduke commented Nov 6, 2019

Out of curiosity, why is the futures executor feature on by default?

Seems like that would make sense as opt-in. Or rather not a feature at all but just a separate crate.

@cramertj
Copy link
Member Author

cramertj commented Nov 6, 2019

@theduke It is exposed as its own crate, futures-executor, which is reexported through the main facade crate. I have no strong opinion about whether or not it be enabled by default.

@theduke
Copy link
Contributor

theduke commented Nov 6, 2019

Yeah I saw that.

It only introduces one additional dependency ( num_cpus, only with the threadpools feature on) but it seems to me like there is no big reason to tie it to the main facade at all and just provide it as the separate crate. It's certainly nice to have a simple stand alone executor available, but it would probably be a rather nieche thing to use, eg in small tests.

@cramertj
Copy link
Member Author

cramertj commented Nov 6, 2019

@theduke The primary value I see is convenience of having things like block_on available, which makes it possible to write small standalone tests and example code using only the futures crate.

@seanmonstar
Copy link
Contributor

It does seem useful for futures to be able to provide a block_on function out-of-the-box, even if a user probably shouldn't use it in most cases. 🤷‍♂

@theduke
Copy link
Contributor

theduke commented Nov 6, 2019

I can see that, and looking at how little code it is (< 1k lines) I'm not terribly bothered anymore.

It would have been nice to avoid the extra dependency, but that's not so relevant with the current crate structure anyway.

@cramertj cramertj merged commit 0c7fa20 into rust-lang:master Nov 6, 2019
@cramertj cramertj deleted the 0.3.0 branch November 6, 2019 18:18
@shepmaster
Copy link
Member

As an arbitrary small datapoint, SNAFU compiles and tests pass against this branch.

@Matthias247
Copy link
Contributor

@cramertj Thanks for coordinating this! And even more so for going through the backlog and closing all long-term outstanding issues!

@seanmonstar

It does seem useful for futures to be able to provide a block_on function out-of-the-box, even if a user probably shouldn't use it in most cases. 🤷‍♂

There was some discussion on including block_on as a minimal executor into std - since it's currently quite hard to use async without any other dependency (you can't even run things in play.rust-lang without boilerplate). I created rust-lang/rust#65875 to implement this - but I don't whether and when we will see it getting there.

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.

None yet

9 participants