Modern C for Fedora (and the world)

Posted Dec 9, 2023 8:35 UTC (Sat) by marcH (subscriber, #57642)
In reply to: Modern C for Fedora (and the world) by pbonzini
Parent article: Modern C for Fedora (and the world)

-Werror (and others) should be easy to turn on and off, this is very important.

What I found to work well is to have -Werror added only in pre-merge CI. Not having it by default makes prototyping more convenient.

This is consistent with running linters in pre-merge CI while not forcing developers to run them all the time.

None of this approach is specific to C.

Of course you need to have some pre-merge CI in the first place. If you don't even have that minimal level of CI then the project is basically unmaintained.

Modern C for Fedora (and the world)

Posted Dec 9, 2023 14:46 UTC (Sat) by mathstuf (subscriber, #69389) [Link] (4 responses)

Even there I find `-Werror` is not the best solution. We prefer to keep the warnings and allow the build to continue on after hitting an "error" and then fail the job at the very end if any warnings happened. This allows one to get more than one round of warnings out of CI at a time. Real failures still stop the build because continuing the build after a hard failure (`make -i`) is a recipe for chasing wild geese. The `-k` flag can be useful, but is a tradeoff between wasted CI cycles and comprehensive error reports.

Modern C for Fedora (and the world)

Posted Dec 9, 2023 17:54 UTC (Sat) by marcH (subscriber, #57642) [Link] (3 responses)

Showing all warnings is nice but you don't want CI to babysit developers too much otherwise there will always be a couple lazy (and frankly: not very smart) developers who will "spam" CI because they can't bother to find how to enable -Werror themselves. They wrongly think they save time that way and waste and in some cases even slow down the whole CI infrastructure. Been there, seen that.

This being said, the simplest and best solution is to compile twice: once without -Werror and once with -Werror. This can be in two separate (and clearly labeled) runs or even consecutively. The first run shows all warnings and the second blocks the merge.

This is a bit similar to the `make || make -j1` technique that avoids (real) errors being drowned by many threads and confusing developers.

Modern C for Fedora (and the world)

Posted Dec 9, 2023 21:43 UTC (Sat) by mathstuf (subscriber, #69389) [Link] (2 responses)

Developers aren't looking through build logs. Instead, CTest gathers the output, does some filtering (e.g., we ignore third party warnings in CI) and uploads it to CDash for viewing. It also obviates the need for the `-j1` trick. We also don't need the second `-Werror` run (which pollutes the build cache) and instead just get the (post-filtered) warning count from CTest and trigger a script failure if it is non-zero.

I'll do an initial run on all of the CI configurations to get a survey of what is broken and then focus on what is broken after that (I don't build all of the configurations locally to know anyways).

Modern C for Fedora (and the world)

Posted Dec 10, 2023 0:53 UTC (Sun) by marcH (subscriber, #57642) [Link] (1 responses)

> Instead, CTest gathers the output, does some filtering

If you have a good test framework that does all that for you then you should absolutely ignore my previous post. Not everyone is that lucky. I mean many projects don't even have any pre-merge CI at all (yet?). Remember that the main article is about Fedora and others stepping up to rescue orphaned projects coded in ancient C. In such a context my simple advice above definitely holds because it's just one extra line in your CI configuration. Super cheap and very high value and something people not familiar with CI may think about.

> Developers aren't looking through build logs.

They don't by default (assuming of course you have developers in the first place...)

They definitely do when there's a CI red light somewhere that threatens the merge of their code any maybe their deadline. In such a case I know from first hand experience that they really enjoy the simple "tricks" I recommended above.

> and uploads it to CDash for viewing.

I don't know anything about CDash but I know neither GitHub nor Jenkins nor Gitlab has any "yellow light"/warning concept, it's either green/pass or red/fail. Running twice with and without -Werror also solves that display limitation problem extremely cheaply. Again: if you have a smarter and better viewer then by all means ignore my tricks.

> We also don't need the second `-Werror` run (which pollutes the build cache)

Curious what you mean here.

Modern C for Fedora (and the world)

Posted Dec 10, 2023 4:10 UTC (Sun) by mathstuf (subscriber, #69389) [Link]

> I don't know anything about CDash but I know neither GitHub nor Jenkins nor Gitlab has any "yellow light"/warning concept, it's either green/pass or red/fail. Running twice with and without -Werror also solves that display limitation problem extremely cheaply. Again: if you have a smarter and better viewer then by all means ignore my tricks.

GitLab-CI does have a "warning" mode with the `allow_failure` key[1]. We use exit code 47 to indicate "warnings happened" so that the testing can proceed even though the build made warning noise. There are issues with PowerShell exit code extraction and that always hard-fails, but that seems to be a gitlab-runner issue (it worked before we upgraded for other reasons). It's actually nifty because it still reports as a `failed` *state* and the `allow_failure` key on the job just changes the render and "can dependent jobs proceed" logic, so our merge robot just sees that state and says "no" to merging.

> > We also don't need the second `-Werror` run (which pollutes the build cache)

> Curious what you mean here.

We have a shared cache for CI (`sccache`-based; `buildcache` on Windows). Adding another set of same-object output for a different set of flags just removes space otherwise ideally suited for storing other build results (*maybe* the object is deduplicated, but it doesn't seem necessary to me; probably backend-dependent anyways).

[1] https://docs.gitlab.com/ee/ci/yaml/#allow_failure