Bug 199108 - [cmake] Switch to built-in handling of C++ standard instead of hardcoding -std=c++17
Summary: [cmake] Switch to built-in handling of C++ standard instead of hardcoding -st...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Konstantin Tokarev
URL:
Keywords:
Depends on: 199181
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-21 09:11 PDT by Konstantin Tokarev
Modified: 2019-06-27 00:11 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.08 KB, patch)
2019-06-21 09:30 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (4.39 KB, patch)
2019-06-22 07:40 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (4.14 KB, patch)
2019-06-26 11:30 PDT, Konstantin Tokarev
don.olmstead: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.62 MB, application/zip)
2019-06-26 15:29 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Tokarev 2019-06-21 09:11:27 PDT
Rationale:

1. It provides an abstraction over exact flags of particular compilers - we just specify required version of C++ standard
2. External libraries (like Qt used in WPEQt) may require particular C++ standard or even particular C++ features to be availabe (e.g. Qt requires INTERFACE_COMPILE_FEATURES cxx_decltype, which causes cmake to add -std=gnu++11 unless proper CXX_STANDARD is defined)
Comment 1 Konstantin Tokarev 2019-06-21 09:30:16 PDT
Created attachment 372630 [details]
Patch
Comment 2 Konstantin Tokarev 2019-06-21 09:45:01 PDT
It seems like CXX_STANDARD 17 is nonly available in CMake >= 3.8 :(
Comment 3 Michael Catanzaro 2019-06-22 06:55:45 PDT
You can use CMake version checks to do it this way with new CMake and do it manually in old CMake, and trust us to remove the old way next time CMake version is bumped.

Or you can give up and close this.
Comment 4 Konstantin Tokarev 2019-06-22 07:03:25 PDT
Debian Stretch is shipping 3.7 in their main repository[1], however up-to-date  versions of WebKitGTK are only available in backports[2], where cmake 3.13 is availble. Ubuntu 18.04 ships cmake 3.10. I wonder if we could raise cmake requirement to 3.8 right now.

OTOH, this particular change is not that important, I can even keep it in my fork

[1] https://packages.debian.org/stretch/cmake
[2] https://packages.debian.org/stretch-backports/libwebkit2gtk-4.0-37
[3] https://packages.debian.org/stretch-backports/cmake
[4] https://packages.ubuntu.com/bionic/cmake
Comment 5 Konstantin Tokarev 2019-06-22 07:40:11 PDT
Created attachment 372679 [details]
Patch
Comment 6 Konstantin Tokarev 2019-06-22 09:56:30 PDT
Comment on attachment 372679 [details]
Patch

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

> Source/WebKit/PlatformWPE.cmake:-400
> -        CXX_STANDARD 17

This code is removed unconditionally because will result in error with cmake < 3.8
Comment 7 Fujii Hironori 2019-06-23 20:20:49 PDT
(In reply to Konstantin Tokarev from comment #4)
> Debian Stretch is shipping 3.7 in their main repository[1], however
> up-to-date  versions of WebKitGTK are only available in backports[2], where
> cmake 3.13 is availble. Ubuntu 18.04 ships cmake 3.10. I wonder if we could
> raise cmake requirement to 3.8 right now.

Supporting Debian 9 (stretch) has been dropped.
https://lists.webkit.org/pipermail/webkit-dev/2019-May/030619.html
https://lists.webkit.org/pipermail/webkit-dev/2019-May/030625.html

We can bump cmake_minimum_required version to 3.10 now.
Comment 8 Michael Catanzaro 2019-06-24 05:47:01 PDT
(In reply to Fujii Hironori from comment #7)
> We can bump cmake_minimum_required version to 3.10 now.

Yeah that's true. Go for it. If any EWS complain, go for it anyway and we'll get them fixed.
Comment 9 Konstantin Tokarev 2019-06-24 12:14:18 PDT
It's already known that EWS will compalin - if it had cmake >= 3.8 it wouldn't complain about original patch
Comment 10 Don Olmstead 2019-06-24 12:38:38 PDT
This is good. As long as Michael is happy Iā€™m fine with the contents. We use 3.14 cmake so our platforms have no problem with this.
Comment 11 Konstantin Tokarev 2019-06-26 11:30:44 PDT
Created attachment 372938 [details]
Patch
Comment 12 Build Bot 2019-06-26 15:29:30 PDT
Comment on attachment 372938 [details]
Patch

Attachment 372938 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12583556

New failing tests:
svg/text/textpath-reference-update.html
Comment 13 Build Bot 2019-06-26 15:29:32 PDT
Created attachment 372956 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 14 Don Olmstead 2019-06-26 17:37:17 PDT
Comment on attachment 372938 [details]
Patch

r=me but you forgot to remove the following in OptionsMSVC.cmake

# Enable C++17
# https://docs.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version
add_compile_options(/std:c++17)
Comment 15 Konstantin Tokarev 2019-06-27 00:11:29 PDT
Committed r246873: <https://trac.webkit.org/changeset/246873>