search

tokio-rs / tokio-core

I/O primitives and event loop for async I/O in Rust
Apache License 2.0
634 stars 116 forks source link

Make the repo compile with newest mio. #292

Closed vorner closed 6 years ago

vorner commented 6 years ago

Currently, if you clone the repository and get fresh dependencies, you get compilation errors due to combination of deprecation warnings from mio and disallowing warnings.

This branch gets rid of some of the warnings ‒ mio events should now be iterater „the rust style“, with an iterator, not by index.

However, there are some more warnings I'm not sure what to do ‒ it seems some methods are now deprecated without a clear replacement (UnixReady::aio and UnixReady::is_aio) and I don't know enough about the internals to decide what to do about it.

To make it compile and because I've read it is generally a bad idea (as a minor bump in dependency can bring new warnings and it is not considered breaking change), I removed the deny(warnings) attribute. Or maybe it could be returned later as #[cfg_attr(test, deny(warnings))], which should catch warnings in the code when it is written, but shouldn't break compilation for downstream users in the future.

alexcrichton commented 6 years ago

Can you leave in deny warnings? Otherwise looks great, thanks!

carllerche commented 6 years ago

@vorner The AIO fns should only be deprecated on platforms that do not support AIO. Those fns should never have been made generally available.

The answer would be to scope the AIO fns in tokio as well by only platforms that support the feature.

vorner commented 6 years ago

The answer would be to scope the AIO fns in tokio as well by only platforms that support the feature.

I'll try to look into it, but I'm not sure how soon I'll get around to it. It'll be at least few days, I'm afraid.

Can you leave in deny warnings? Otherwise looks great, thanks!

Well, not until the AIO stuff is solved ‒ it'd still fail to compile.

But even then, there's always risk that someone will create a new project and some dependency is pushed to crates.io that adds warnings to the existing code. Such person will either give up on the library (hey, it's broken!) or have to fiddle with Cargo.lock to bring in an older dependency.

What is the exact purpose of the attribute in the crate? To make sure it compiles without warnings? Would it be enough to have it as #[cfg_attr(test, deny(warnings))]? That way code pushed to the repository wouldn't pass the compile checks while still compiling for downstream users if a new dep brings something in.

carllerche commented 6 years ago

@vorner You can allow deprecation on the AIO fns for now if you want.

Its important for the build to catch deprecation errors. In theory, the deny warnings flag should not bubble to downstream deps.