I announced the change on the public mailing list, so we can now close this.

> I think we have to set CXXFLAGS in the respective docker images.
> Do you agree?

Yes, these are compiler-specific options and logically belong in the environment
definition inside the docker container.

Exporting the environment variable in master.cfg when calling build.py worked
for the Linux workers: 
http://buildbot.fast-downward.org/#/builders/17/builds/367

Unfortunately, this confuses the Windows worker:
http://buildbot.fast-downward.org/#/builders/16/builds/100

It seems CMake picks up CXXFLAGS and passes /Werror to MSVC, which it doesn't
recognize. I think we have to set CXXFLAGS in the respective docker images. Do
you agree?

We just talked about this in person but for the record: we do use incremental
builds followed by non-incremental builds (for details, see
http://www.fast-downward.org/ForDevelopers/BuildBot). CMake is smart about
recognizing which files need to be rebuilt.

Perhaps this is the best way to proceed. I also realized that unless we use
-Werror, incremental builds won't work properly in the sense that if we build a
file that produces warnings but not errors, we will get a build failure, but the
next build will not try to recompile this file and therefore not produce any
warning or error.

I think we don't use incremental builds anyway at the moment because we'd have
to teach the build system in which cases it needs to do a complete rebuild (when
something in the cmake files changes), but it's perhaps a good idea to be
future-proof there.

Unfortunately, flunkOnWarnings does not lead to the desired results: when using
"haltOnFailure=True, flunkOnFailure=True, flunkOnWarnings=True" for a build step
producing warnings, subsequent steps are still run 
(http://buildbot.fast-downward.org/#/builders/17/builds/366). Shall we go for
the CXXFLAGS=-Werror option?

*All* these flags have to be set when the build is created. This is documented
on the wiki.

(BTW, it should be "-m32", not "--m32". Perhaps gcc accepts double dashes; I
never tested it. But it's documentation uses single dashes for everything. It
predates the widespread usage of double dashes for long options.)

I just wanted to add my two cents about the "--m32" question. I think it is
necessary to have this flag at the time the build directory is created. Using a
64-bit build with "--m32" will probably not look up paths to libraries again.

Add TODO to summary.

I completely agree, just didn't know flunkOnWarnings is necessary for this. I'll
change the master.cfg accordingly.

> It seems we don't have to change anything about the buildbot configuration.
> Warnings are already emailed and shown appropriately:
> http://buildbot.fast-downward.org/#/builders/13/builds/144

That's the default buildbot behaviour, but it's not comparable to "warnings as
errors". By the same argument, one wouldn't want "-Werror" when building locally
because warnings are already shown (as warnings) without "-Werror".

When we initially configured the buildbot (before your time), we wanted builds
with warnings to count as failed builds. Otherwise there is a high risk that
warnings don't get fixed. The difference is not just in the colors in the
buildbot; it affects whether the overall build counts as successful or not and
the buildbot continues with all the later steps (updating the wiki etc.).

Without warnings as errors, IIRC we will see these orange colours once, but on
further incremental builds that don't affect the same files, we won't see any
indication that anything is wrong.

We could decide that we don't want build warnings to be errors on the buildbot,
but so far our consensus has been that builds with warnings should not count as
successful.

Using -Werror (or the equivalent) in the automated build system is also often
mentioned as one of the most basic best practices in software development.

On my machine, just passing "-m32" was enough. I've updated the documentation
and cleaned it up a bit.

I applied the good old YAGNI principle :-)

It seems we don't have to change anything about the buildbot configuration.
Warnings are already emailed and shown appropriately: 
http://buildbot.fast-downward.org/#/builders/13/builds/144

I guess if we set flunkOnWarnings=True, the orange builds will become red. I
think having the three different colors green, orange and red is better than
just two colors.

Do we need to guess, or is there a way to find out? ;-) I'm not so sure because
passing -m32 when compiling is generally not enough; this also affects the
linking step, and things get messier when enabling LPs. I'll try it out.

