Handling Warnings in a CI


#1

Hey all,

I’m looking for feedback on an idea before I write up an RFC for it.

Motivation

Where I am coming from:

  • I love how compiler warnings help me catch problems in my code.
  • I want to automate maintaining crates, including reviewing PRs, as much as possible due to the proliferation of crates because of the trend to keep them small.
    • I want to minimize human fallacy, in preparing or reviewing a PR, that exist with manual checks (running clippy, etc)
    • I don’t want to have to look at the logs, on success or failure
    • I don’t want the build breaking for things unrelated to the PR at hand
      • Discourages contributors when they are held accountable for unrelated code or if they have to wait on the maintainer to fix it
      • If there is an expectation of this happening, then it requires manually checking the logs to help the contributor through the PR process

The naive solution for warnings is either

#![deny(warnings)]

or

RUSTFLAGS=-D warnings cargo check

But when a new release of Rust comes out, there might be new warnings, breaking your PRs. Some recommend working around this by explicitly listing every warning. The problem is that it either goes stale or takes work to identify new warnings and update it in every crate.

This problem is not unique to Rust. Some fear enabling -Werror but we don’t want to go to the other extreme like gcc where -Wall has been frozen out of concern for breaking people. The fact that they froze it show enough people care about minimizing breakages from new warnings.

Proposal

I feel like Epochs provide a good compromise between constantly evolving warning groups or warning groups going stale.

Warning groups would remain “frozen” during an epoch. New warnings cannot be added to a group. Warnings can be removed from a group when being removed from the compiler.

warning groups would have an -unstable variant that represents what the group will be in the next release.

  • Supports developers wanting the latest
  • Helps developers preparing for a new epoch
  • Helps avoid warnings from falling through the cracks when transitioning between epochs.

Alternatives

Not using groups

Mentioned above

Versioning warning groups

With Rust’s release schedule, this would quickly lead to a unhelpfully large set of warning groups

Pin the rust compiler in the CI

So instead of locking down the warnings, we lock down the compiler

matrix:
  include:
  - rust: 1.24.0  # Locking down for consistent behavior
    env: RUSTFLAGS=-D warnings
    script:
    - cargo check --tests

This is a good short term solution and might get us by in the long run.

The downsides:

  • Maintaining the compiler version in all of your crates.
  • Contributors get surprised with a breaking build for their PR, especially for allows being turned into denys.
    • A possible workaround is putting a #![warn(warnings)] in the code. It sounds like it won’t lower denys to warn and -D warnings will still do the right thing?

Post warnings back to the PR

Idea from @skade

Write a tool that builds the code with warnings denied, captures the warnings, and reports them back to github. Run this in on Travis in after_script.

Pros

  • Doesn’t require manual inspection of the logs
  • Makes information available in a forum the maintainer is already interacting on

Cons

  • Still requires the maintainer to sift through them to help guide the contributor on what they are accountable for
  • Still requires the maintainer to immediately turn around fixes for the warnings so he doesn’t have to sift through them on the next PR
  • One time cost to implement a fairly special case tool but would hopefully be used widely.

EDIT: Added “Post warnings back to the PR”


#2

Considering that most added warnings nowadays are for pretty serious things (like type systems holes that soon become errors), is this an actual issue or a “could be” issue?

Nevertheless, I think this should be solved on a travis level. The build should be costumised in a way that sends you a notice on warnings.

The fear of -Werror comes from a missing additional feature that we have: rustc caps lints to warnings during release builds and for cargo dependencies, which ensures old cold always builds, even if a new warning was added and deny is on.


#3

Nevertheless, I think this should be solved on a travis level. The build should be costumised in a way that sends you a notice on warnings

Let me see if I understand what you’re suggesting. Are you suggesting that we find way to have the build succeed but still send out a message on warnings?

Do you know of any examples on how to do this? I’ve not seen anything in the Travis docs yet that would help with this.

A downside of this is it still requires maintainer intervention to decide if the warnings should be ignored by the contributor or not.

The fear of -Werror comes from

Yup, I do agree that the problem with -Werror is a lot less in the Rust ecosystem. Macros are still a problem.


#4

You could approximate this by having an allow_failures target which builds in deny(warnings) mode.


#5

