Opened 2 years ago

Closed 2 years ago

#66165 closed defect (fixed)

qt5-qtwebengine @5.15.10_1 (aqua): Processing of port qt5-qtwebengine failed

Reported by: wurzelsand Owned by: MarcusCalhoun-Lopez (Marcus Calhoun-Lopez)
Priority: Normal Milestone:
Component: ports Version: 2.8.0
Keywords: Cc: chrstphrchvz (Christopher Chavez), mf2k (Frank Schima), devernay (Frédéric Devernay)
Port: qt5-qtwebengine

Description

Could not install qt5-qtwebengine on MacOS Ventura 13.0 (22A380) with Xcode Version 14.1 (14B47b). MacBook Pro Intel Core i5, 16 GB. Retried after sudo port clean qt5-qtwebengine.

Attachments (2)

main shortened.log (455.3 KB) - added by wurzelsand 2 years ago.
patch-qt5-qtwebengine-RectF-ambiguous.diff (2.9 KB) - added by kencu (Ken) 2 years ago.

Download all attachments as: .zip

Change History (23)

Changed 2 years ago by wurzelsand

Attachment: main shortened.log added

comment:1 Changed 2 years ago by jmroot (Joshua Root)

Cc: mcalhoun@… MarcusCalhoun-Lopez removed
Owner: set to MarcusCalhoun-Lopez
Status: newassigned

comment:2 Changed 2 years ago by kencu (Ken)

this issue is mentioned here, for a newer qtwebengine, but same error:

https://bugreports.qt.io/browse/QTBUG-108207?filter=-4&jql=text%20~%20%22qtwebengine%22%20order%20by%20created%20DESC

no fix as yet. I'll see what I can come up with. Some kind of cast might make the call unambiguous, but it's complicated c++ to work in...

upstream chromium has completely redone this and this call looks to no longer exist in this fashion upstream.

If all else fails, perhaps forcing some version of macports-clang-1[1-5] might work. Probably 14 would be a good try?

comment:3 Changed 2 years ago by kencu (Ken)

well, I'll embarass myself and post my last attempt, none of which worked:

