Page MenuHomeFreeBSD

www/qt5-webengine: update to 5.12.1
ClosedPublic

Authored by kai on Jan 5 2019, 8:10 PM.

Details

Summary

Import the update of Kai Knoblich <freebsd_ports@k-worx.org>

Test Plan
  • 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

Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23288
Build 22320: arc lint + arc unit

Event Timeline

tcberner created this revision.Jan 5 2019, 8:10 PM
novel added a subscriber: novel.Jan 13 2019, 3:58 PM
kai updated this revision to Diff 55230.Mar 19 2019, 1:02 PM

Update www/qt5-webengine to 5.12.1. Many kudos to rakuco and tcberner!

kai retitled this revision from www/qt5-webengine: update to 5.12.0 to www/qt5-webengine: update to 5.12.1.Mar 19 2019, 1:09 PM
kai edited the test plan for this revision. (Show Details)
kai updated this revision to Diff 55245.Mar 19 2019, 7:39 PM

Add missing debug files in pkg-plist.

tcberner accepted this revision.Mar 21 2019, 10:43 AM

As long as @rakuco has nothing to add, ship it!

Macro stlgtm:

This revision is now accepted and ready to land.Mar 21 2019, 10:43 AM
In D18757#420567, @kai wrote:

Add missing debug files in pkg-plist.

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.

kai updated this revision to Diff 55325.Mar 21 2019, 4:27 PM

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.

This revision now requires review to proceed.Mar 21 2019, 4:27 PM

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?

kai commandeered this revision.Mar 24 2019, 9:24 PM
kai removed a reviewer: kai.
kai updated this revision to Diff 55414.Mar 24 2019, 11:19 PM

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
kai marked 4 inline comments as done.Mar 24 2019, 11:29 PM
kai added inline comments.
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);
kai updated this revision to Diff 55415.Mar 24 2019, 11:33 PM

Really add back files/patch-config.tests-enable-on-FreeBSD

rakuco added inline comments.Mar 25 2019, 12:55 PM
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:

  • patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.cc
  • patch-src_3rdparty_chromium_content_browser_webui_web__ui__data__source__impl.h
  • patch-src_3rdparty_chromium_content_browser_webui_web__ui__impl.cc
  • patch-src_3rdparty_chromium_content_browser_webui_web__ui__impl.h
  • patch-src_3rdparty_chromium_content_public_browser_web__ui.h
  • patch-src_3rdparty_chromium_content_public_browser_web__ui__data__source.h
kai updated this revision to Diff 55427.Mar 25 2019, 2:07 PM

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/

kai marked 2 inline comments as done.Mar 25 2019, 2:15 PM
kai added inline comments.
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?

rakuco accepted this revision.Mar 25 2019, 2:45 PM

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.

This revision is now accepted and ready to land.Mar 25 2019, 2:45 PM
kai marked an inline comment as done.Mar 25 2019, 3:40 PM

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.

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...

In D18757#422029, @kai wrote:
  • File a bug for tracking the KDE-inotify bug.

If I'm not mistaken the Chromium Bug-Tracker is the right platform for that bug?

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.

kai added a comment.Mar 25 2019, 5:14 PM

If I'm not mistaken the Chromium Bug-Tracker is the right platform for that bug?

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.

Ah, k, the bug has been filed under PR236787 . The builders are running...it's only a matter of hours now.

rakuco added inline comments.Mar 27 2019, 8:08 PM
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).

kai marked an inline comment as done.EditedMar 27 2019, 8:35 PM

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.

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.

kai added a comment.Mar 27 2019, 10:13 PM

I trust you tested it well enough :) so just go ahead.

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 .

arrowd added a subscriber: arrowd.Apr 9 2019, 4:59 AM

This can be closed, I guess.

kai closed this revision.Apr 14 2019, 11:14 AM

This can be closed, I guess.

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."

In D18757#427717, @kai wrote:

This can be closed, I guess.

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."

Yeah, big diffs tend to never get auto closed... well sometimes after weeks :D