Page MenuHomeFreeBSD

build: provide a default WARNS for src builds
ClosedPublic

Authored by kevans on Thu, Sep 10, 6:49 PM.

Details

Summary

The current default is provided in various Makefile.inc in some top-level directories and covers a good portion of the tree, but doesn't cover parts of the build a little deeper (e.g. libcasper).

Provide a default in src.sys.mk and set WARNS to it in bsd.sys.mk if it's defined. This lets us relatively cleanly provide a default WARNS no matter where you're building in the src tree without breaking things outside of the tree.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 33596

Event Timeline

kevans created this revision.Thu, Sep 10, 6:49 PM
kevans requested review of this revision.Thu, Sep 10, 6:49 PM
brooks accepted this revision.Thu, Sep 10, 7:07 PM

Seems sensible. I guess it's good that most of the issues are in tests or GPL code we're deleting soon?

I'll probably need to watch out for this change when I merge to CheriBSD, but it seems easy to handle.

This revision is now accepted and ready to land.Thu, Sep 10, 7:07 PM

Seems sensible. I guess it's good that most of the issues are in tests or GPL code we're deleting soon?

Indeed- all of ofed is a mess with this stuff, but that's contrib and there's a lot there that needs fixed. While I'm mentioning it, here's a list of the changes I've already flushed out this branch that were detected by this patch set:

I'll probably need to watch out for this change when I merge to CheriBSD, but it seems easy to handle.

The conflicts proper should really be trivial, you'll mainly need to watch out if your version of LLVM differs significantly in warning heuristics from what's currently in base or you've changed warning sets at different WARN levels.

phk added a subscriber: phk.Fri, Sep 11, 8:40 AM

Maybe, as inspiration, we should make a web-service which made the warnings at current and higher levels for the non-6 directories available, along with a X/T plot of how many there are ?

In D26397#587148, @phk wrote:

Maybe, as inspiration, we should make a web-service which made the warnings at current and higher levels for the non-6 directories available, along with a X/T plot of how many there are ?

What an excellent idea- I do believe it'd be trivial to strip -Werror and force WARNS to 6 for an entire build. I'll hack something together for this.

In D26397#587148, @phk wrote:

Maybe, as inspiration, we should make a web-service which made the warnings at current and higher levels for the non-6 directories available, along with a X/T plot of how many there are ?

What an excellent idea- I do believe it'd be trivial to strip -Werror and force WARNS to 6 for an entire build. I'll hack something together for this.

Jenkins has a nice graph view for compiler warnings with the warnings next plugin. Maybe we could add another job without Werror and max warnings and run that like once a week?

kevans added a subscriber: lwhsu.Fri, Sep 11, 2:57 PM
In D26397#587148, @phk wrote:

Maybe, as inspiration, we should make a web-service which made the warnings at current and higher levels for the non-6 directories available, along with a X/T plot of how many there are ?

What an excellent idea- I do believe it'd be trivial to strip -Werror and force WARNS to 6 for an entire build. I'll hack something together for this.

Jenkins has a nice graph view for compiler warnings with the warnings next plugin. Maybe we could add another job without Werror and max warnings and run that like once a week?

Oh, that would be perfect. I'll ping @lwhsu and see if we can get that on his list, though my understanding is that his backlog is unnecessarily long because he's constantly hunting down peoples' problems. =(

I did commit rS365631 in the interim to help support this idea, so that in an ideal world we can do this purely with environment manipulations rather than having to apply a patch for it -- which is good whether we get it into the Jenkins workflow at some point or whether I hack something together in the interim.

ngie accepted this revision.Sun, Sep 13, 11:51 PM
ngie added a subscriber: ngie.

surewhynot

I think I'm going to go ahead and commit this in the coming days -- FWIW, I hacked together a reallllly primitive interface for browsing warnings by crude 'path' (based solely on output, so not perfect) : https://warns.kevans.dev/ -- this should work in the interim until we can get something in CI proper. I'll hack together some pretty graphs from it when it gets a couple more builds under its belt.

emaste accepted this revision.Mon, Sep 14, 1:24 AM
emaste added inline comments.
contrib/libarchive/test_utils/test_main.c
478 ↗(On Diff #76896)

This file is the only code change left? IMO submit this upstream and commit this change separately, in advance of the rest.

kevans added inline comments.Mon, Sep 14, 1:33 AM
contrib/libarchive/test_utils/test_main.c
478 ↗(On Diff #76896)

This is actually needing rebased -- I landed that upstream, pulled it through vendor and committed as rS365637. I've since discovered some other warnings in libarchive, but they're masked by the build stuff for all those having already papered over it with WARNS.

This revision now requires review to proceed.Wed, Sep 16, 5:15 PM
This revision is now accepted and ready to land.Fri, Sep 18, 5:20 PM
kevans closed this revision.Fri, Sep 18, 5:20 PM