diff --git a/src/3rdparty/chromium/third_party/blink/renderer/core/editing/finder/text_finder.cc b/src/3rdparty/chromium/third_party/blink/renderer/core/editing/finder/text_finder.cc
index e41a894fc..a1db7d77f 100644
--- a/src/3rdparty/chromium/third_party/blink/renderer/core/editing/finder/text_finder.cc
+++ b/src/3rdparty/chromium/third_party/blink/renderer/core/editing/finder/text_finder.cc
@@ -629,13 +629,13 @@ gfx::RectF TextFinder::ActiveFindMatchRect() {
   if (!current_active_match_frame_ || !active_match_)
     return gfx::RectF();
 
-  return gfx::RectF(FindInPageRectFromRange(EphemeralRange(ActiveMatch())));
+  return gfx::RectF((const CGRect&)FindInPageRectFromRange(EphemeralRange(ActiveMatch())));
 }
 
 Vector<gfx::RectF> TextFinder::FindMatchRects() {
   UpdateFindMatchRects();
 
-  Vector<gfx::RectF> match_rects;
+  Vector<(const CGRect&)gfx::RectF> match_rects;
   match_rects.ReserveCapacity(match_rects.size() + find_matches_cache_.size());
   for (const FindMatch& match : find_matches_cache_) {
     DCHECK(!match.rect_.IsEmpty());

perhaps someone who knows a little more c++ than I do can sort out how to do the cast right to remove the ambiguous call.

I'm going to try clang-14 next, as I have it installed... but it may have the same issue, as it's a similar vintage.

comment:4 Changed 2 years ago by kencu (Ken)

it turns out that forcing qt5-qtwebengine to use a specified compiler is not trivial, either. And then it wants to find other tools like dsymutil, that we also have to spec. Getting harder, not easier, down this path….

comment:5 Changed 2 years ago by chrstphrchvz (Christopher Chavez)

Likely not an ideal approach (and not something to propose to Qt without platform guards), but the only suggestion I currently have would be to try using a temporary CGRect:

--- a/src/3rdparty/chromium/third_party/blink/renderer/core/editing/finder/text_finder.cc
+++ b/src/3rdparty/chromium/third_party/blink/renderer/core/editing/finder/text_finder.cc
@@ -629,7 +629,8 @@ gfx::RectF TextFinder::ActiveFindMatchRect() {
   if (!current_active_match_frame_ || !active_match_)
     return gfx::RectF();
 
-  return gfx::RectF(FindInPageRectFromRange(EphemeralRange(ActiveMatch())));
+  CGRect r = FindInPageRectFromRange(EphemeralRange(ActiveMatch()));
+  return gfx::RectF(r);
 }
 
 Vector<gfx::RectF> TextFinder::FindMatchRects() {
@@ -639,7 +640,8 @@
   match_rects.ReserveCapacity(match_rects.size() + find_matches_cache_.size());
   for (const FindMatch& match : find_matches_cache_) {
     DCHECK(!match.rect_.IsEmpty());
-    match_rects.push_back(match.rect_);
+    CGRect r = match.rect_;
+    match_rects.push_back(r);
   }
 
   return match_rects;

PS I would appreciate if a volunteer could try building the 5.15.11 update: https://github.com/macports/macports-ports/pull/16311

comment:6 Changed 2 years ago by chrstphrchvz (Christopher Chavez)

Cc: chrstphrchvz added

comment:7 Changed 2 years ago by mf2k (Frank Schima)

Cc: mf2k added

comment:8 Changed 2 years ago by kencu (Ken)

Cc: mf2k removed
 % port -v installed qt5-qtwebengine
The following ports are currently installed:
  qt5-qtwebengine @5.15.11_0 (active) requested_variants='' platform='darwin 21' archs='x86_64' date='2022-11-06T12:59:07-0800'

comment:9 Changed 2 years ago by kencu (Ken)

The PR works, but the build on Monterey is also broken due to Xcode 14.1 -- Chris' idea worked. I hope it's the right bunch of fixes -- it seemed like they wanted to be CGRects. I will put up the patch for others to try.

A successful build on Ventura and we can PR it. Like Chris says, for upstream it will need __APPLE__ guards.

Changed 2 years ago by kencu (Ken)

comment:10 Changed 2 years ago by mf2k (Frank Schima)

Cc: mf2k added

comment:11 Changed 2 years ago by kencu (Ken)

Oh, sorry about that Frank -- my mistake there.

comment:12 Changed 2 years ago by kencu (Ken)

looks like this patch works on Ventura (Intel at least) as well:

% port -v installed qt5-qtwebengine
The following ports are currently installed:
  qt5-qtwebengine @5.15.11_0 (active) requested_variants='' platform='darwin 22' archs='x86_64' date='2022-11-06T17:44:34-0800'

now to use it a bit and try to see if anything is broken.

comment:13 Changed 2 years ago by kencu (Ken)

I put a PR here, for anyone who wants to test it. Also included is Christopher's update, as it builds on that PR.

https://github.com/macports/macports-ports/pull/16605

comment:14 Changed 2 years ago by chrstphrchvz (Christopher Chavez)

Ken, thanks for testing this and the update.

Looking again at float_rect.h and rect_f.h, I believe the intention was to rely on the gfx::RectF method of FloatRect for the conversion, and not the longer way of getting a CGRect from a FloatRect and then constructing a gfx::RectF from a CGRect; I would guess the existence of both approaches is what lead to the ambiguity error.

So a platform-agnostic alternative (probably still not ideal, but what I would prefer proposing to Qt) might be to duplicate what is done by the gfx::RectF method of FloatRect: use a temporary FloatRect and pass its x/y/width/height to the gfx::RectF(float x, float y, float width, float height) constructor, e.g.

-  return gfx::RectF(FindInPageRectFromRange(EphemeralRange(ActiveMatch())));
+  FloatRect r = FindInPageRectFromRange(EphemeralRange(ActiveMatch()));
+  return gfx::RectF(r.X(), r.Y(), r.Width(), r.Height());

comment:15 Changed 2 years ago by kencu (Ken)

Certainly both approaches indeed led to the ambiguity -- good for the compiler to fail to build it !

As it is a many-hours build, this is built, and I'm done for the time being, I'll test what I've got installed for a while here, while we see what upstream does with this.

Certainly 100% of current macOS systems are broken, Monterey and Ventura, so they'll have some heat on them to get it sorted properly once the "me toos" start piling on the upstream ticket.

comment:16 Changed 2 years ago by devernay (Frédéric Devernay)

Cc: devernay added

comment:17 Changed 2 years ago by chrstphrchvz (Christopher Chavez)

This is a duplicate of #66110

comment:18 Changed 2 years ago by devernay (Frédéric Devernay)

I applied manually the patches from the PR to a failed build (didn't want to rebuild everything), and the build succeeded. Thank you for those fixes! Is there anything that prevents the PR from being merged?

comment:19 Changed 2 years ago by kencu (Ken)

I was going to merge the PR if we had a few users who found the final product worked OK.

CGRect or FloatRect both handle floats, so I think they're about equivalent. Upstream doesn't seem too interested in this issue, and tells people to move on to qt6.4 which uses a newer chromium without this issue, so we may not get any upstream guidance.

Does the qt5-qtwebengine seem to work right for you, Fred? I tried one port with it and it seemed to work OK, but I don't know if I hit the area this patch affected.

comment:20 Changed 2 years ago by devernay (Frédéric Devernay)

It builds, and the fix is simple enough and seems correct, but I don't have a text case for that. shipit

comment:21 Changed 2 years ago by kencu (Ken)

Resolution: fixed
Status: assignedclosed

In efea74f90c9a10874e9ee249fa552c6cff525d35/macports-ports (master):

qt5-webengine: repair ambiguous constructor

newer clang versions error on this.

closes: #66165

[ci_skip]

Note: See TracTickets for help on using tickets.