tokio-rs / tokio-core

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

deny warnings only in CI #301

Closed ignatenkobrain closed 6 years ago

ignatenkobrain commented 6 years ago

It's very unpleasant when after library update in distribution, crate stops building just because some new warning appears.

Signed-off-by: Igor Gnatenko i.gnatenko.brain@gmail.com

carllerche commented 6 years ago

Hi, thanks for the PR.

I'm a bit confused, are you saying that warnings on Tokio are causing your app / lib builds to fail? If so, that would be a bug w/ Cargo.

ignatenkobrain commented 6 years ago

@carllerche we actually spoke with you in one of other projects about this ;) You suggested passing --cap-lints warn, but that basically changes every error to warning (except for fatal errors), even those which are explicitly #[deny(unused_result)].

Also there was some thread on GNOME ML which describes why -Werror (or #[deny(warnings) in Rust) is bad

https://mail.gnome.org/archives/desktop-devel-list/2012-July/msg00100.html

cuviper commented 6 years ago

are you saying that warnings on Tokio are causing your app / lib builds to fail? If so, that would be a bug w/ Cargo.

No, we build and test each packaged crate on their own in Fedora rpms. When a crate is built directly, Cargo doesn't pass --cap-lints.

seanmonstar commented 6 years ago

Just popping in to clarify it was actually me that recommend capping the lints, not Carl :D

carllerche commented 6 years ago

@cuviper Are you saying that you are trying to package tokio-core as a crate? Could you elaborate more?

cuviper commented 6 years ago

We're packaging individual crates as -devel RPMs in Fedora. For example, tokio-core has a rust-tokio-core.src.rpm, which builds a rust-tokio-core-devel.noarch.rpm. https://koji.fedoraproject.org/koji/packageinfo?packageID=26167

We basically just package the crate source into the -devel RPM, since Rust has no ABI, feature use may differ, etc. But as part of the packaging phase, we do run cargo build and cargo test on the crate to make sure it really works, before it's made available as a dependency for other RPMs.

So it's this phase of testing the crate directly which may be broken by deny(warnings), if the system rustc has added any new warnings.

cuviper commented 6 years ago

to make sure it really works

Meaning, works in the context of Fedora and all its current components. I don't doubt that it worked when you published it. But it also means we get this testing on all architectures we support, which often reveals flaws that developers didn't notice on their x86_64 machines.

seanmonstar commented 6 years ago

A reason that I keep deny(warnings) on always instead of just for tests is I've found cases where the tests happen to use something that isn't used outside of tests, and so the warnings aren't triggered then.

I'd offer the question here, as well: is there a reason that --cap-lints warn cannot be used? The lints don't mean errors, such that things are horribly bad, just warnings.

cuviper commented 6 years ago

I'd offer the question here, as well: is there a reason that --cap-lints warn cannot be used? The lints don't mean errors, such that things are horribly bad, just warnings.

Presumably, lints that are denied by default are worse, but --cap-lints will mask those too.

For that reason, I really hesitate to apply --cap-lints globally in the distro. I would tend toward just selectively applying it to crates that need it. If crates which try to use deny(warnings) for stronger protection end up getting capped on our side, this results in less protection than we'll get for crates that we leave at default lint levels.

carllerche commented 6 years ago

tokio-core is deprecated at this point. If you want to pursue this, I'd recommend opening a new issue on the tokio repository.

It may be possible to not deny warnings by default and pass RUSTFLAGS="-D warnings" in CI.