Bug 155047 - [GTK] Build with -Werror in DEVELOPER_MODE
Summary: [GTK] Build with -Werror in DEVELOPER_MODE
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 157732 157733 157734 174463 226193 226557 226643
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-04 14:47 PST by Michael Catanzaro
Modified: 2021-07-02 11:55 PDT (History)
13 users (show)

See Also:


Attachments
Patch (1.45 KB, patch)
2016-05-15 19:07 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.55 KB, patch)
2017-07-13 11:49 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (11.19 MB, application/zip)
2017-07-13 14:12 PDT, Build Bot
no flags Details
Patch (3.94 KB, patch)
2021-06-03 14:35 PDT, Michael Catanzaro
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-03-04 14:47:17 PST
We should default ENABLE_WERROR to the value of DEVELOPER_MODE is set, to force developers to fix warnings rather than just ignoring them. This should be changed by editing WEBKIT_SET_EXTRA_COMPILER_FLAGS. Currently its default value is that of IGNORECXX_WARNINGS, which doesn't make much sense to me.

Before making this change, we need to:

 * Fix current warnings affecting the compilers we care about (at the very least: Clang 3.7, GCC 5.x, and GCC 6.x).
 * Change the bots to pass -DENABLE_WERROR=OFF, because we *really* don't want bots to break unnecessarily.
