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.
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.
(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.
(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)
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.
Another source of warnings is unused arguments in case function contains #if guards. Silencing that would require addition of lots of source lines
(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.
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.
Created attachment 278993 [details] Patch
(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.
(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.
I'd still like to do this, and for WPE as well. Developers must not ignore compiler warnings, they must be fixed or suppressed.
Let's start with GTK only for now.
Created attachment 315365 [details] Patch
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.
(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 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
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
Note the EWS failure is fixed in bug #174463.
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.
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.
(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.
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?
Reopening.
Created attachment 430499 [details] Patch
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.
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....
Created attachment 453403 [details] Patch
Note that EWS will fail because (1) patch in bug #237275 has not landed yet, and (2) I haven't tested the same build configuration used by the EWS bots, so they're very likely to have unfixed warnings.
Created attachment 453746 [details] Patch
My main concern here is that each clang update always brings with it a set of new warnings so whenever our SDK updates with a new clang release our builds will definitely go red.
(In reply to Don Olmstead from comment #30) > My main concern here is that each clang update always brings with it a set > of new warnings so whenever our SDK updates with a new clang release our > builds will definitely go red. For bots, we shouldn't enable developer mode.
(In reply to Basuke Suzuki from comment #31) > (In reply to Don Olmstead from comment #30) > > My main concern here is that each clang update always brings with it a set > > of new warnings so whenever our SDK updates with a new clang release our > > builds will definitely go red. > > For bots, we shouldn't enable developer mode. By default build-webkit uses developer mode. https://github.com/WebKit/WebKit/blob/main/Tools/Scripts/webkitdirs.pm#L2555-L2556 PlayStation port isn't listed there but it should be.
(In reply to Don Olmstead from comment #30) > My main concern here is that each clang update always brings with it a set > of new warnings so whenever our SDK updates with a new clang release our > builds will definitely go red. Yup, that's the goal. Fixing new warnings is an expected part of updating to a newer compiler. (In reply to Basuke Suzuki from comment #31) > For bots, we shouldn't enable developer mode. Although all bots necessarily use developer mode, the non-EWS bots will use --no-fatal-warnings.
(In reply to Michael Catanzaro from comment #33) > (In reply to Don Olmstead from comment #30) > > My main concern here is that each clang update always brings with it a set > > of new warnings so whenever our SDK updates with a new clang release our > > builds will definitely go red. > > Yup, that's the goal. Fixing new warnings is an expected part of updating to > a newer compiler. > > (In reply to Basuke Suzuki from comment #31) > > For bots, we shouldn't enable developer mode. > > Although all bots necessarily use developer mode, the non-EWS bots will use > --no-fatal-warnings. That seems reasonable to me. Carry on.
Pull request: https://github.com/WebKit/WebKit/pull/4437
Committed 254583@main (1460c1507d3b): <https://commits.webkit.org/254583@main> Reviewed commits have been landed. Closing PR #4437 and removing active labels.
<rdar://problem/100058793>