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

Update futures/hyper crate to prepare async/.await #2112

Merged
merged 2 commits into from
Dec 13, 2019
Merged

Update futures/hyper crate to prepare async/.await #2112

merged 2 commits into from
Dec 13, 2019

Conversation

tetsuharuohzeki
Copy link
Contributor

Ideally, we can replace futures with 0.3, but there are some limitation by the current ecosystem.

hyper v0.12 depends on future v0.1 and hyper::server::Server implements its traits.
So we need to introduce futures01 crate as a cushion to migrate it.

@kinnison
Copy link
Contributor

kinnison commented Nov 7, 2019

Hi @saneyuki

Thank you for wanting to enhance rustup. At this time, given the async/.await stuff is due to drop today, I'd rather wait for the ecosystem crates to catch up and then switch properly without needing futures01 and friends. I'll leave this PR open, but really it should evolve into replacing our use of hyper/reqwest/futures with async/.await versions thereof, rather than adding backwards-compatibility shims too.

@tetsuharuohzeki
Copy link
Contributor Author

Ah, okay.

@kinnison
Copy link
Contributor

kinnison commented Nov 7, 2019

I look forward to seeing this evolve into a more complete solution over the coming weeks, so please don't be disheartened :D If you need help with any of the extra work, please pop onto our Discord channel on the Rust discord server and we can discuss things.

@tetsuharuohzeki tetsuharuohzeki changed the title Update 'futures' crate to v0.3 Update futures/hyper crate to prepare async/.await Dec 11, 2019
@tetsuharuohzeki
Copy link
Contributor Author

As hyper v0.13 has been published, I added changes to switch to it.


@kinnison

After working to switch to hyper v0.13, I felt wee don't have to wait async/.await support by reqwest by following reasons.

  1. In this change, we touched only test codes for download workspace and they are well separated from production code.
    • These changes happens in dev-dependencies
  2. After upgrading hyper to v0.13, we are free from futures crate and its shim layer.
  3. In download workspace, test codes launches a http server internally and these changes related to the server for testing.
    • I seem that it's separated concern to start to use async/.await in production code.
    • After this change, we can use async/.await for our test code. But I suspect that the benefits from async/.await would be limited.

So I'd like to request to merge this without waiting reqwest's next version to make this pull request small changesets and I also think we should do work for request separately.
How about do you think?

@kinnison
Copy link
Contributor

This code looks much nicer than the old code, 👍

Could you please tidy/squash the series into two commits, one which changes all the source and Cargo.toml, and one which is just the result of the change to Cargo.lock from running a build?

That way I can more easily confirm that the Cargo.lock update is safe.

I agree that this is good to merge, content-wise, once that's done.

@tetsuharuohzeki
Copy link
Contributor Author

@kinnison

Could you please tidy/squash the series into two commits, one which changes all the source and Cargo.toml, and one which is just the result of the change to Cargo.lock from running a build?

Okay! I pushed squashed commits

Copy link
Contributor

@kinnison kinnison 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 👍 Let's just wait on the CI to confirm the squash was good.

@tetsuharuohzeki
Copy link
Contributor Author

CI is passed 😄

@kinnison kinnison merged commit d21211c into rust-lang:master Dec 13, 2019
@tetsuharuohzeki tetsuharuohzeki deleted the futures-03 branch December 13, 2019 06:06
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.

2 participants