Comment 1 Konstantin Tokarev 2016-03-05 11:22:11 PST
I would rather select a small set of warnings we want to be fatal, and apply -Werror only for them, e.g. -Werror=return-type -Werror=non-virtual-dtor. Later we could extend this list gradually.
Comment 2 Michael Catanzaro 2016-03-05 11:53:18 PST
(In reply to comment #1)
> I would rather select a small set of warnings we want to be fatal, and apply
> -Werror only for them, e.g. -Werror=return-type -Werror=non-virtual-dtor.
> Later we could extend this list gradually.

TBH I don't see value in this, I would like to enforce either fixing or disabling all warnings. (For developers performing development builds.)

We have very few warnings right now, so this shouldn't be any problem.
Comment 3 Konstantin Tokarev 2016-03-05 11:58:37 PST
(In reply to comment #2)
> (In reply to comment #1)
> > I would rather select a small set of warnings we want to be fatal, and apply
> > -Werror only for them, e.g. -Werror=return-type -Werror=non-virtual-dtor.
> > Later we could extend this list gradually.
> 
> TBH I don't see value in this, I would like to enforce either fixing or
> disabling all warnings. (For developers performing development builds.)
> 
> We have very few warnings right now, so this shouldn't be any problem.

New version of gcc or clang may introduce new warning by default, resulting in compilation errors in case -Werror is used (e.g., -Winconsistent-missing-override become enabled by -Wall in clang 3.5)
Comment 4 Michael Catanzaro 2016-03-05 13:48:32 PST
Which is why we should only enable it in developer mode, so developers are forced to fix or silence the new warnings to keep the build clean (unless they pass -DENABLE_WERROR=OFF), but users aren't affected either way.

New warnings will also be introduced by folks using a different compiler than you.
Comment 5 Konstantin Tokarev 2016-03-05 14:27:57 PST
Another source of warnings is unused arguments in case function contains #if guards. Silencing that would require addition of lots of source lines
Comment 6 Michael Catanzaro 2016-03-05 17:00:02 PST
(In reply to comment #5)
> Another source of warnings is unused arguments in case function contains #if
> guards. Silencing that would require addition of lots of source lines

You shouldn't see any of these, that's what the UNUSED_PARAM macro is for! Yes, it is often annoying to silence these, but we made a project decision not to disable the warning, though there's a good argument to be made for disabling that warning. (I'm currently planning to do so for DerivedSources only, as that's currently the only place we see any of these, at least on the GTK port.)

But it's irrelevant to whether we should turn warnings into errors, as we don't want preexisting warnings preventing developers from detecting new warnings. Enforcing -Werror in development builds is the best way to ensure the build stays clean and shiny. If you don't want to fix them, you can just use -DENABLE_WERROR=OFF.
Comment 7 Michael Catanzaro 2016-05-15 19:06:06 PDT
Let's enable it even on the bots for now, to match what EFL has been doing for years without causing too much trouble. But we should keep an eye on this and disable it on the bots if it becomes an issue.
Comment 8 Michael Catanzaro 2016-05-15 19:07:17 PDT
Created attachment 278993 [details]
Patch
Comment 9 Michael Catanzaro 2016-05-15 19:09:38 PDT
(In reply to comment #0)
> We should default ENABLE_WERROR to the value of DEVELOPER_MODE is set, to
> force developers to fix warnings rather than just ignoring them. This should
> be changed by editing WEBKIT_SET_EXTRA_COMPILER_FLAGS. Currently its default
> value is that of IGNORECXX_WARNINGS, which doesn't make much sense to me.

FYI I misunderstood how this works; it's not creating a CMake option inside the macro, rather it's defining a list of optional parameters. So ENABLE_WERROR and IGNORECXX_WARNINGS are different optional parameters to WEBKIT_SET_EXTRA_COMPILER_FLAGS. IGNORECXX_WARNINGS is not the default value of ENABLE_WERROR.
Comment 10 Csaba Osztrogonác 2016-05-19 06:55:59 PDT
(In reply to comment #7)
> Let's enable it even on the bots for now, to match what EFL has been doing
> for years without causing too much trouble. But we should keep an eye on
> this and disable it on the bots if it becomes an issue.

I suggest disabling werror on your ARM bot, because 
there are zillion warnings on ARM long time ago.

clean build on ARM GTK bot warnings:
https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/11162

Today I submitted many patches fix a dozen ARM warnings,
but there are many cast-align warnings remaining. Maybe we 
should use Wno-error=cast-align until somebody can fix them.
Comment 11 Michael Catanzaro 2017-07-11 12:14:51 PDT
I'd still like to do this, and for WPE as well. Developers must not ignore compiler warnings, they must be fixed or suppressed.
Comment 12 Michael Catanzaro 2017-07-13 10:13:24 PDT
Let's start with GTK only for now.
Comment 13 Michael Catanzaro 2017-07-13 11:49:00 PDT
Created attachment 315365 [details]
Patch
Comment 14 Michael Catanzaro 2017-07-13 11:49:33 PDT
It's not perfect as it does not apply to ThirdParty libs which don't use this macro, but I think that's fine for now.
Comment 15 Michael Catanzaro 2017-07-13 11:51:49 PDT
(In reply to Csaba Osztrogonác from comment #10)
> I suggest disabling werror on your ARM bot, because 
> there are zillion warnings on ARM long time ago.

We should add -Wno-error to its CMAKE_C_FLAGS and CMAKE_CXX_FLAGS.
Comment 16 Build Bot 2017-07-13 14:12:39 PDT
Comment on attachment 315365 [details]
Patch

Attachment 315365 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4114987

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Comment 17 Build Bot 2017-07-13 14:12:41 PDT
Created attachment 315371 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 18 Michael Catanzaro 2017-07-13 18:37:40 PDT
Note the EWS failure is fixed in bug #174463.
Comment 19 Carlos Garcia Campos 2017-07-13 22:55:09 PDT
I don't want this. Sometimes just upgrading GCC makes new warnings to appear, or even in other cases new code using new C++ features introduces warnings in some compilers, that it could even be a compiler bug. Also harmless warnings like unused parameters that depend on an #ifdef and someone forgot to use UNUSED_PARAM. Let's not make so easy to break our build, please. I don't want the bots to be red for the whole (European) night because of a harmless warning.
Comment 20 Michael Catanzaro 2017-07-14 08:35:16 PDT
We could add -Wno-error to CMAKE_C_FLAGS and CMAKE_CXX_FLAGS on all our bots. Would you be OK with it then? That will override because it will come later on the command line.
Comment 21 Carlos Garcia Campos 2017-07-14 09:06:07 PDT
(In reply to Michael Catanzaro from comment #20)
> We could add -Wno-error to CMAKE_C_FLAGS and CMAKE_CXX_FLAGS on all our
> bots. Would you be OK with it then? That will override because it will come
> later on the command line.

I'll do the same in my build script, then.
Comment 22 Michael Catanzaro 2021-06-02 15:25:24 PDT
So, four years later! I want -Werror on EWS at least to avoid excessive newly-introduced warnings. I proposed -Werror for EWS in https://lists.webkit.org/pipermail/webkit-dev/2021-May/031867.html with no objections. But for sure we do not want -Werror in use on the buildbots, because that will inevitably cause us to lose test results due to silly warnings. So goal is to enable -Werror on EWS, but not on buildbots.

We could implement it in two ways:

 (a) Add -Werror specially to the EWS config and nowhere else
 (b) Add -Werror when using ENABLE_DEVELOPER_MODE, and add -Wno-error to our buildbot configs to override it there

(b) is this bug, and I prefer (b). WebKit developers should fix warnings, not ignore them. Understanding that you might choose to use -Wno-error locally, are you OK with that, Carlos?
Comment 23 Michael Catanzaro 2021-06-03 05:39:42 PDT
Reopening.
Comment 24 Michael Catanzaro 2021-06-03 14:35:08 PDT
Created attachment 430499 [details]
Patch
Comment 25 Michael Catanzaro 2021-06-03 14:39:46 PDT
Comment on attachment 430499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430499&action=review

> Source/cmake/WebKitCompilerFlags.cmake:326
> +            # Since -Werror will cause the checks to fail, we've got to prepend it
> +            # it manually, without checking it. Also note we intentionally add
> +            # this at the bottom of this file, after testing all the other flags,
> +            # since it may break any subsequent compiler tests.

So here's the problem: this doesn't actually work. WebKitCompilerFlags.cmake is not the last place we add compiler flags. We do it in every WebKit framework project as well. It breaks things like this:

WEBKIT_ADD_TARGET_CXX_FLAGS(gtest -Wno-undef
                                                                             -Wno-stringop-truncation
                                                                            -Wno-suggest-attribute=format)

That one's from Source/ThirdParty/gtest/CMakeLists.txt, but we have similar code in most of our CMakeLists.txt.

After appending -Werror, basically all CMake compiler flags tests start failing. In CMakeError.log the failure looks like this:

Source file was:
int main() { return 0; }
Performing C++ SOURCE FILE Test CXX_COMPILER_SUPPORTS_-Wno-undef failed with the following output:
Change Dir: /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/CMakeFiles/CMakeTmp

Run Build Command(s):/usr/bin/ninja-build cmTC_8fffa && [1/2] Building CXX object CMakeFiles/cmTC_8fffa.dir/src.cxx.o
FAILED: CMakeFiles/cmTC_8fffa.dir/src.cxx.o 
/usr/lib64/ccache/c++   -Werror -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare -g -O0 -fno-strict-aliasing -fno-exceptions -fno-rtti  -fPIE   -DCXX_COMPILER_SUPPORTS_-Wno-undef -Wno-undef -std=c++17 -o CMakeFiles/cmTC_8fffa.dir/src.cxx.o -c src.cxx
<command-line>: error: ISO C++11 requires whitespace after the macro name [-Werror]
cc1plus: all warnings being treated as errors
ninja: build stopped: subcommand failed.

To make this work we're going to have to figure out how to find the source code that cmake_check_cxx_compiler_flag() is trying to build and figure out how to make it not trip this macro whitespace problem. Then hope that CMake's test never trips any other warnings at any point in the future. That might very well be too much to expect....

CMake should probably use -Wno-error for its compiler flags tests, but that might be controversial.
Comment 26 Michael Catanzaro 2021-07-02 11:55:56 PDT
OK I don't think we can do this using CMAKE_CXX_FLAGS since it interferes with cmake_check_cxx_compiler_flag(). We'll have to add it in separately when building targets....