I have no example at hand, but travis holds all information to write an after_script that can post a notification through this API: https://developer.github.com/v3/repos/statuses/#create-a-status

This is basically how you’d implement deploy to gh-pages by foot, you can also do that to create a notification. It ends up being rather brief - run the appropriate check command, probably with json output, post the notification the the PR.

Also, you could set the cap-lints level differently depending on master and PR, making PRs work, but merges to master fail.

(If you want a better workflow for this, you are probably entering BORS territory)

Yes, I find this an extreme edge-case, though. Also, you can fix this by raising deprecation to plain warnings again and you will probably kill 99% of the reasons for this edge-case.

I can follow your “I want reviewer convenience” argument (having a standard solution for that would be great!), not quite your “this has huge impact on contributors”.


#6

@HadrienG

You could approximate this by having an allow_failures target which builds in deny(warnings) mode.

I assume that will give you the green light and the only way to know if there are warnings is to manually inspect the log.

@skade

I have no example at hand, but travis holds all information to write an after_script that can post a notification through this API: https://developer.github.com/v3/repos/statuses/#create-a-status

Ok, so no existing solution but a new solution could be made by connecting the pieces together. I’ll add this to my solution alternatives.

I can follow your “I want reviewer convenience” argument (having a standard solution for that would be great!), not quite your “this has huge impact on contributors”.

I’ve had a lot of problems with spurious failures in my fairly strict CI. One case stands out in my mind that is warning related. I’ve not catalogued the failures though to have a feel for how much it was warnings vs other problems.

It has been a pain to me as the one submitting changes. The reasons it hasn’t been too much of a pain for my contributors is (1) I don’t have many and (2) I proactively take the burden on myself (inspect failures immediately, post to give the contributor a heads up, resolve the issue in another PR).


#7

Travis can also send a notification by e-mail when a failure happens. Maybe there is a way to have an “orange light” feature where Travis tells GitHub that the build succeeded, but some allowed failures occured, and to have that automatically displayed in the PR somehow. But I have not experimented enough with GitHub - CI integration to tell how that could be made to work.


#8

With quite a bit of experience in larger projects, I would, in general, be relatively relaxed on warnings in PR mode. Travis definitely gives you the ability to run a different suite on PRs and on the main branch.

My biggest argument for relaxed styles on PRs is that I’d like people to show WIP things as soon as possible, in which case they might still emit warnings.

I just checked with a person who used to work at Travis: the scenario I describe should be possible.


#9

My biggest argument for relaxed styles on PRs is that I’d like people to show WIP things as soon as possible, in which case they might still emit warnings.

Thanks for bringing up this use case. Definitely something to be kept in mind.

With quite a bit of experience in larger projects, I would, in general, be relatively relaxed on warnings in PR mode. Travis definitely gives you the ability to run a different suite on PRs and on the main branch.

I feel I must be missing something about this idea because it sounds less than ideal. it sounds like you wouldn’t know about the warnings after the changes have been merged and once a build has finished.

This would lead to:

  • Master can now be “broken”
  • People might be forking / creating branches from this master, causing extra noise for them
  • The maintainer now needs to go and clean up the mess left by a contributor.

#10

I’ve wanted a “turn rustc/rustfmt diagnostics into Github reviews” tool for years!


#11

That’s why I mention bors. It accepts and serializes accepted PRs, automatically merges them into an integration branch and if builds break, they break on the integration branch. That gives you the best of both approaches.

You’ll notice that rustc PRs are not tested with the full suite, they are fully tested on merge, but off master.


#12

My understanding is that, at least for libraries, this is already a best practice so that you don’t accidentally start depending on a newer version of rust anyway. (Like new syntactic sugar, new std APIs, …)

So maybe just do that, then have a not-part-of-CI build that runs against rust beta or nightly so you get the advantages of the new warnings ASAP without being forced to fix them immediately.


#13

@HadrienG

Travis can also send a notification by e-mail when a failure happens. Maybe there is a way to have an “orange light” feature where Travis tells GitHub that the build succeeded, but some allowed failures occured, and to have that automatically displayed in the PR somehow. But I have not experimented enough with GitHub - CI integration to tell how that could be made to work.

@killercup

I’ve wanted a “turn rustc/rustfmt diagnostics into Github reviews” tool for years!

I’ve started a write-up of this for the crate-ci org.