[ci] disallow failures in fmt and clippy

[gnzlbg]

Reviewers interpret the travis "green light" as "ok to merge" instead of "check if the allowed failures are green, and then merge". The proof is that basically every pull-request since rustfmt was enabled has broken the formatting. This pollutes unrelated pull-requests with formatting changes.

The downside of this is that when clippy and rustfmt fail to build, the PRs will appear "red". Nothing more, nothing less.

In particular, I want to emphasize that red pull-requests can still be merged. Just check that everything is green, and that the reason, e.g., formatting is broken is because rustfmt-nigthly doesn't build and not because the submitter forgot to run it.

[BurntSushi]

@alexcrichton What do you think about this?

I guess I'm OK with this, since I agree that there is otherwise little point of having these things in CI if we aren't going to actually look at them. If this breaks the build too frequently, then I'd probably suggest just removing them and say that we gave it the good ol' college try.

[alexcrichton]

I'd personally still be wary of doing this. I fear that this will increase the threshold for contribution relatively significantly. Clippy is likely to break multiple times a week, meaning the CI would probably rarely be green. Additionally rustfmt is still (afaik) somewhat rapidly change, so it as well is likely to break throughout each week.

I agree that I don't personally see much use in running CI builders that are allowed to fail, but we've got the capacity as they're fast enough so I wouldn't block them.

All in all I would prefer to not merge this until we get to the point where we could leave the repo alone for like a week, have a new contributor send a PR, and the CI is still green.

[gnzlbg]

I've moved the fmt and clippy fixes to a different PR and left here the changes required to make the clippy and fmt builds always fail.

On a C++ project we do the following with clang-tidy: we have two builds, one that is allowed to fail, and one that is not. Then we have two .clang-tidy files that specify which lints to use. The build that is allowed to fail uses all lints except those that we have explicitly disabled, while the build that is not allowed to fail only contains explicitly enabled lints.

We could do the same for clippy. That is have an allow_fail build that checks everything except the things we explicitly ignore and that we try to keep clean on a best effort basis, and one that cannot fail and checks for things that we want to explicitly enforce, like documentation on public items. We would still have the problem that clippy will sometimes fail to build, but at least that problem will be solved "soon". The problem that clippy lints can change will always exist: we will always be adding lints, and fixing bugs in old lints. I don't think that is going to stop in the forseable future.

@oli-obk Is it possible to enable/disable clippy lints globally in the clippy.toml file?

[oli-obk]

@gnzlbg clippy.toml is just for configuring lints for now, not allow/denying them. You'd need to do that with attributes. We have an open issue about it.

[gnzlbg]

The issue is: rust-lang/rust-clippy#1313

[alexcrichton]

@gnzlbg that could work yeah for the actual lints themselves but I don't think that'd solve the "rustdoc is changing" problem or the "sometimes clippy just doesn't build" problem?

[gnzlbg]

@alexcrichton yes for that we need to wait till clippy-nightly and rustfmt-nightly are distributed with nightly rust via rustup (that is, so that we don't have to build them to use them). I hope this will happen sooner rather than later.

[alexcrichton]

Indeed yeah, at that point I'd be much more comfortable about making this change!

[gnzlbg]

Blocked by rust-lang/rustup#1285 , closed until that's fixed.