Similar to what happened with std::tie in bug 180692 it seems libstdc++-4 (as shipped on Debian Jessie) doesn't have a version of std::max or std::min annotated with constexpr, so build fails after r225769 <https://trac.webkit.org/r225769> Build failure: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/8668/steps/compile-webkit/logs/stdio Related: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60271
I have one question, do we need to support libstdc++4? Our GCC baseline is now 5.2. And I think Debian jessie is not supported by WebKit ToT right now since we have Debian stretch, which is newer stable version of Debian. While std::tie's issue is not fixed in libstdc++5, this std::{max,min} is fixed in libstdc++5.
(In reply to Yusuke Suzuki from comment #1) > I have one question, do we need to support libstdc++4? > Our GCC baseline is now 5.2. And I think Debian jessie is not supported by > WebKit ToT right now since we have Debian stretch, which is newer stable > version of Debian. While std::tie's issue is not fixed in libstdc++5, this > std::{max,min} is fixed in libstdc++5. Per WebKitGTK+ Dependencies Policy https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy we should try to support Debian stable releases up to one year after the next release (even when that means using a non-default toolchain). Debian 9 (Stretch) was released on 2017-06. Which means that we should try to support Debian 8 (Jessie) up to 2018-06. For that reason the Debian stable bot is still running Debian 8 (Jessie). Since GCC-4.9 is not modern enough to build WebKit, it is using as compiler clang-3.8 from the repository jessie-backports.
Created attachment 330191 [details] Patch
Created attachment 330192 [details] Patch Try to fix Mac build: add algorithm header
Created attachment 330194 [details] Patch Try to fix Win build: disambiguate usage of min and max in UniscribeController.cpp
Created attachment 330198 [details] Patch Try to fix Win build (2): disambiguate usage of max in MediaPlayerPrivateAVFoundationCF.cpp
Created attachment 330200 [details] Patch Try to fix Win build (3): disambiguate usage of min in PluginView.cpp
I don't know. libstdc++4 is *really* old. Would it be possible to change the bot to static link to libc++? There is no C++ in the WebKit API, so we don't need to worry about C++ ABI, so I don't think dynamic linking to the system libstdc++ should be required.
(In reply to Michael Catanzaro from comment #8) > I don't know. libstdc++4 is *really* old. Would it be possible to change the > bot to static link to libc++? There is no C++ in the WebKit API, so we don't > need to worry about C++ ABI, so I don't think dynamic linking to the system > libstdc++ should be required. Regarding static linking WebKit: That's would be the holy grail! I have tried several times to do that (for different reasons) but I have never managed to get it working. If you know how to achieve this I'm certainly interested. Also take into account that even if you manage to do this certain dependencies of WebKit like glib/gio dlopen() libraries. Regarding using libc++: Maybe its possible, but what would be the point? I don't think any of our downstream users is interested in building WK with libc++. Rather than moving to libc++ I will just give up on supporting Debian jessie before the promised year and move the bot to Debian Stretch. Another options to fix this is building FontSelectionAlgorithm.[h/cpp] with -Wno-invalid-constexpr. That will make clang happy.
Comment on attachment 330200 [details] Patch Attachment 330200 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5836611 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/update.https.html
Created attachment 330203 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
I think the a(In reply to Build Bot from comment #10) > Comment on attachment 330200 [details] > Patch > > Attachment 330200 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/5836611 > > New failing tests: > imported/w3c/web-platform-tests/service-workers/service-worker/ > ServiceWorkerGlobalScope/update.https.html I think this failure is totally unrelated (the EWS seems flaky)
(In reply to Carlos Alberto Lopez Perez from comment #9) > Another options to fix this is building FontSelectionAlgorithm.[h/cpp] with > -Wno-invalid-constexpr. That will make clang happy. This would be less intrusive, IMO.
(In reply to Michael Catanzaro from comment #13) > (In reply to Carlos Alberto Lopez Perez from comment #9) > > Another options to fix this is building FontSelectionAlgorithm.[h/cpp] with > > -Wno-invalid-constexpr. That will make clang happy. > > This would be less intrusive, IMO. Unfortunately that switch is only recognized by Clang, but not by GCC, and I can't find a similar one for GCC. Therefore doing this it will fix only the build with libstdc++-4 and Clang but not with GCC (you can in theory use a newer GCC with an old libstdc++).
Comment on attachment 330200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330200&action=review > Source/WTF/wtf/StdLibExtras.h:470 > +template <class T> > +inline constexpr const T& min(const T& a, const T& b) > +{ > + return std::min(a, b); > +} > + > +template <class T> > +inline constexpr const T& max(const T& a, const T& b) > +{ > + return std::max(a, b); > +} If the new rule will be "don't use std::min(), better to use WTF::min()" can we add a style check or commit check or something to enforce it?
(In reply to Myles C. Maxfield from comment #15) > Comment on attachment 330200 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330200&action=review > > > Source/WTF/wtf/StdLibExtras.h:470 > > +template <class T> > > +inline constexpr const T& min(const T& a, const T& b) > > +{ > > + return std::min(a, b); > > +} > > + > > +template <class T> > > +inline constexpr const T& max(const T& a, const T& b) > > +{ > > + return std::max(a, b); > > +} > > If the new rule will be "don't use std::min(), better to use WTF::min()" can > we add a style check or commit check or something to enforce it? I think this is not the new rule. The idea is to only use WTF::min when you expect std::min to return a constexpr result, but otherwise just use standard std::min. Regarding checking/enforcing it, on one hand I think is currently enough with the Ubuntu LTS and Debian buildbots that will turn red if the build breaks on them. We are ok with carrying with the burden of fixing this after the fact. On the other hand, this will be only a temporally situation, because on 2018-06 we will be updating this bot to the next release of Debian, so then we can officially stop supporting libstdc++-4 and we can remove this workaround from the WebKit code. I will add a note on the comments about this timing to not forget.
Committed r226299: <https://trac.webkit.org/changeset/226299>
(In reply to Carlos Alberto Lopez Perez from comment #9) > Regarding static linking WebKit: That's would be the holy grail! Too much typos in word "sh*t" :P Every c++ package depending on webkit will be screwed, as distro will build it against system library.
(In reply to Konstantin Tokarev from comment #18) > (In reply to Carlos Alberto Lopez Perez from comment #9) > > Regarding static linking WebKit: That's would be the holy grail! > > Too much typos in word "sh*t" :P > > Every c++ package depending on webkit will be screwed, as distro will build > it against system library. There is no C++ in our API, so I don't see why it would be a problem. InjectedBundle loading does use C++, but not the standard library, so that should be fine too.
Reality is that it's impossible to mix C++ standard libraries on Linux in one application. See e.g. http://lists.qt-project.org/pipermail/development/2017-March/029115.html, and I can probably find other email where Thiago explains the issue.
Committed r226300: <https://trac.webkit.org/changeset/226300>
Committed r226301: <https://trac.webkit.org/changeset/226301>
Committed r226302: <https://trac.webkit.org/changeset/226302>
(In reply to Michael Catanzaro from comment #19) > (In reply to Konstantin Tokarev from comment #18) > > (In reply to Carlos Alberto Lopez Perez from comment #9) > > > Regarding static linking WebKit: That's would be the holy grail! > > > > Too much typos in word "sh*t" :P > > > > Every c++ package depending on webkit will be screwed, as distro will build > > it against system library. > > There is no C++ in our API, so I don't see why it would be a problem. > InjectedBundle loading does use C++, but not the standard library, so that > should be fine too. What about someone implementing the UIProcess on C++? Even when there is no C++ in our API, there is WebKit-related C++ code under the hood on the UIProcess.
(In reply to Carlos Alberto Lopez Perez from comment #24) > (In reply to Michael Catanzaro from comment #19) > > (In reply to Konstantin Tokarev from comment #18) > > > (In reply to Carlos Alberto Lopez Perez from comment #9) > > > > Regarding static linking WebKit: That's would be the holy grail! > > > > > > Too much typos in word "sh*t" :P > > > > > > Every c++ package depending on webkit will be screwed, as distro will build > > > it against system library. > > > > There is no C++ in our API, so I don't see why it would be a problem. > > InjectedBundle loading does use C++, but not the standard library, so that > > should be fine too. > > What about someone implementing the UIProcess on C++? Even when there is no > C++ in our API, there is WebKit-related C++ code under the hood on the > UIProcess. I mean: "What about someone implementing a MiniBrowser/launcher on C++?"
<rdar://problem/36261379>