New buildbot with -Werror

Greetings,

I would like to propose adding a buildbot which builds with -Werror. The reason for a new buildbot in this configuration is twofold:

  1. It helps users who track and release from ToT, because they (generally) seem to build with -Werror. Speaking from experience :-), new warnings tend to crop up in a large range of commits, and end up blocking one or more of these downstream users. These users also seem to span several organizations, which makes coordination difficult.

1a. The current buildbots do not build with -Werror so that they will run tests even if warnings are generated. It may or may not be reasonable to enable -Werror by default for buildbots at some point in the future, but I don’t think it’s quite reasonable to do so yet (judging based on the rate at which new warnings seem to get added, that would leave too many buildbots broken). Adding a buildbot in the -Werror configuration should help to inform any such future changes.

  1. It helps users who develop without -Werror (either by conscious choice or by oversight). Commits which inadvertently add warnings often seem to get reverted; however, if a buildbot can deliver warnings quickly, the committer can submit a small fix right away.

I have uploaded a diff to Phabricator, however I ask to please keep high-level comments on this thread:
http://reviews.llvm.org/D18382

This build would be owned and monitored by Google.

If there are no strong objections in the next few days, I will go ahead with this plan.

Thanks,
dlj

Could we change the buildbot config to treat warnings-as-errors-that-continue in the build step? (so we get the benefit of further execution tests, while still sending fail-mail, etc for the warning?)

From: "David Jones via cfe-dev" <cfe-dev@lists.llvm.org>
To: cfe-dev@lists.llvm.org, llvm-dev@lists.llvm.org
Cc: gkistanova@gmail.com
Sent: Tuesday, March 22, 2016 7:00:28 PM
Subject: [cfe-dev] New buildbot with -Werror

Greetings,

I would like to propose adding a buildbot which builds with -Werror.
The reason for a new buildbot in this configuration is twofold:

1. It helps users who track and release from ToT, because they
(generally) seem to build with -Werror. Speaking from experience
:-), new warnings tend to crop up in a large range of commits, and
end up blocking one or more of these downstream users. These users
also seem to span several organizations, which makes coordination
difficult.

1a. The current buildbots do not build with -Werror so that they will
run tests even if warnings are generated. It may or may not be
reasonable to enable -Werror by default for buildbots at some point
in the future, but I don't think it's quite reasonable to do so yet
(judging based on the rate at which new warnings seem to get added,
that would leave too many buildbots broken). Adding a buildbot in
the -Werror configuration should help to inform any such future
changes.

2. It helps users who develop without -Werror (either by conscious
choice or by oversight). Commits which inadvertently add warnings
often seem to get reverted; however, if a buildbot can deliver
warnings quickly, the committer can submit a small fix right away.

I have uploaded a diff to Phabricator, however I ask to please keep
high-level comments on this thread:
http://reviews.llvm.org/D18382

This build would be owned and monitored by Google.

If there are no strong objections in the next few days, I will go
ahead with this plan.

I think having -Werror bots is a good idea, at least when self hosting. Non-self-hosting -Werror (i.e. with older versions of Clang, or with GCC, etc.) might also be useful, but I'm less sure (since we can't fix those warnings if the warning is the problem).

-Hal

My thought was that its reasonable to expect no warnings when building from bootstrap (as you say) and the last release of Clang. Generally, I think we should work around warnings in the last release of Clang if only for the convenience of folks using that release to build stage1 and using Werror.

Certainly, we tend to fix warnings even from earlier Clang versions and from GCC in order to keep Werror clean.

Yeah - tend to prefer self-host, or maybe at most “the prior release of clang” if we want to be able to get these results quickly (faster than rebuilding the compiler, etc)

Could we change the buildbot config to treat
warnings-as-errors-that-continue in the build step? (so we get the benefit
of further execution tests, while still sending fail-mail, etc for the
warning?)

That sounds reasonable, although the buildbot implementation seems to be
pretty focused on make:
https://github.com/buildbot/buildbot/blob/master/master/buildbot/steps/shell.py#L374
I'm guessing that between CMake and Ninja, buildbot will probably need some
minor changes.

Could we change the buildbot config to treat
warnings-as-errors-that-continue in the build step? (so we get the benefit
of further execution tests, while still sending fail-mail, etc for the
warning?)

That sounds reasonable, although the buildbot implementation seems to be
pretty focused on make:

https://github.com/buildbot/buildbot/blob/master/master/buildbot/steps/shell.py#L374
I'm guessing that between CMake and Ninja, buildbot will probably need
some minor changes.

I suppose I was thinking of implementing it at a higher level in buildbot
"any warnings in this task, treat as ErrorAndContinue" - but that might be
problematic for other warnings that aren't actually from the compiler.
Dunno what'll be the best approach.

I've dug around a bit, and it looks to me like separating phase1 and phase2
behavior is going to be a bit tricky (looking at zorg [*]) as long as we
want -Wno-error as the default for buildbots.

So, the first thing I would propose is to add this buildbot and run it for
a while, just to see what the current background rate of breakages will be.
I think getting a feel for the baseline is going to be a solid first step.

If that signal is good enough, and folks are happy getting nag mail when
they cause regressions :slight_smile: , then it probably makes sense to change the
zorg build commands to use -Wno-error on phase1 and -Werror on phase 2 by
default. That insulates against host compiler issues, but remains strict
when self-hosting.

For Chandler's suggestion to use -Werror on a phase 1 build with the
current release, that seems perfectly reasonable; but again, it's probably
best encoded with a zorg change specifically for that case. (I don't want
to add too many magical layers of CMake option overrides for two-phase
builds...)

[*] I'm specifically looking at _getClangCMakeBuildFactory here:
http://llvm.org/svn/llvm-project/zorg/trunk/zorg/buildbot/builders/ClangBuilder.py

I just changed this build on green dragon to warn and email if warnings are found in the console log:

http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA_build/

I agree. Since the environment is controllable, and if the bot starts
green while self-hosting, it's reasonable to assume -Werror can break
without problems.

The only problem is if both GCC and Clang have the same warning, and
fixing stage1 fixes stage2, we'd never know if Clang would have hit
the same assert.

But since this is regarding the build process, not Clang's warnings, I
think it's not a problem.

cheers,
--renato

Hey, that’s handy. Thanks Chris!

-eric