Bug 222907 - [GTK] Add GTK4 tests expectations
Summary: [GTK] Add GTK4 tests expectations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Lauro Moura
URL:
Keywords: InRadar
Depends on:
Blocks: GTK4
  Show dependency treegraph
 
Reported: 2021-03-08 07:12 PST by Lauro Moura
Modified: 2021-03-16 06:53 PDT (History)
8 users (show)

See Also:


Attachments
Initial patch (11.53 KB, patch)
2021-03-09 20:56 PST, Lauro Moura
no flags Details | Formatted Diff | Diff
Patch with initial expectations (17.91 KB, patch)
2021-03-11 20:50 PST, Lauro Moura
no flags Details | Formatted Diff | Diff
Patch checking ldd output (12.30 KB, patch)
2021-03-14 22:02 PDT, Lauro Moura
no flags Details | Formatted Diff | Diff
Patch checking for filename (10.90 KB, patch)
2021-03-15 10:42 PDT, Lauro Moura
no flags Details | Formatted Diff | Diff
Patch without cmakeconfig check (8.39 KB, patch)
2021-03-16 04:51 PDT, Lauro Moura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lauro Moura 2021-03-08 07:12:14 PST
After r269945, we now have a GTK4 post-commit bot at [1]. It's currently exiting early due to some crashes and timeouts specific to GTK4.

As such, we need a way to define expectations specific to it.

[1] https://build.webkit.org/#/builders/GTK-Linux-64-bit-Release-GTK4-Tests
Comment 1 Lauro Moura 2021-03-09 20:56:09 PST
Created attachment 422796 [details]
Initial patch

Initial patch. To make the usage of gtk4 platform automatic, it first checks the contents of cmakeconfig.h, and if not present, runs an executable to check the GTK version. This fallback will be used in the test-only bots, as they do not download the cmakeconfig.h. It's missing the actual TestExpectation file and a way to test the MiniBrowser fallback check in the webkitpy unittests.\n\nIf this fallback feels too complicated, we can drop it and move to use some envvar in the bots to pass --additional-platform-directory.
Comment 2 Lauro Moura 2021-03-11 20:50:11 PST
Created attachment 423011 [details]
Patch with initial expectations
Comment 3 Philippe Normand 2021-03-12 04:52:12 PST
Comment on attachment 423011 [details]
Patch with initial expectations

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

> Tools/Scripts/webkitpy/port/gtk.py:307
> +            mb = self._build_path('bin', 'MiniBrowser')
> +            output = self._executive.run_command([mb, "--gtk-version"])
> +            return output.startswith("GTK 4")

Maybe the ldd output could be parsed?
Comment 4 Carlos Garcia Campos 2021-03-12 04:55:38 PST
Comment on attachment 423011 [details]
Patch with initial expectations

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

>> Tools/Scripts/webkitpy/port/gtk.py:307
>> +            return output.startswith("GTK 4")
> 
> Maybe the ldd output could be parsed?

I think we could just check the name of the lib, for GTK4 build the library is libwebkit2gtk-5.0.so
Comment 5 Carlos Garcia Campos 2021-03-12 05:00:13 PST
Comment on attachment 423011 [details]
Patch with initial expectations

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

> LayoutTests/platform/gtk4/TestExpectations:35
> +webkit.org/b/223108 http/tests/dom/noreferrer-window-not-targetable.html [ Crash ]

We can probably investigate and fix this bug before adding gtk4 specific expectations, I guess the bot is exiting early, so it's not easy to know other differences.
Comment 6 Lauro Moura 2021-03-14 22:02:10 PDT
Created attachment 423142 [details]
Patch checking ldd output

Also removed the MiniBrowser chances and the expectations from already solved bug, leaving only the expectations stub.
Comment 7 Carlos Garcia Campos 2021-03-15 03:09:28 PDT
(In reply to Carlos Garcia Campos from comment #4)
> Comment on attachment 423011 [details]
> Patch with initial expectations
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423011&action=review
> 
> >> Tools/Scripts/webkitpy/port/gtk.py:307
> >> +            return output.startswith("GTK 4")
> > 
> > Maybe the ldd output could be parsed?
> 
> I think we could just check the name of the lib, for GTK4 build the library
> is libwebkit2gtk-5.0.so

Isn't it possible to just check the library name?
Comment 8 Lauro Moura 2021-03-15 09:03:46 PDT
(In reply to Carlos Garcia Campos from comment #7)
> (In reply to Carlos Garcia Campos from comment #4)
> > Comment on attachment 423011 [details]
> > Patch with initial expectations
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=423011&action=review
> > 
> > >> Tools/Scripts/webkitpy/port/gtk.py:307
> > >> +            return output.startswith("GTK 4")
> > > 
> > > Maybe the ldd output could be parsed?
> > 
> > I think we could just check the name of the lib, for GTK4 build the library
> > is libwebkit2gtk-5.0.so
> 
> Isn't it possible to just check the library name?

Do you mean just checking if libwebkitgtk2gtk-5.0.so is present in `self._build_path('lib')`? That's an option too, avoiding the executive calls.

The only issue would be the corner case of reusing the build dir to build both GTK3/GTK4, but script can log a warning if it finds both.

I'll update the patch with this approach.
Comment 9 Lauro Moura 2021-03-15 09:07:19 PDT
(In reply to Lauro Moura from comment #8)
> The only issue would be the corner case of reusing the build dir to build
> both GTK3/GTK4, but script can log a warning if it finds both.
> 

Well, in this case the latest cmakeconfig would provide the right version. The issue would be the even more corner case of a bot downloading both the binary products of both versions.
Comment 10 Carlos Garcia Campos 2021-03-15 09:18:40 PDT
I don't think we should reuse the same build dir for gtk3 and gtk4 builds.
Comment 11 Lauro Moura 2021-03-15 10:42:40 PDT
Created attachment 423196 [details]
Patch checking for filename
Comment 12 Carlos Garcia Campos 2021-03-16 01:51:49 PDT
Comment on attachment 423196 [details]
Patch checking for filename

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

> Tools/Scripts/webkitpy/port/gtk.py:303
> +        # The test-only bots won't have a cmake config file as they download
> +        # only the binaries from the build-only bots.

This should work in both cases, I don't think we need to parse cmakeconfig.h
Comment 13 Lauro Moura 2021-03-16 04:51:46 PDT
Created attachment 423320 [details]
Patch without cmakeconfig check
Comment 14 EWS 2021-03-16 06:52:24 PDT
Committed r274474: <https://commits.webkit.org/r274474>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423320 [details].
Comment 15 Radar WebKit Bug Importer 2021-03-16 06:53:16 PDT
<rdar://problem/75475455>