Import the update of Kai Knoblich <freebsd_ports@k-worx.org>
Details
Details
- Reviewers
rakuco tcberner - Group Reviewers
kde - Commits
- rP496989: www/qt5-webengine: Update to 5.12.1
- F it, we'll do it live!
- Builds fine on 11.2-, 12.0-RELEASE, 13.0-CURRENT@r344874 amd64 + i386 for ALSA/SNDIO/PULSE
- Playback of multimedia content should be smoother (thanks to rakuco)
Diff Detail
Diff Detail
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 23288 Build 22320: arc lint + arc unit
Event Timeline
Comment Actions
That's not supposed to work, and I think the corresponding commit to the qtwebengine-5.12.1 branch is wrong as well. See https://github.com/freebsd/freebsd-ports-kde/commit/081752e6ca70e2fb3f66452928048e5b776f890a for a longer explanation.
Comment Actions
Prefixed *.debug entries with @comment in pkg-plist as suggested by @tcberner.
I forgot to mention that I also did some minor clean-up of some patches to reduce the overall churn.
Comment Actions
Thanks a lot for working on this. I've left some inline comments in some of the patches, but there isn't a lot that I think needs changing \o/
| www/qt5-webengine/files/patch-config.tests-enable-on-FreeBSD | ||
|---|---|---|
| 1 ↗ | (On Diff #55325) | I think it makes sense to keep this patch (which has an explanation and groups the related changes together) rather than splitting it up into two separate ones. |
| www/qt5-webengine/files/patch-configure.pri | ||
| 8 ↗ | (On Diff #55325) | Is this patch really necessary? We shouldn't be detecting glibc in the first place (and we seem to disable the check in the configure.prf patch), and this change is actually replacing ldd with a completely unrelated program. |
| www/qt5-webengine/files/patch-mkspecs_features_functions.prf | ||
| 4 | I think it makes sense to keep the patch explanation at the top. | |
| www/qt5-webengine/files/patch-src_3rdparty_chromium_chrome_app_generated__resources.grd | ||
| 8 | is_is_posix should be is_posix here. | |
| www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h | ||
| 1 ↗ | (On Diff #55325) | These web_ui patches (I think there are 4 or 5) that replace base::StringPiece usage with std::string seem wrong to me. StringPiece is a core part of Chromium (and it's used everywhere, so it should be working on FreeBSD), and removing those patches passed in my build-test on HEAD. |
| www/qt5-webengine/files/patch-src_3rdparty_chromium_net_proxy__resolution_proxy__config__service__linux.cc | ||
| 18 | Why do we need to disable KDE support here? | |
| www/qt5-webengine/files/patch-src_3rdparty_chromium_third__party_skia_src_images_SkJpegEncoder.cpp | ||
| 1 ↗ | (On Diff #55325) | Do you know why this patch (as well as patch-src_3rdparty_chromium_third__party_skia_src_ports_SkFontHost__FreeType.cpp and patch-src_3rdparty_chromium_ui_gfx_codec_jpeg__codec.cc) is needed? |
| www/qt5-webengine/files/patch-src_3rdparty_chromium_third__party_yasm_BUILD.gn | ||
| 1 ↗ | (On Diff #55325) | Do we need this? We're unbundling yasm. |
| www/qt5-webengine/files/patch-src_core_core__module.pro | ||
| 5 | Can you restore the patch explanation here? | |
Comment Actions
Addressing the most of @rakuco's suggestions:
- Restore patch-config.test-enable-on-FreeBSD (and remove both patch-config.tests_{khr_khr,snappy_snappy}.pro)
- Fixed typo in patch-src_3rdparty_chromium_chrome_app_generated_resources.grd
Restore explanations/boiler plates:
- patch-mkspecs_features_functions.prf
- patch-src_code_core__module.pro
Remove superflous patches:
- Remove superflous patch-configure.pri (it was a leftover from the phase where I tried to get around the glibc check)
- patch-src_3rdparty_chromium_third__party_skia_src_images_SkJpegEncoder.cpp
- patch-src_3rdparty_chromium_thirdparty_skia_src_ports_SkFontHostFreeType.cpp
- patch-src_3rdparty_chromium_third__party_yasm_BUILD.gn
- patch-src_3rdparty_chromium_ui_gfx_codec_jpeg__codec.cc
| www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h | ||
|---|---|---|
| 1 ↗ | (On Diff #55325) | Hrm, I removed all four patch-src_3rdparty_chromium_content_browser_webui_web__ui* patches and then the build failed with ../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/content/browser/webui/web_ui_impl.h:56:65: error: non-virtual member function marked 'override' hides virtual member function
const MessageCallback& callback) override;
^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/content/public/browser/web_ui.h:77:16: note: hidden overloaded virtual function 'content::WebUI::RegisterMessageCallback' declared here: type mismatch at 1st parameter ('const std::string &' (aka 'const basic_string<char, char_traits<char>, allocator<char> > &') vs 'base::StringPiece' (aka 'BasicStringPiece<basic_string<char, char_traits<char>, allocator<char> > >'))
virtual void RegisterMessageCallback(const std::string& message, |
| www/qt5-webengine/files/patch-src_3rdparty_chromium_net_proxy__resolution_proxy__config__service__linux.cc | ||
| 18 | I tried to build webengine without the both hunks that exclude the KDE support but it failed with: ../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/base/message_loop/message_loop_current.h:190:22: note: copy assignment operator of 'MessageLoopCurrent' is implicitly deleted because field 'current_' is of const-qualified type 'base::MessageLoop *const'
MessageLoop* const current_;
^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:589:19: error: use of undeclared identifier 'inotify_init'
inotify_fd_ = inotify_init();
^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:633:27: error: use of undeclared identifier 'IN_MODIFY'
IN_MODIFY | IN_MOVED_TO) < 0) {
^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:633:39: error: use of undeclared identifier 'IN_MOVED_TO'
IN_MODIFY | IN_MOVED_TO) < 0) {
^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:911:28: error: use of undeclared identifier 'inotify_event'
char event_buf[(sizeof(inotify_event) + NAME_MAX + 1) * 4];
^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:919:9: error: unknown type name 'inotify_event'
inotify_event* event = reinterpret_cast<inotify_event*>(event_ptr);
^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:919:49: error: unknown type name 'inotify_event'
inotify_event* event = reinterpret_cast<inotify_event*>(event_ptr);
^
../../../../qtwebengine-everywhere-src-5.12.1/src/3rdparty/chromium/net/proxy_resolution/proxy_config_service_linux.cc:921:37: error: use of undeclared identifier 'inotify_event'
CHECK_LE(event_ptr + sizeof(inotify_event), event_buf + r); | |
| www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h | ||
|---|---|---|
| 1 ↗ | (On Diff #55325) | Sorry, I should've been more specific. These are the files I removed locally to get the port to build:
|
Comment Actions
Remove further superfluous patches pointed out by @rakuco.
Many thanks for your all your hints, @rakuco, and that you took the time to look at it all!
Actually now only the thing with patch-src_3rdparty_chromium_net_proxyresolution_proxyconfigservicelinux.cc seems to be open? Otherwise nothing more would stand in the way to bring it finally into the ports tree... \o/
| www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h | ||
|---|---|---|
| 1 ↗ | (On Diff #55325) | Maybe it makes sense to remove those patches from the www/chromium port where I took them, as well? |
Comment Actions
Thank you very much for this patch. I think we're good to go, and the KDE inotify thing can be tracked separately. I haven't test-built the latest version of this patch in older releases like 11.2 though, it'd be good to do the following:
- File a bug for tracking the KDE-inotify bug.
- Test this on at least 11.2 if you haven't already (I can do it tonight if you prefer).
- Land and start worrying about the next update :-)
@tcberner had different results with the .debug files in the plist, but I think we can also follow this separately and not block the update on it.
| www/qt5-webengine/files/patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h | ||
|---|---|---|
| 1 ↗ | (On Diff #55325) | I think it would be a good idea, yes. It's not clear to me why they were needed in the first place. |
Comment Actions
If I'm not mistaken the Chromium Bug-Tracker is the right platform for that bug?
- Test this on at least 11.2 if you haven't already (I can do it tonight if you prefer).
I'll do a final build for all releases (11.2-, 12.0-RELEASE and 13.0-CURRENT@r345355 amd64 + i386) with the ALSA or SNDIO or PULSE (and with/without DEBUG) options enabled. I'm optimistic, that all builds will succeed.
- Land and start worrying about the next update :-)
I did some preparations for qt5-webengine 5.12.2 and as far I can tell is that the same set of patches as here with 5.12.1 work seamless. I did also a quick runtime test that went without any issues. ;-)
Well, qt5-webengine 5.13.x is another story...
Comment Actions
I meant a bug in our very own bugs.freebsd.org -- from an upstream Chromium point of view everything's working fine on Linux, we just need to fix that on FreeBSD.
Comment Actions
Ah, k, the bug has been filed under PR236787 . The builders are running...it's only a matter of hours now.
| www/qt5-webengine/files/freebsd.pri | ||
|---|---|---|
| 3 | Just one thing I forgot to mention before: when landing, you can get rid of disable_nacl here, this option doesn't exist in Chromium anymore (and GN warns about it during the build). | |
Comment Actions
Thank you for the pointer. I removed that line from the freebsd.pri file. I discovered one small issue with the patch-src_3rdparty_chromium_third__party_yasm_BUILD.gn:
That file already exist with the old webengine and I did accidentially a "svn revert" instead of "svn rm" and created this version of diff then. So that file isn't not listed in this diff but patching will fail then. On my builders the patch-src_3rdparty_chromium_third__party_yasm_BUILD.gn is removed with "svn rm" and all builds were successful so far (except the builds with debug symbols on 13-CURRENT i386 due memory exhaustion during the linking).
If you like I'll update this diff to reflect those minor changes for the sake of completeness.
Edit: Typo fix.
Comment Actions
If you like I'll update this diff to reflect those minor changes for the sake of completeness.
I trust you tested it well enough :) so just go ahead.
Comment Actions
Alright, I'll unleash the behemoth in a few minutes...
| www/qt5-webengine/files/freebsd.pri | ||
|---|---|---|
| 6 | The enable_hidpi variable seems also not to exist anymore but I think we can rid of that with the update to 5.12.2 . | |
Comment Actions
Thanks for your info. Indeed, I was wondering why this review was still open after www/qt5-webengine has been committed in rP496989. It seems that Phabricator has some problems with the import of that commit as it still display "This commit is still importing. Changes will be visible once the import finishes."