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

Replace futures-util with futures-lite #2469

Closed
wants to merge 4 commits into from

Conversation

notgull
Copy link

@notgull notgull commented Dec 30, 2023

futures-lite is a lightweight alternative to futures-util that comes
with much less baggage (e.g. no proc macros, no channel implementation)
and compiles much faster. This commit replaces futures-util with
futures-lite in both the axum and axum-core crates.

@davidpdrsn
Copy link
Member

Awesome!

What are the rustc-ice-2023-12-30T16_17_52-3660.txt files for?

@jplatte
Copy link
Member

jplatte commented Dec 30, 2023

Those are files rustc writes when it crashes with an Internal Compiler Error 😄

For a while I frequently got rustc writing them in at least one of my projects too. Seems committed by accident.

@notgull
Copy link
Author

notgull commented Dec 30, 2023

Whoops, my mistake. Will fix once I'm back in front of my laptop

@jplatte
Copy link
Member

jplatte commented Dec 30, 2023

I'm skeptical about this change, since it pulls in an extra dependency without actually getting rid of futures-util in the dependency tree. Both tower and tower-http depend on futures-util, but nothing in axum's dependency tree depends on futures-lite right now AFAICT.

@davidpdrsn
Copy link
Member

Those are files rustc writes when it crashes with an Internal Compiler Error 😄

Ah I see! I've gotten plenty of ICEs but don't think I've noticed those files before.

@jplatte
Copy link
Member

jplatte commented Dec 30, 2023

I think it's a new-ish change that ICEs are also written to files (I'd estimate 6-12mo).

@davidpdrsn
Copy link
Member

I'm skeptical about this change, since it pulls in an extra dependency without actually getting rid of futures-util in the dependency tree.

Yep that's true but I suppose one could do the same to our dependencies and we have to start somewhere. I think its probably okay, especially since futures-lite only depends on futures-core and pin-project-lite, which axum depends on anyway.

@notgull
Copy link
Author

notgull commented Dec 30, 2023

I'm skeptical about this change, since it pulls in an extra dependency without actually getting rid of futures-util in the dependency tree. Both tower and tower-http depend on futures-util, but nothing in axum's dependency tree depends on futures-lite right now AFAICT.

At the moment my goal is to replace futures-util piecemeal. I'm working on replacing it in hyper at the moment, see hyperium/h2#721

@jplatte
Copy link
Member

jplatte commented Dec 30, 2023

Right, I would start with something like this in the lower-level libraries. I think I even looked into this exact thing myself a year or two ago and it wasn't that easy to really do it across all of axum's dependencies so I gave up. But I'm not against the change in principle.

edit: Oh, I see from that h2 PR that it's linked to an issue I created back when I looked into it!

futures-lite is a lightweight alternative to futures-util that comes
with much less baggage (e.g. no proc macros, no channel implementation)
and compiles much faster. This commit replaces futures-util with
futures-lite in both the axum and axum-core crates.

Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Author

notgull commented Jan 28, 2024

@davidpdrsn Apologies for the delay, but I've gotten the CI to pass now. Would you be open to reviewing this?

It was mentioned upthread that we should start this conversion process in the lower level libraries. While I agree with this, it seems that the maintainer of hyper/h2 is predisposed at the moment. Therefore I've started on other parts of the ecosystem.

@notgull
Copy link
Author

notgull commented Feb 11, 2024

@davidpdrsn Any chance you can take a look at this?

@notgull
Copy link
Author

notgull commented Apr 19, 2024

Closing as I do not have the time to rebase this

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