I documented CXXFLAGS="-Werror" in the wiki. My guess is that "-m32" can be set
the same way, but I haven't tested it. The CXXFLAGS variable is cached into the
CMAKE_CXX_FLAGS variable, so it has to be present when the cache is created.

I'm currently testing the buildbot setting.

Sounds good. Once this is resolved (i.e., we're happy with the buildbot), I
suggest we announce this change to the mailing list, e.g. in the old thread that
prompted this.

I would also recommend documenting somewhere in our documentation what needs to
be done to get "-Werror" (or other options). Can we get "-m32" (assuming LPs are
disabled) the same way? Is the flag relevant at the time the cmake build
directory is created, at build time, or both?

Yes, I don't think such a change to run-all-tests would be worth the effort.

I'll try out changing the Buildbot settings next week.

So you prefer not to change the run-all-tests scripts to end with a warning if
there were warnings? Works for me; someone interested in adding that could
always work on it themselves. Just want to be sure that we're on the same page. 

(BTW, it may be the case that "flunkOnWarnings" doesn't work correctly with our
setup, where we use the ./build.py script rather than a direct compiler
invocation. I'm not sure how we have currently set this up in buildbot or how it
handles these things internally. But I would suggest trying out the general
buildbot setting first anyway. If it doesn't do the job, we can add "-Werror" to
the buildbot setup instead.)

I made the suggested changes here: 
https://bitbucket.org/jendrikseipp/downward/pull-requests/131

On the "Activity" tab you can see that that I deliberately made the build fail
and work again to test that the changes work as expected.

From my point of view all that's left is changing the Buildbot (see summary).

For the buildbot, I think the more standard way of achieving this is to mark
warnings as failures in the buildbot, using the "flunkOnWarnings" option of
BuildStep. This has the advantage that the buildbot can distinguish between
warnings and errors in its emails and visualizations (I think they are shown as
orange rather than red). It should also hopefully work out of the box for all
builds, including the Visual Studio one.

For run-all-tests, it would be more standard practice to leave the choice of
setting the CXXFLAGS variable to the user if they want to; they might want to
keep going despite certain warnings, and they might also want to set other
options in this variable. Setting "-Werror" in the script unconditionally would
also not work if they have configured a compiler that doesn't know this setting
(like Visual Studio). It would be useful for warnings not to disappear off the
screen, though, ending the output with something like "There were warnings." if
there were warnings.

If we think we almost always want this setting here, one compromise is to set
"-Werror" iff CXXFLAGS is unset. Then the user can still override the choice by
setting the variable to something else externally, even to the empty string. But
I would prefer if we didn't mess with this variable (which is meant to be set by
the user) at all in the run-all-tests scripts, left this choice to the user, and
optionally made warnings a bit more visible here.

I like the proposed change.

It seems like we could just remove -Werror 
from the CMake files and define the 
environment variable CXXFLAGS="-Werror" in the 
buildbot config and in misc/run-all-tests. 
CMake will then store the value of CXXFLAGS in 
CMAKE_CXX_FLAGS 
(https://cmake.org/cmake/help/latest/variable/
CMAKE_LANG_FLAGS.html). What do you think?

Several times, we have heard about broken builds when people upgrade compiler
versions, especially when using Mac OS. The most common reason for this are
non-critical compiler warnings that cause the build to fail because we use the
"-Werror" option.

I think we should not compile with "-Werror" by default. We may want to use it
on the buildbot, or at least use one of the noisier buildbot warning settings.
(The buildbot can be configured to not notify about warnings, notify about
warnings but not treat them as critical failures, or treat warnings as errors.)

For example, see this discussion thread for background:

    https://groups.google.com/forum/#!topic/fast-downward/fFSTDLRBp-A

Marking this as "infrastructure" because it may involve the buildbot.