Release builds shouldn't use -Werror

[davidgiven]

On my platform, building head of git fails because of uninitialised variable warnings being treated as errors.

/home/dg/src/OpenShadingLanguage/src/liboslexec/lpeparse.cpp: In member function ‘OSL_v1_10::lpexp::LPexp* OSL_v1_10::Parser::parseSymbol()’:
/home/dg/src/OpenShadingLanguage/src/liboslexec/lpeparse.cpp:117:9: error: ‘iscustom’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
         if (iscustom) {

Please don't use -Werror for release builds --- warnings aren't standardised between different compilers, which means that as people upgrade their compilers to have better analysis and more warnings, stuff randomly stops building. (In this case, iscustom is being initialised via an output variable in a function call; I bet what's happened is that gcc 7.2.0, which is what I have, has started taking into account the function implementation rather than just assuming that because iscustom is being passed by reference then it must be initialised.)

[davidgiven]

...so I completely failed to notice that you actually have formal releases as well as git; luckily for my credibility -Werror is turned on there too. I've tested against OSL 1.9.6.

I'm on ARM Debian with gcc 7.2.0, by the way.

[lgritz]

We're purposely using -Werror all the time, because we want to definitely catch a build warning as soon as it happens. But we intend for it to happen rarely, and get fixed right away!

I'll fix the particular warning you are reporting right now, stay tuned...

By the way, it is possible to do a build with the -Werror turned off, just build with the STOP_ON_WARNING cmake option turned off.

[lgritz]

Fix proposed in #847

[lgritz]

Merged, and also backported to RB-1.9.

[davidgiven]

I'm mainly concerned is that this leads to unreliable builds. As there's no way of knowing what set of warnings gcc 8 will have, there's no way of knowing whether any given release of OSL will build on gcc 8 --- even though it's known to be perfectly good. Adding -Werror puts an implicit undocumented dependency on the compiler version.

This is why the C and C++ specifications dictate what the standard set of errors are, and compilers aren't allowed to add more without custom compilation options.

Development should always happen with -Werror turned on, but releases shouldn't...

[lgritz]

Yeah, I get it. On one hand, if we are satisfied that the code is correct, we want an official release to just build for anybody, no headaches, even if maybe just by chance they are building against a newer compiler or a newer version of some dependency that changed slightly in a way that creates new warnings when building our code with their headers.

On the other hand, though, we want to catch every warning. I'm leery of using warnings for debug builds and turning them off for Release builds. Mostly because I build by default in Release mode, and only make a debug build when I need to actually use a debugger like gdb or lldb.

The current strategy is that we always use -Werror for all build types on all branches, but if people run into an unexpected warning, they can always just build with STOP_ON_WARNING=0 to disable the -Werror.

I suppose that an alternative is that we could use -Werror by default in master, but when we cut new release branches, one of the things we do in the release branch is change the default release builds to not use -Werror (though I argue that we should still -Werror for all CI builds on any branch, and for debug builds on any branch).

Does that sound better to you? Or do you have an alternative suggestion?

[davidgiven]

Fair enough --- mainly just wanted to be sure you were aware of the issue. I've been bitten by this in real life.

TBH, the easiest option is just a line in INSTALL.md about the existence of STOP_ON_WARNING=0. That way the people who care will know about it, nobody else will be affected, and it won't need any workflow changes. I totally agree that warnings should be fixed as soon as possible.

[lgritz]

OK, I will certainly put something in INSTALL.md about it, good suggestion.

I am still considering the merits of turning off the warnings for Release builds of released branches (except when it's running on TravisCI or Appveyor). Maybe that will remove these occasional headaches, while still allowing us to easily and immediately catch any new warnings in the development branch.

[lgritz]

I updated INSTALL.md to point out how to build without warnings if that stops one's build.

I also propose #850 to ensure that if we turn warnings off as part of the release branching process, the CI tests will still catch any new warnings that crop up.