Bug 144747

Summary: [CMake] Ignore warnings in system headers
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: Tools / TestsAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Enhancement CC: achristensen, commit-queue, mcatanzaro, mrobinson, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=144959
Bug Depends on:    
Bug Blocks: 145121    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Michael Catanzaro
Reported 2015-05-07 09:25:21 PDT
To be able to see any warnings in our code when compiling with Clang, which gives tons of warnings for problems in other projects' header files, we need to mark them as SYSTEM headers. The first parameter to include_directories() should be SYSTEM, then all warnings from headers in those directories will be suppressed, using -isystem instead of -I.
Attachments
Patch (26.27 KB, patch)
2015-05-13 11:51 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (26.26 KB, patch)
2015-05-13 12:25 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (25.39 KB, patch)
2015-05-14 13:33 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 2015-05-13 08:04:37 PDT
The forthcoming patch is not ready for review, due to concerns about a CMake version bump: https://lists.webkit.org/pipermail/webkit-dev/2015-May/027436.html I think we might be able to do a WEBKIT_TARGET_INCLUDE_DIRECTORIES compatibility macro. The implementation would not be pretty, though. But I see almost everywhere we need this is in PlatformGTK.cmake, with just one exception. I'm thinking the path of least resistance is to require CMake 2.8.12 only in PlatformGTK.cmake, and guard the one use that affects Mac with a version check.
Michael Catanzaro
Comment 2 2015-05-13 08:07:23 PDT
(In reply to comment #1) > But I > see almost everywhere we need this is in PlatformGTK.cmake, with just one > exception. Actually, we don't even need to do that. I was inconsistently treating our bundled ANGLE as a system library to suppress warnings from, but it's not a system library, we should show warnings from its headers, it's our responsibility because we bundle it. And that's the only case that matters for Macs.
Michael Catanzaro
Comment 3 2015-05-13 11:47:52 PDT
This is ready for review. I'm having trouble building it fully do to a different bug; let's see what EWS thinks.
Michael Catanzaro
Comment 4 2015-05-13 11:51:28 PDT
Created attachment 253040 [details] Patch
WebKit Commit Bot
Comment 5 2015-05-13 11:53:04 PDT
Attachment 253040 [details] did not pass style-queue: ERROR: Source/WebKit2/PlatformGTK.cmake:491: Alphabetical sorting problem. "PluginProcess/PluginControllerProxy.cpp" should be before "PluginProcess/EntryPoint/unix/PluginProcessMain.cpp". [list/order] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Michael Catanzaro
Comment 6 2015-05-13 12:25:52 PDT
Created attachment 253047 [details] Patch
WebKit Commit Bot
Comment 7 2015-05-13 12:28:24 PDT
Attachment 253047 [details] did not pass style-queue: ERROR: Source/WebKit2/PlatformGTK.cmake:491: Alphabetical sorting problem. "PluginProcess/PluginControllerProxy.cpp" should be before "PluginProcess/EntryPoint/unix/PluginProcessMain.cpp". [list/order] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Michael Catanzaro
Comment 8 2015-05-14 13:33:00 PDT
Created attachment 253140 [details] Patch
WebKit Commit Bot
Comment 9 2015-05-14 13:36:06 PDT
Attachment 253140 [details] did not pass style-queue: ERROR: Source/WebKit2/PlatformGTK.cmake:491: Alphabetical sorting problem. "PluginProcess/PluginControllerProxy.cpp" should be before "PluginProcess/EntryPoint/unix/PluginProcessMain.cpp". [list/order] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Gyuyoung Kim
Comment 10 2015-05-14 18:56:01 PDT
Comment on attachment 253140 [details] Patch I think EFL port also needs to use this patch.
Alex Christensen
Comment 11 2015-05-14 18:57:43 PDT
Comment on attachment 253140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253140&action=review > CMakeLists.txt:1 > -cmake_minimum_required(VERSION 2.8.8) > +cmake_minimum_required(VERSION 2.8.12) :)
Gyuyoung Kim
Comment 12 2015-05-14 19:08:27 PDT
Comment on attachment 253140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253140&action=review >> CMakeLists.txt:1 >> +cmake_minimum_required(VERSION 2.8.12) > > :) I believe you let Mac cmake ports use newer ver. in near future ;)
Michael Catanzaro
Comment 13 2015-05-14 20:36:04 PDT
(In reply to comment #11) > > CMakeLists.txt:1 > > -cmake_minimum_required(VERSION 2.8.8) > > +cmake_minimum_required(VERSION 2.8.12) > > :) Ah, I should have called that out: I decided to do this unconditionally since you decided it wouldn't hurt your efforts. Seems easiest to have one minimum for the entire build system.
WebKit Commit Bot
Comment 14 2015-05-18 18:36:05 PDT
Comment on attachment 253140 [details] Patch Clearing flags on attachment: 253140 Committed r184536: <http://trac.webkit.org/changeset/184536>
WebKit Commit Bot
Comment 15 2015-05-18 18:36:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.