Bug 186771 – [WPE]: Add a way to setup our development environment inside flatpak

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Thibault Saunier]

This aims at replacing jhbuild with a flatpak based workflow for our development
environment.

More information to come with the patch.

[Thibault Saunier]

Created attachment 342945 [details]
[WPE]: Add a way to setup our development environment inside flatpak

This patch introduces a script (webkit-flatpak) to setup and work with a development environment
based on flatpak[0] and flatpak-builder, basically as a replacement for jhbuild. 

This patch is not totally finished yet but I believe that it is a pretty good starting point, at least to get the discussion started.

The worflow works as follow:

1. Building WebKit

    # No arguments, setup a WPE environment on the first run, and launch MiniBrowser once done (use `--gtk` to build WebKitGTK)
    $ webkit-flatpak

    # Build anything required to build WebKit itself if needed, it builds only WebKit itself when nothing specified
    $ webkit-flatpak --build-app

When building webkit, we mount `WebKit/` sources into `/app/webkit` inside the sandbox to have a more reproducible build and so that
we can potentially rsync content of `WebKitBuild` from a build server without problem. The `build-webkit` script is used inside the sandbox,
(at /app/webkit/Tools/Script/build-webkit) and the same `--cmarkargs` and `--makeargs` are exposed.

Also `WebKitBuild/WPE/Release` is mounted on `/app/webkit/WebKitBuild/Release` so that we can build several ports in the same worktree.

To be able to run layout tests, the httpd server is built inside the sandbox and Xvfb is built for the GTK port.

2. Running Layout tests

We reuse the run-webkit-tests script and all arguments after `--test` are passed to that script. There might have regressions that we still need to hunt.

    $ webkit-flatpak [--gtk] [--debug] --tests ...

3. Retrieve stacktrace

Since everything runs inside a flatpak sandbox, gdb needs to be run from within the sandbox, the script exposes a way to do it
easily with the `--gdb` option:

    $ webkit-flatpak --gdb [-m COREDUMPCTL MATCHES]

The Layout test `GDBCrashLogGenerator` has been taugth how to use that and is able to retrieve stacktrace as with the jhbuild based workflow.

[Thibault Saunier]

Created attachment 342946 [details]
Patch

Added forgottent ChangeLogs

[Michael Catanzaro]

LGTM

[EWS Watchlist]

Comment on attachment 342946 [details]
Patch

Attachment 342946 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8233814

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 342957 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Carlos Alberto Lopez Perez]

Comment on attachment 342946 [details]
Patch

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

> Tools/ChangeLog:43
> +        The worflow works as follow:
> +
> +        1. Building WebKit
> +        
> +          # No arguments, setup a WPE environment on the first run, and launch MiniBrowser once done (use `--gtk` to build WebKitGTK)
> +          $ webkit-flatpak
> +
> +          # Build anything required to build WebKit itself if needed, it builds only WebKit itself when nothing specified
> +          $ webkit-flatpak --build-app
> +
> +        When building webkit, we mount `WebKit/` sources into `/app/webkit` inside the sandbox to have a more reproducible build and so that
> +        we can potentially rsync content of `WebKitBuild` from a build server without problem. The `build-webkit` script is used inside the sandbox,
> +        (at /app/webkit/Tools/Script/build-webkit) and the same `--cmarkargs` and `--makeargs` are exposed.
> +
> +        Also `WebKitBuild/WPE/Release` is mounted on `/app/webkit/WebKitBuild/Release` so that we can build several ports in the same worktree.
> +
> +        To be able to run layout tests, the httpd server is built inside the sandbox and Xvfb is built for the GTK port.
> +
> +        2. Running Layout tests
> +
> +        We reuse the run-webkit-tests script and all arguments after `--test` are passed to that script.
> +
> +          $ webkit-flatpak [--gtk] [--debug] --tests ...
> +
> +        3. Retrieve stacktrace
> +
> +        Since everything runs inside a flatpak sandbox, gdb needs to be run from within the sandbox, the script exposes a way to do it
> +        easily with the `--gdb` option:
> +
> +          $ webkit-flatpak --gdb [-m COREDUMPCTL MATCHES]
> +
> +        The Layout test `GDBCrashLogGenerator` has been taugth how to use that and is able to retrieve stacktrace as with the jhbuild based workflow.
> +

I think we should make the usage of this as transparent of possible, so if possible the way of building-webkit and running tests should remain unchanged.
The rationale is:
1. Avoid users having to learn about a new way of running tests or building webkit.
2. Avoid having to modify all the buildbot configs to pass new arguments.

Currently, the JHBuild tooling automatically runs everything under the JHBuild environment if possible (no need to pass any special argument to the script run-webkit-tests, etc). It does so by simply checking the existence of the directory WebKitBuild/Dependencies${PORT} and if that directory exists it just enters into the jhbuild environment before running any test or building anything. Check the function enter_jhbuild_environment_if_available() that its automatically called from the layout test scripts, run perf test scripts as well as from the API test scripts.


So I think this will work better if the arguments and command needed to build webkit and run the test steps remain unchanged.
The scripts doing this should somehow check if the flatpak environment has been initialized and then try to use it automatically, otherwise continue with current behaviour (try to use jhbuild)

[Thibault Saunier]

(In reply to Carlos Alberto Lopez Perez from comment #6)
> I think we should make the usage of this as transparent of possible, so if
> possible the way of building-webkit and running tests should remain
> unchanged.
> The rationale is:
> 1. Avoid users having to learn about a new way of running tests or building
> webkit.
> 2. Avoid having to modify all the buildbot configs to pass new arguments.
> 
> Currently, the JHBuild tooling automatically runs everything under the
> JHBuild environment if possible (no need to pass any special argument to the
> script run-webkit-tests, etc). It does so by simply checking the existence
> of the directory WebKitBuild/Dependencies${PORT} and if that directory
> exists it just enters into the jhbuild environment before running any test
> or building anything. Check the function
> enter_jhbuild_environment_if_available() that its automatically called from
> the layout test scripts, run perf test scripts as well as from the API test
> scripts.
> 
> 
> So I think this will work better if the arguments and command needed to
> build webkit and run the test steps remain unchanged.
> The scripts doing this should somehow check if the flatpak environment has
> been initialized and then try to use it automatically, otherwise continue
> with current behaviour (try to use jhbuild)

I was expecting a comment like that one and I understand the reasoning. While it
would be simple to keep the current behaviour I didn't do it this mostly because
of personal test and habits, I have not problem adding the jhbuild behaviour back
if you think it is worth it.

[Michael Catanzaro]

That's not what Carlos Lopez is asking for. He is asking you to modify our *existing* scripts to use flatpak. E.g. for build-webkit and run-webkit-tests to both make use of webkit-flatpak. Then we can remove the jhbuild support if everything is working fine after a short while.

The tricky part here is the language barrier (perl).

[Thibault Saunier]

(In reply to Michael Catanzaro from comment #8)
> That's not what Carlos Lopez is asking for. He is asking you to modify our
> *existing* scripts to use flatpak. E.g. for build-webkit and
> run-webkit-tests to both make use of webkit-flatpak. Then we can remove the
> jhbuild support if everything is working fine after a short while.

I know yes...

The main problem is mostly about the fact that in flatpak we should run **everything** in the sandbox (server is not on the host.. etc) so the implementation will be a bit different.
 
> The tricky part here is the language barrier (perl).

Yeah... that is also one of the reason I would like to avoid putting my hands in there :-)

[Carlos Alberto Lopez Perez]

(In reply to Thibault Saunier from comment #9)
> (In reply to Michael Catanzaro from comment #8)
> > That's not what Carlos Lopez is asking for. He is asking you to modify our
> > *existing* scripts to use flatpak. E.g. for build-webkit and
> > run-webkit-tests to both make use of webkit-flatpak. Then we can remove the
> > jhbuild support if everything is working fine after a short while.
> 
> I know yes...
> 
> The main problem is mostly about the fact that in flatpak we should run
> **everything** in the sandbox (server is not on the host.. etc) so the
> implementation will be a bit different.
>  

Why should be different?

Le't me rephrase me previous point. I think it maybe I didn't make it clear enough.

This patch proposes that for building webkit inside the flatpak and running tests with it, I need to do:

Tools/Scripts/webkit-flatpak
Tools/Scripts/webkit-flatpak --build-app
Tools/Scripts/webkit-flatpak [--gtk] [--debug] --tests ...

And I ask to be able to do instead:

Tools/Scripts/webkit-flatpak
Tools/Scripts/build-webkit --gtk
Tools/Scripts/run-webkit-tests --gtk


To make that possible the scripts build-webkit and run-webkit-tests should detect that the flapak environment has been initialized and is ready to be used, and then they should enter into it automatically before doing anything else.

I understand the problem of entering into the flatpak environment automatically is a bit trickier than in the case of JHBuild, because with JHBuild is just a matter of changing the process environment, but here we need to execute a new process inside a new chroot/namespace... 

I hope we can find a good way of making this work easily, perhaps a helper can be added that takes care of forking and re-exec inside the flatpak meanwhile keeping the stdout/stderr descriptors shared with the caller? Just a wild idea.. perhaps it doesn't work well in practice.

> > The tricky part here is the language barrier (perl).
> 
> Yeah... that is also one of the reason I would like to avoid putting my
> hands in there :-)

There is no much perl there... only for build-webkit, but the others for the tests (layout, perf and API) are all python.

[Thibault Saunier]

Created attachment 343178 [details]
Patch.

Reworked as discussed so that the various scripts work within flatpak.

[Thibault Saunier]

Created attachment 343181 [details]
Patch.

Fix run-gtk-test script, and build PyGObject for python2 as it is required by this script.

Those tests still fail because of a "(MiniBrowser:29): dbind-WARNING **: 17:27:17.924: Error retrieving accessibility bus address: org.freedesktop.DBus.Error.ServiceUnknown: org.freedesktop.DBus.Error.ServiceUnknown"
I started investigating that without success yet.

[Carlos Alberto Lopez Perez]

Comment on attachment 343181 [details]
Patch.

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

> Tools/Scripts/run-gtk-tests:133
> +    flatpakutils.run_in_sandbox_if_available(sys.argv)
> +    if not flatpakutils.is_sandboxed() and not jhbuildutils.enter_jhbuild_environment_if_available("gtk"):
>          print "***"
>          print "*** Warning: jhbuild environment not present. Run update-webkitgtk-libs before build-webkit to ensure proper testing."
>          print "***"

I think this message can be improved. We are telling the user to just use the jhbuild option without also telling about the possibility of using flatpak instead

> Tools/Scripts/run-webdriver-tests:81
> +        if not jhbuildutils.enter_jhbuild_environment_if_available(port.name()):
> +            print '***'
> +            print '*** Warning: jhbuild environment not present. Run update-webkitgtk-libs before build-webkit to ensure proper testing.'
> +            print '***'

ditto

> Tools/Scripts/run-wpe-tests:55
> +    flatpakutils.run_in_sandbox_if_available(sys.argv)
> +    if not flatpakutils.is_sandboxed() and not jhbuildutils.enter_jhbuild_environment_if_available("wpe"):
>          print "***"
>          print "*** Warning: jhbuild environment not present. Run update-webkitgtk-libs before build-webkit to ensure proper testing."
>          print "***"

ditto

> Tools/flatpak/flatpakutils.py:36
> +try:
> +    import yaml
> +except ImportError:
> +    sys.stderr.write("PyYaml not found, please install it before continuing\n")
> +    sys.exit(1)

I this is not necessary.
The standard python import error is already verbose enough for the user/dev to know he misses such dep in her environment.

> Tools/flatpak/org.webkit.GTK.yaml:50
> +- name: xvfb
> +  sources:
> +    - type: git
> +      url: https://anongit.freedesktop.org/git/xorg/xserver.git
> +      branch: xorg-server-1.19.6
> +    - type: patch
> +      path: patches/xvfb-0001-HACK-Avoid-compiling-a-kbm-file.patch
> +  config-opts:
> +    - --enable-xvfb
> +    - --disable-xwayland

Currently a very important part of the WebKitGTK+ tests is the Mesa software-only renderer which is installed in a different path (softGL preffix inside JHBuild).
That works as follows:
1. The JHBuild tooling builds an special version of Mesa that only enables a software-based llvmpipe software rasterizer as OpenGL (glx and egl) library. But it installs it in a special path (${jhbuild_bindir}/softGL)
2. The test suite automatically loads the llvmpipe software rendererer (by adding ${jhbuild_bindir}/softGL to LD_LIBRARY_PATH) when running layout tests, but not when running the MiniBrowser.
The idea is that when running layout tests with --display-server=xvfb (the default) or with --display-server=weston the llvmpipe swrast mesa renderer is used. This allows layout tests with WebKitGTK+ to work on machines/containers without a GPU. And it also allows results to be consistent no matter what kind of GPU drivers (or even no GPU) the user has.
3. However when running the MiniBrowser or when running layout tests with --display-server=xorg or --display-server=wayland this path is not set to LD_LIBRARY_PATH, so it runs with the system's real opengl libraries.

This functionality should be preserved with flatpak as well for WebKitGTK+.
For WPE this doesn't matter as its not possible currenlty to run WPE tests without a real GPU.

[Thibault Saunier]

Created attachment 343191 [details]
Patch.

(In reply to Carlos Alberto Lopez Perez from comment #13)
> Comment on attachment 343181 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343181&action=review
> 
> > Tools/Scripts/run-gtk-tests:133
> > +    flatpakutils.run_in_sandbox_if_available(sys.argv)
> > +    if not flatpakutils.is_sandboxed() and not jhbuildutils.enter_jhbuild_environment_if_available("gtk"):
> >          print "***"
> >          print "*** Warning: jhbuild environment not present. Run update-webkitgtk-libs before build-webkit to ensure proper testing."
> >          print "***"
> 
> I think this message can be improved. We are telling the user to just use
> the jhbuild option without also telling about the possibility of using
> flatpak instead

Fixed all occurence.
> 
> > Tools/flatpak/flatpakutils.py:36
> > +try:
> > +    import yaml
> > +except ImportError:
> > +    sys.stderr.write("PyYaml not found, please install it before continuing\n")
> > +    sys.exit(1)
> 
> I this is not necessary.
> The standard python import error is already verbose enough for the user/dev
> to know he misses such dep in her environment.

Removed.

> > Tools/flatpak/org.webkit.GTK.yaml:50
> > +- name: xvfb
> > +  sources:
> > +    - type: git
> > +      url: https://anongit.freedesktop.org/git/xorg/xserver.git
> > +      branch: xorg-server-1.19.6
> > +    - type: patch
> > +      path: patches/xvfb-0001-HACK-Avoid-compiling-a-kbm-file.patch
> > +  config-opts:
> > +    - --enable-xvfb
> > +    - --disable-xwayland
> 
> Currently a very important part of the WebKitGTK+ tests is the Mesa
> software-only renderer which is installed in a different path (softGL
> preffix inside JHBuild).
> That works as follows:
> 1. The JHBuild tooling builds an special version of Mesa that only enables a
> software-based llvmpipe software rasterizer as OpenGL (glx and egl) library.
> But it installs it in a special path (${jhbuild_bindir}/softGL)
> 2. The test suite automatically loads the llvmpipe software rendererer (by
> adding ${jhbuild_bindir}/softGL to LD_LIBRARY_PATH) when running layout
> tests, but not when running the MiniBrowser.
> The idea is that when running layout tests with --display-server=xvfb (the
> default) or with --display-server=weston the llvmpipe swrast mesa renderer
> is used. This allows layout tests with WebKitGTK+ to work on
> machines/containers without a GPU. And it also allows results to be
> consistent no matter what kind of GPU drivers (or even no GPU) the user has.
> 3. However when running the MiniBrowser or when running layout tests with
> --display-server=xorg or --display-server=wayland this path is not set to
> LD_LIBRARY_PATH, so it runs with the system's real opengl libraries.
> 
> This functionality should be preserved with flatpak as well for WebKitGTK+.
> For WPE this doesn't matter as its not possible currenlty to run WPE tests
> without a real GPU.

Added that same behaviour in flatpak.

[Carlos Alberto Lopez Perez]

Comment on attachment 343191 [details]
Patch.

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

> Tools/ChangeLog:14
> +        The workflow is very similar to the "jhbuild based" on except that when running `build-webkit` you need to pass
> +        the `--flatpak` argument, afterward, other script will detect that a flatpak based environment
> +        has been setup and will be used.

Can we have a way of enabling flatpak environment that can be called before "build-webkit" and can we make the "build-webkit" script smart enough to detect that this previous step has been called and do the expected thing?

The bots run a "jhbuild" step before the step "build-webkit". But the step "build-webkit" doesn't receive any special parameter. It simply auto-detects the JHBuild dir and enables it if its there.


So the idea is to replace the step "jhbuild" with a step "flatpak", but without having to pass any extra parameter to the step/script "build-webkit".

[Thibault Saunier]

Created attachment 343195 [details]
Patch.

(In reply to Carlos Alberto Lopez Perez from comment #15)
> Comment on attachment 343191 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343191&action=review
> 
> > Tools/ChangeLog:14
> > +        The workflow is very similar to the "jhbuild based" on except that when running `build-webkit` you need to pass
> > +        the `--flatpak` argument, afterward, other script will detect that a flatpak based environment
> > +        has been setup and will be used.
> 
> Can we have a way of enabling flatpak environment that can be called before
> "build-webkit" and can we make the "build-webkit" script smart enough to
> detect that this previous step has been called and do the expected thing?
> 
> The bots run a "jhbuild" step before the step "build-webkit". But the step
> "build-webkit" doesn't receive any special parameter. It simply auto-detects
> the JHBuild dir and enables it if its there.
> 
> 
> So the idea is to replace the step "jhbuild" with a step "flatpak", but
> without having to pass any extra parameter to the step/script "build-webkit".

You can now do `mkdir -p WebKitBuild/GTK/FlatpakTreeRelease && build-webkit --release --gtk` or
`mkdir -p WebKitBuild/WPE/FlatpakTreeRelease && build-webkit --debug --wpe`

Is that enough?

[Carlos Alberto Lopez Perez]

(In reply to Thibault Saunier from comment #16)
> Created attachment 343195 [details]
> Patch.
> 
> (In reply to Carlos Alberto Lopez Perez from comment #15)
> > Comment on attachment 343191 [details]
> > Patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=343191&action=review
> > 
> > > Tools/ChangeLog:14
> > > +        The workflow is very similar to the "jhbuild based" on except that when running `build-webkit` you need to pass
> > > +        the `--flatpak` argument, afterward, other script will detect that a flatpak based environment
> > > +        has been setup and will be used.
> > 
> > Can we have a way of enabling flatpak environment that can be called before
> > "build-webkit" and can we make the "build-webkit" script smart enough to
> > detect that this previous step has been called and do the expected thing?
> > 
> > The bots run a "jhbuild" step before the step "build-webkit". But the step
> > "build-webkit" doesn't receive any special parameter. It simply auto-detects
> > the JHBuild dir and enables it if its there.
> > 
> > 
> > So the idea is to replace the step "jhbuild" with a step "flatpak", but
> > without having to pass any extra parameter to the step/script "build-webkit".
> 
> You can now do `mkdir -p WebKitBuild/GTK/FlatpakTreeRelease && build-webkit
> --release --gtk` or
> `mkdir -p WebKitBuild/WPE/FlatpakTreeRelease && build-webkit --debug --wpe`
> 
> Is that enough?

Its enough if that is done as part of the script that creates the initial environment for flatpak

Working with the new flatpak environment instead of jhbuild should require no extra or different arguments to any of the scripts, but the one to intialize&build third-party.


* With jhbuild we build third-party as follows:

  $ Tools/Scripts/update-webkitgtk-libs

And then we run all the other scripts (build-webkit, run-tests, etc) without passing any extra arguments. That scripts are smart enough to detect the JHBuild third-party dir has been initialized and they use it.

* So, for the new flatpak environment we need something like:

  $ Tools/Scripts/update-webkitgtk-flatpak (or any other name you wish)

And then all the scripts should detect that has been initialized and proceed to use flatpak

[Michael Catanzaro]

All the Apple EWS bots are failing with this error:

"Cannot get Flatpak path for platform that isn't GTK+ or WPE."

[Thibault Saunier]

Created attachment 343238 [details]
Patch.

Should fix build errors for ports that are not Gtk and WPE. Also build pyaml in the sandboxed
as it is required.

[Thibault Saunier]

(In reply to Carlos Alberto Lopez Perez from comment #17)
> Its enough if that is done as part of the script that creates the initial
> environment for flatpak
> 
> Working with the new flatpak environment instead of jhbuild should require
> no extra or different arguments to any of the scripts, but the one to
> intialize&build third-party.
> 
> 
> * With jhbuild we build third-party as follows:
> 
>   $ Tools/Scripts/update-webkitgtk-libs
> 
> And then we run all the other scripts (build-webkit, run-tests, etc) without
> passing any extra arguments. That scripts are smart enough to detect the
> JHBuild third-party dir has been initialized and they use it.
> 
> * So, for the new flatpak environment we need something like:
> 
>   $ Tools/Scripts/update-webkitgtk-flatpak (or any other name you wish)
> 
> And then all the scripts should detect that has been initialized and proceed
> to use flatpak

OK, I added those scripts that do just that.

[Carlos Alberto Lopez Perez]

Comment on attachment 343238 [details]
Patch.

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

> Tools/Scripts/run-gtk-tests:134
> +    flatpakutils.run_in_sandbox_if_available(sys.argv)
> +    if not flatpakutils.is_sandboxed() and not jhbuildutils.enter_jhbuild_environment_if_available("gtk"):
> +        print '***'
> +        print '*** Warning: jhbuild environment not present and not running in flatpak.'
> +        print '*** Run update-webkitgtk-libs before build-webkit or directly build-webkit --flatpak to ensure proper testing.'
> +        print '***'

This message now should reflect the last update.

- print '*** Run update-webkitgtk-libs before build-webkit or directly build-webkit --flatpak to ensure proper testing.'
+ print '*** Run update-webkitgtk-libs or update-webkitgtk-flatpak before build-webkit to ensure proper testing.'

> Tools/Scripts/run-webdriver-tests:82
> +        if not jhbuildutils.enter_jhbuild_environment_if_available(port.name()):
> +            print '***'
> +            print '*** Warning: jhbuild environment not present and not running in flatpak.'
> +            print '*** Run update-webkitgtk-libs before build-webkit or directly build-webkit --flatpak to ensure proper testing.'
> +            print '***'

ditto

> Tools/Scripts/run-wpe-tests:56
> +    if not flatpakutils.is_sandboxed() and not jhbuildutils.enter_jhbuild_environment_if_available("wpe"):
> +        print '***'
> +        print '*** Warning: jhbuild environment not present and not running in flatpak.'
> +        print '*** Run update-webkitgtk-libs before build-webkit or directly build-webkit --flatpak to ensure proper testing.'
> +        print '***'

ditto

> Tools/Scripts/update-webkitgtk-libs:-18
> -

This line deletion is not needed

> Tools/Scripts/update-webkitwpe-libs:-18
> -

ditto

> Tools/flatpak/org.webkit.GTK.yaml:62
> +- name: xvfb
> +  sources:
> +    - type: git
> +      url: https://anongit.freedesktop.org/git/xorg/xserver.git
> +      branch: xorg-server-1.19.6
> +    - type: patch
> +      path: patches/xvfb-0001-HACK-Avoid-compiling-a-kbm-file.patch

I think the patch xserver-search-for-DRI-drivers-at-LIBGL_DRIVERS_PATH-environ.patch should be preserved here.
That one is needed for making xvfb load the dri driver (swrast_dri.so) from the software-only mesa built at /app/softGL instead of the default's one.

[Thibault Saunier]

Created attachment 343279 [details]
Patch.

(In reply to Carlos Alberto Lopez Perez from comment #21)
> Comment on attachment 343238 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343238&action=review
> 
> > Tools/Scripts/run-gtk-tests:134
> > +    flatpakutils.run_in_sandbox_if_available(sys.argv)
> > +    if not flatpakutils.is_sandboxed() and not jhbuildutils.enter_jhbuild_environment_if_available("gtk"):
> > +        print '***'
> > +        print '*** Warning: jhbuild environment not present and not running in flatpak.'
> > +        print '*** Run update-webkitgtk-libs before build-webkit or directly build-webkit --flatpak to ensure proper testing.'
> > +        print '***'
> 
> This message now should reflect the last update.
> 
> - print '*** Run update-webkitgtk-libs before build-webkit or directly
> build-webkit --flatpak to ensure proper testing.'
> + print '*** Run update-webkitgtk-libs or update-webkitgtk-flatpak before
> build-webkit to ensure proper testing.'

Fixed, as well as for the following occurences.

> > Tools/Scripts/update-webkitgtk-libs:-18
> > -
> 
> This line deletion is not needed

Fixed, as well as for the following occurences.

> > Tools/flatpak/org.webkit.GTK.yaml:62
> > +- name: xvfb
> > +  sources:
> > +    - type: git
> > +      url: https://anongit.freedesktop.org/git/xorg/xserver.git
> > +      branch: xorg-server-1.19.6
> > +    - type: patch
> > +      path: patches/xvfb-0001-HACK-Avoid-compiling-a-kbm-file.patch
> 
> I think the patch
> xserver-search-for-DRI-drivers-at-LIBGL_DRIVERS_PATH-environ.patch should be
> preserved here.
> That one is needed for making xvfb load the dri driver (swrast_dri.so) from
> the software-only mesa built at /app/softGL instead of the default's one.

Indeed, added as well as the other patch for xserver and used the exact same configure options.

[Carlos Alberto Lopez Perez]

Comment on attachment 343279 [details]
Patch.

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

I'm testing it and I have this comment so far:

Each time you run build-webkit (or re-run update-webkitgtk-flatpak) it tries to update the flatpak environment, even when this has been already completed and the flatpak definition has not changed.

That is a process that is taking in my tests around 30-60 seconds even when there are no updates.
So this makes incremental builds kind of inconvenient.

Also its a process that seems to be dependent of networking available.. does that mean that I can't re-build webkit without an internet connection when I have already built the flatpak previously? that's bad.

So I think we should do something like we do with JHBuild, that is:

 - 1. When the JHBuild environment is (correctly) initialized we store a md5 hash of the input files to JHBuild (the moduleset). Check script update-webkit-libs-jhbuild
 - 2. The next time the JHBuild scripts are executed, first thing they do is to check if the input files have changed compared to that md5 hash
 - 3. If the input files have not changed, then they skip trigerring any update of the third-party/jhbuild directory
 
 
That allows it to trigger updates when needed (someone modified the moduleset), but avoids doing unneeded work otherwise.


Do you think something like that would be also possible for update-webkitgtk-flatpak?

> Tools/flatpak/flatpakutils.py:532
> +            version = output.decode("utf-8").split(" ")[1].strip("\n")
> +            if comparable_version(version) < comparable_version(required_version):
> +                Console.message("\n%s%s %s required but %s found."
> +                                " Please update and try again%s\n", Colors.FAIL,
> +                                app, version, version, Colors.ENDC)

This should be:
-                                app, version, version, Colors.ENDC)
+                                app, required_version, version, Colors.ENDC)

[Thibault Saunier]

(In reply to Carlos Alberto Lopez Perez from comment #23)
> Comment on attachment 343279 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343279&action=review
> 
> I'm testing it and I have this comment so far:
> 
> Each time you run build-webkit (or re-run update-webkitgtk-flatpak) it tries
> to update the flatpak environment, even when this has been already completed
> and the flatpak definition has not changed.
> 
> That is a process that is taking in my tests around 30-60 seconds even when
> there are no updates.
> So this makes incremental builds kind of inconvenient.

Really? I wonder how that is posible at all looking at the code.

Can you paste the output of `build-webkit` to see what happens?

> Also its a process that seems to be dependent of networking available.. does
> that mean that I can't re-build webkit without an internet connection when I
> have already built the flatpak previously? that's bad.
> 
> So I think we should do something like we do with JHBuild, that is:
> 
>  - 1. When the JHBuild environment is (correctly) initialized we store a md5
> hash of the input files to JHBuild (the moduleset). Check script
> update-webkit-libs-jhbuild
>  - 2. The next time the JHBuild scripts are executed, first thing they do is
> to check if the input files have changed compared to that md5 hash
>  - 3. If the input files have not changed, then they skip trigerring any
> update of the third-party/jhbuild directory
> That allows it to trigger updates when needed (someone modified the
> moduleset), but avoids doing unneeded work otherwise.
> 
> 
> Do you think something like that would be also possible for
> update-webkitgtk-flatpak?

flatpak-builder does all that for us (an clever caching of already built modules etc...)


> > Tools/flatpak/flatpakutils.py:532
> > +            version = output.decode("utf-8").split(" ")[1].strip("\n")
> > +            if comparable_version(version) < comparable_version(required_version):
> > +                Console.message("\n%s%s %s required but %s found."
> > +                                " Please update and try again%s\n", Colors.FAIL,
> > +                                app, version, version, Colors.ENDC)
> 
> This should be:
> -                                app, version, version, Colors.ENDC)
> +                                app, required_version, version, Colors.ENDC)

Fixed, will be part of the next update

[Carlos Alberto Lopez Perez]

(In reply to Thibault Saunier from comment #24)
> > So this makes incremental builds kind of inconvenient.
> 
> Really? I wonder how that is posible at all looking at the code.
> 
> Can you paste the output of `build-webkit` to see what happens?
> 



This happens:

1. I build correcty flatpak with Tools/Scripts/update-webkitgtk-flatpak
2. I try to build webkit with Tools/Scripts/build-webkit --gtk and I see this:

$ Tools/Scripts/build-webkit --gtk
Building flatpak based environment
Building org.webkit.GTK and dependencies in /home/clopez/webkit/webkit/WebKitBuild/GTK/FlatpakTreeRelease
Emptying app dir '/home/clopez/webkit/webkit/WebKitBuild/GTK/FlatpakTreeRelease'
Downloading sources
Fetching git repo https://github.com/apache/httpd.git, ref refs/tags/2.4.33
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 1 (delta 0), pack-reused 0
Unpacking objects: 100% (1/1), done.
Fetching git repo https://github.com/libevent/libevent.git, ref refs/tags/release-2.1.8-stable
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 1 (delta 0), pack-reused 0
Unpacking objects: 100% (1/1), done.
Fetching git repo https://chromium.googlesource.com/webm/libvpx, ref refs/tags/v1.7.0
remote: Counting objects: 1, done
remote: Finding sources: 100% (1/1)
remote: Total 1 (delta 0), reused 1 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching git repo https://anongit.freedesktop.org/git/gstreamer/gstreamer, ref refs/tags/1.14.1
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching full git repo https://anongit.freedesktop.org/git/gstreamer/common.git
Fetching git repo https://anongit.freedesktop.org/git/gstreamer/gst-plugins-base, ref refs/tags/1.14.1
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching full git repo https://anongit.freedesktop.org/git/gstreamer/common.git
Fetching git repo https://anongit.freedesktop.org/git/gstreamer/gst-plugins-good, ref refs/tags/1.14.1
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching full git repo https://anongit.freedesktop.org/git/gstreamer/common.git
Fetching git repo https://anongit.freedesktop.org/git/gstreamer/gst-plugins-ugly, ref refs/tags/1.14.1
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching full git repo https://anongit.freedesktop.org/git/gstreamer/common.git
Fetching git repo https://anongit.freedesktop.org/git/gstreamer/gst-plugins-bad, ref refs/tags/1.14.1
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 1 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching full git repo https://anongit.freedesktop.org/git/gstreamer/common.git
Fetching git repo https://github.com/WebKitGTK/webkitgtk-test-fonts.git, ref refs/heads/master
remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
Fetching git repo https://anongit.freedesktop.org/git/xorg/util/macros.git, ref refs/tags/util-macros-1.19.2
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching git repo https://anongit.freedesktop.org/git/xorg/font/util.git, ref refs/tags/font-util-1.3.1
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 1 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching git repo https://anongit.freedesktop.org/git/xorg/lib/libxkbfile.git, ref refs/tags/libxkbfile-1.0.9
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 1 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching git repo https://anongit.freedesktop.org/git/xorg/lib/libfontenc.git, ref refs/tags/libfontenc-1.1.3
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 1 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching git repo https://anongit.freedesktop.org/git/xorg/lib/libXfont.git, ref refs/tags/libXfont2-2.0.3
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (1/1), done.
Fetching git repo https://anongit.freedesktop.org/git/xorg/xserver.git, ref refs/tags/xorg-server-1.19.6
remote: Counting objects: 1, done.
remote: Total 1 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (1/1), done.
Stopping at module org.webkit.GTK
Starting build of org.webkit.GTK
Cache hit for apr, skipping build
Cache hit for apr-util, skipping build
Cache hit for httpd, skipping build
Cache hit for php, skipping build
Cache hit for libevent, skipping build
Cache hit for python3-pyaml, skipping build
Cache hit for python2-pyaml, skipping build
Cache hit for libvpx, skipping build
Cache hit for gstreamer, skipping build
Cache hit for gst-plugins-base, skipping build
Cache hit for gst-plugins-good, skipping build
Cache hit for gst-plugins-ugly, skipping build
Cache hit for gst-plugins-bad, skipping build
Cache hit for mesa, skipping build
Cache hit for webkitgtk-test-fonts, skipping build
Cache hit for xorg-util-macros, skipping build
Cache hit for xorg-font-util, skipping build
Cache hit for xkbfile, skipping build
Cache hit for fontenc, skipping build
Cache hit for xfont, skipping build
Cache hit for xvfb, skipping build
Cache hit for pycairo, skipping build
Cache hit for pygobject-python2, skipping build
Stopping at module org.webkit.GTK
Everything cached, checking out from cache
Pruning cache
Building webkit
Running in sandbox: "flatpak" "build" "--die-with-parent" "--bind-mount=/run/host//tmp=/tmp" "--bind-mount=/app/webkit=/home/clopez/webkit/webkit" "--bind-mount=/app/webkit/WebKitBuild/Release=/home/clopez/webkit/webkit/WebKitBuild/GTK/Release" "--env=LANG=en_US.utf8" "--env=WEBKIT_TOP_LEVEL=/app/" "--env=GTK_MODULES=gail:atk-bridge" "--env=TEST_RUNNER_INJECTED_BUNDLE_FILENAME=/app/webkit/lib/libTestRunnerInjectedBundle.so" "--env=DISPLAY=:0.0" "--share=ipc" "--socket=x11" "--socket=wayland" "--device=all" "--share=network" "--socket=pulseaudio" "--system-talk-name=org.freedesktop.GeoClue2" "--filesystem=host" "--socket=system-bus" "--talk-name=org.freedesktop.Flatpak" "/home/clopez/webkit/webkit/WebKitBuild/GTK/FlatpakTreeRelease" "/app/webkit/Tools/Scripts/build-webkit" "--release" "--gtk"

Use of uninitialized value $previousContents in chomp at /app/webkit/Tools/Scripts/webkitdirs.pm line 2027.
Use of uninitialized value $previousContents in string ne at /app/webkit/Tools/Scripts/webkitdirs.pm line 2030.
+  cmake --build /app/webkit/WebKitBuild/Release --config Release -- 
[...]

And If I retry another re-build I get also the same update process from flatpak

[Carlos Alberto Lopez Perez]

More minor issues:

====================================================================
 WebKit is now built (00m:01s). 
 To run MiniBrowser with this newly-built code, use the
 "../../../../app/webkit/Tools/Scripts/run-minibrowser" script.
====================================================================


That message should be fixed somehow, as the script name is just "Tools/Scripts/run-minibrowser"

[Thibault Saunier]

Created attachment 343333 [details]
Patch

(In reply to Carlos Alberto Lopez Perez from comment #26)
> More minor issues:
> 
> ====================================================================
>  WebKit is now built (00m:01s). 
>  To run MiniBrowser with this newly-built code, use the
>  "../../../../app/webkit/Tools/Scripts/run-minibrowser" script.
> ====================================================================
> 
> 
> That message should be fixed somehow, as the script name is just
> "Tools/Scripts/run-minibrowser"

Made sure we just print "Tools/Scripts/run-minibrowser" when running inside flatpak, we can't
really figure out the host path in there. 

I also fixed wrong check that was leading to updating flatpak when calling webkit-build.

+ Minor cleanup to check wheather `webkit-build` should be run inside flatpak.

[Dawei Fenton (:realdawei)]

(In reply to Thibault Saunier from comment #27)
> Created attachment 343333 [details]
> Patch
> 
> (In reply to Carlos Alberto Lopez Perez from comment #26)
> > More minor issues:
> > 
> > ====================================================================
> >  WebKit is now built (00m:01s). 
> >  To run MiniBrowser with this newly-built code, use the
> >  "../../../../app/webkit/Tools/Scripts/run-minibrowser" script.
> > ====================================================================
> > 
> > 
> > That message should be fixed somehow, as the script name is just
> > "Tools/Scripts/run-minibrowser"
> 
> Made sure we just print "Tools/Scripts/run-minibrowser" when running inside
> flatpak, we can't
> really figure out the host path in there. 
> 
> I also fixed wrong check that was leading to updating flatpak when calling
> webkit-build.
> 
> + Minor cleanup to check wheather `webkit-build` should be run inside
> flatpak.

It looks like the EWS bots that run run-webkit-tests do hot have the yaml module available. Is there an alternative solution for this? 

Sample error:
 Traceback (most recent call last):
  File "Tools/Scripts/run-webkit-tests", line 38, in <module>
    import flatpakutils
  File "./Tools/flatpak/flatpakutils.py", line 32, in <module>
    import yaml
ImportError: No module named yaml

Failed to run "['Tools/Scripts/run-webkit-tests', '--release', '--no-new-test-results', '--no-show-results', '--exit-after-n-failures=30', '--quiet', '--skip-failing-tests']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/run-webkit-tests", line 38, in <module>
    import flatpakutils
  File "./Tools/flatpak/flatpakutils.py", line 32, in <module>
    import yaml
ImportError: No module named yaml

[Thibault Saunier]

Created attachment 343401 [details]
Patch.

Lazy import yaml to avoid failling when importing flatpakutils

[Carlos Alberto Lopez Perez]

(In reply to Thibault Saunier from comment #29)
> Created attachment 343401 [details]
> Patch.
> 
> Lazy import yaml to avoid failling when importing flatpakutils

It looks this is making the mac EWS abort in the run-test step:

Running run-webkit-tests
Failed to run "['Tools/Scripts/run-webkit-tests', '--release', '--dump-render-tree', '--no-new-test-results', '--no-show-results', '--exit-after-n-failures=30', '--quiet', '--skip-failing-tests']" exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/run-webkit-tests", line 39, in <module>
    flatpakutils.run_in_sandbox_if_available(sys.argv)
  File "./Tools/flatpak/flatpakutils.py", line 772, in run_in_sandbox_if_available
    flatpak_runner = WebkitFlatpak.load_from_args(args)
  File "./Tools/flatpak/flatpakutils.py", line 459, in load_from_args
    self.clean_args()
  File "./Tools/flatpak/flatpakutils.py", line 561, in clean_args
    self.check_flatpak()
  File "./Tools/flatpak/flatpakutils.py", line 515, in check_flatpak
    output = subprocess.check_output([app, "--version"])
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 566, in check_output
    process = Popen(stdout=PIPE, *popenargs, **kwargs)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__
    errread, errwrite)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

From: https://webkit-queues.webkit.org/results/8327880

[Adrian Perez]

I finally got some spare cycles to apply this locally and try
things a bit. First and foremost: having a reproducible build
environment would be amazing, and I love the direction where
this patch is pointing to.

I have just one small request: the “update-webkitgtk-flatpak”
script requests user input, for example:

   % ./Tools/Scripts/update-webkitgtk-flatpak
   Updating Flatpak environment for GTK (Release)
   Looking for updates...
   Updating in user:
   org.gnome.Platform/x86_64/3.28 flathub 9785cf7dc622
   Is this ok [y/n]: _

Interaction can be avoided using “flatpak install --assumeyes”,
which would be better for automated environments like EWSs and
any other kind of build bot. Do you think it would be possible
to add this command line flag?

Anyway, awesome work!  \m/ (>_<) \m/

[Carlos Alberto Lopez Perez]

Comment on attachment 343401 [details]
Patch.

Setting r- to stop the EWS bots from retrying this again and again.

Not sure why the EWS bots are unable to claim this patch is bad for them instead of retrying forever. Seems a bug on the EWS logic..

In any case, this is the failure they have: https://webkit-queues.webkit.org/results/8341006

Please upload a fixed patch that avoids that error.

I suggest that the flatpak scripts should all return early if the system is not linux (or the port is not GTK/WPE).

[Thibault Saunier]

Created attachment 343610 [details]
Patch

(In reply to Adrian Perez from comment #31)
> I finally got some spare cycles to apply this locally and try
> things a bit. First and foremost: having a reproducible build
> environment would be amazing, and I love the direction where
> this patch is pointing to.
> 
> I have just one small request: the “update-webkitgtk-flatpak”
> script requests user input, for example:
> 
>    % ./Tools/Scripts/update-webkitgtk-flatpak
>    Updating Flatpak environment for GTK (Release)
>    Looking for updates...
>    Updating in user:
>    org.gnome.Platform/x86_64/3.28 flathub 9785cf7dc622
>    Is this ok [y/n]: _
> 
> Interaction can be avoided using “flatpak install --assumeyes”,
> which would be better for automated environments like EWSs and
> any other kind of build bot. Do you think it would be possible
> to add this command line flag?


Added the option.


(In reply to Carlos Alberto Lopez Perez from comment #32)
> Comment on attachment 343401 [details]
> Patch.
> 
> Setting r- to stop the EWS bots from retrying this again and again.
> 
> Not sure why the EWS bots are unable to claim this patch is bad for them
> instead of retrying forever. Seems a bug on the EWS logic..
> 
> In any case, this is the failure they have:
> https://webkit-queues.webkit.org/results/8341006
> 
> Please upload a fixed patch that avoids that error.
> 
> I suggest that the flatpak scripts should all return early if the system is
> not linux (or the port is not GTK/WPE).

I now check if we are on linux at the very beginning.

[EWS Watchlist]

Attachment 343610 [details] did not pass style-queue:


ERROR: Tools/flatpak/flatpakutils.py:611:  missing whitespace around operator  [pep8/E225] [5]
ERROR: Tools/flatpak/flatpakutils.py:611:  [WebkitFlatpak.clean_args] Undefined variable 'assumeyes'  [pylint/E0602] [5]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thibault Saunier]

Created attachment 343611 [details]
Patch

Style fix.

[Philippe Normand]

Comment on attachment 343611 [details]
Patch

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

> Tools/flatpak/org.webkit.GTK.yaml:121
> +      branch: mediastream-minus-enumerateDevices

Hum?

> Tools/flatpak/org.webkit.WPE.yaml:44
> +      branch: mediastream-minus-enumerateDevices

Ditto :)

[Carlos Alberto Lopez Perez]

Comment on attachment 343611 [details]
Patch

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

>> Tools/flatpak/org.webkit.GTK.yaml:121
>> +- name: org.webkit.GTK
>> +  sources:
>> +    - type: git
>> +      url: https://github.com/WebKit/WebKit.git
>> +      branch: mediastream-minus-enumerateDevices
> 
> Hum?

Besides from the weird branch name....
Why is needed to specify an URL to the webkit repo here? (AND BTW ... github is a mirror, but not the official WK git repo).

>> Tools/flatpak/org.webkit.WPE.yaml:44
>> +    - type: git
>> +      url: https://github.com/WebKit/WebKit.git
>> +      branch: mediastream-minus-enumerateDevices
> 
> Ditto :)

same

[Thibault Saunier]

Created attachment 343710 [details]
Patch.

Removed "source" for WebKit itself in the manifest.

[Carlos Alberto Lopez Perez]

Comment on attachment 343710 [details]
Patch.

I'm testing to run tests with this, and all crypto tests are super slow. That is because this misses to add a custom libgcrypt with the patch "libgcrypt-use-only-dev-urandom-for-testing.patch" like JHBuild does (for both GTK and WPE)

[Thibault Saunier]

(In reply to Carlos Alberto Lopez Perez from comment #39)
> Comment on attachment 343710 [details]
> Patch.
> 
> I'm testing to run tests with this, and all crypto tests are super slow.
> That is because this misses to add a custom libgcrypt with the patch
> "libgcrypt-use-only-dev-urandom-for-testing.patch" like JHBuild does (for
> both GTK and WPE)


OK, will add that one too. It would be nice if someone could list the patches that should be added, I added a few that I have been told/thought were useful, but I don't really know tbh.

[Carlos Alberto Lopez Perez]

(In reply to Thibault Saunier from comment #40)
> (In reply to Carlos Alberto Lopez Perez from comment #39)
> > Comment on attachment 343710 [details]
> > Patch.
> > 
> > I'm testing to run tests with this, and all crypto tests are super slow.
> > That is because this misses to add a custom libgcrypt with the patch
> > "libgcrypt-use-only-dev-urandom-for-testing.patch" like JHBuild does (for
> > both GTK and WPE)
> 
> 
> OK, will add that one too. It would be nice if someone could list the
> patches that should be added, I added a few that I have been told/thought
> were useful, but I don't really know tbh.

Well.. the rule of thumb is to add every patch that is not already merged upstream. There is always a reason for all those patches. In case of doubt checking the git log will help to understand what is the reason.

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #41)
> Well.. the rule of thumb is to add every patch that is not already merged
> upstream. There is always a reason for all those patches. In case of doubt
> checking the git log will help to understand what is the reason.

Agreed.

The vast majority of our patches should have been committed upstream, and should be already present in the GNOME runtime, but we should check to make sure we're not accidentally dropping something that's not.

[Thibault Saunier]

Created attachment 343861 [details]
Patch.

I went over the various patches and tried to bring them all in, hoping I didn't make any mistake.

[EWS Watchlist]

Comment on attachment 343861 [details]
Patch.

Attachment 343861 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8376789

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 343885 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Carlos Alberto Lopez Perez]

Comment on attachment 343861 [details]
Patch.

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

> Tools/flatpak/org.webkit.WebKit.yaml:187
> +  - name: libgcrypt # Speedup libgrcypt
> +    sources:
> +      - type: git
> +        url: https://dev.gnupg.org/source/libgcrypt.git
> +        branch: libgcrypt-1.7.6
> +      - type: patch
> +        path: ../gtk/patches/libgcrypt-use-only-dev-urandom-for-testing.patch

It doesn't build:

configure: error: libgpg-error is needed.
                See ftp://ftp.gnupg.org/gcrypt/libgpg-error/ .
Error: module libgcrypt: Child process exited with code 1
Traceback (most recent call last):
  File "Tools/Scripts/update-webkitgtk-flatpak", line 28, in <module>
    WebkitFlatpak.load_from_args(["--gtk", "--update"] + sys.argv[1:]).run()
  File "./Tools/flatpak/flatpakutils.py", line 710, in run
    return self.setup_dev_env()
  File "./Tools/flatpak/flatpakutils.py", line 737, in setup_dev_env
    subprocess.check_call(builder_args)
  File "/usr/lib/python2.7/subprocess.py", line 186, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['flatpak-builder', '--disable-rofiles-fuse', '--state-dir', '/home/clopez/webkit/webkit/WebKitBuild/FlatpakCache', '--ccache', '/home/clopez/webkit/webkit/WebKitBuild/GTK/FlatpakTreeRelease', '--force-clean', '/home/clopez/webkit/webkit/WebKitBuild/FlatpakCache/org.webkit.GTK-generated.json', '--build-only', '--stop-at=org.webkit.GTK']' returned non-zero exit status 1

[Thibault Saunier]

Created attachment 343911 [details]
Patch.

(In reply to Carlos Alberto Lopez Perez from comment #46)
> Comment on attachment 343861 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343861&action=review
> 
> > Tools/flatpak/org.webkit.WebKit.yaml:187
> > +  - name: libgcrypt # Speedup libgrcypt
> > +    sources:
> > +      - type: git
> > +        url: https://dev.gnupg.org/source/libgcrypt.git
> > +        branch: libgcrypt-1.7.6
> > +      - type: patch
> > +        path: ../gtk/patches/libgcrypt-use-only-dev-urandom-for-testing.patch
> 
> It doesn't build:
> 
> configure: error: libgpg-error is needed.
>                 See ftp://ftp.gnupg.org/gcrypt/libgpg-error/ .
> Error: module libgcrypt: Child process exited with code 1
> Traceback (most recent call last):
>   File "Tools/Scripts/update-webkitgtk-flatpak", line 28, in <module>
>     WebkitFlatpak.load_from_args(["--gtk", "--update"] + sys.argv[1:]).run()
>   File "./Tools/flatpak/flatpakutils.py", line 710, in run
>     return self.setup_dev_env()
>   File "./Tools/flatpak/flatpakutils.py", line 737, in setup_dev_env
>     subprocess.check_call(builder_args)
>   File "/usr/lib/python2.7/subprocess.py", line 186, in check_call
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['flatpak-builder',
> '--disable-rofiles-fuse', '--state-dir',
> '/home/clopez/webkit/webkit/WebKitBuild/FlatpakCache', '--ccache',
> '/home/clopez/webkit/webkit/WebKitBuild/GTK/FlatpakTreeRelease',
> '--force-clean',
> '/home/clopez/webkit/webkit/WebKitBuild/FlatpakCache/org.webkit.GTK-
> generated.json', '--build-only', '--stop-at=org.webkit.GTK']' returned
> non-zero exit status 1


Oops, sorryt about that. Fixed.

[Carlos Alberto Lopez Perez]

Comment on attachment 343911 [details]
Patch.

I think this is now good enough to commit. Thanks a lot for the work!

Running layout tests with it is still kind of broken.

I get this:
Regressions: Unexpected text-only failures (929)
Regressions: Unexpected image-only failures (157)
Regressions: Unexpected audio failures (1)
Regressions: Unexpected crashes (2)
Regressions: Unexpected timeouts (395)

The failures are somehow expected due to the different system, but the timeouts have to be investigated and fixed. Also the api tests are broken. I will report a bug about that...

But let's commit this so others can start testing and report/fix bugs as they appear

Thanks for this!

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #48)
> Also the api tests are broken. I will report a bug about that...
> 

Reported in bug 187184

[WebKit Commit Bot]

Comment on attachment 343911 [details]
Patch.

Clearing flags on attachment: 343911

Committed r233362: <https://trac.webkit.org/changeset/233362>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41642656>

[Carlos Alberto Lopez Perez]

Committed r233364: <https://trac.webkit.org/changeset/233364>

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #53)
> Committed r233364: <https://trac.webkit.org/changeset/233364>

I fixed a bug here that broke the bots (flatpak is not installed on the bots) and reported bug 187187 to improve this.

[Michael Catanzaro]

I tried running layout tests without much luck. WebKitTestRunner is crashing. There's no obvious way to get a coredump for the crash.

$ run-webkit-tests --gtk http/tests/mime/asan-crash-downloading-file.html
Running in sandbox: "flatpak" "build" "--die-with-parent" "--bind-mount=/run/host//tmp=/tmp" "--bind-mount=/app/webkit=/home/mcatanzaro/Projects/WebKit" "--bind-mount=/app/webkit/WebKitBuild/Release=/home/mcatanzaro/Projects/WebKit/WebKitBuild/GTK/Release" "--env=LANG=en_US.UTF-8" "--env=WEBKIT_TOP_LEVEL=/app/" "--env=WAYLAND_DISPLAY=wayland-0" "--env=TEST_RUNNER_INJECTED_BUNDLE_FILENAME=/app/webkit/lib/libTestRunnerInjectedBundle.so" "--env=DISPLAY=:0" "--share=ipc" "--socket=wayland" "--share=network" "--socket=pulseaudio" "--system-talk-name=org.freedesktop.GeoClue2" "--filesystem=host" "--socket=system-bus" "--talk-name=org.freedesktop.Flatpak" "--env=GST_PRESET_PATH=/app/share/gstreamer-1.0/presets/" "/home/mcatanzaro/Projects/WebKit/WebKitBuild/GTK/FlatpakTreeRelease" "/app/webkit/Tools/Scripts/run-webkit-tests" "--gtk" "http/tests/mime/asan-crash-downloading-file.html"

Using port 'gtk-wk2'
Test configuration: <, x86, debug>
Placing test results in /app/webkit/WebKitBuild/Debug/layout-test-results
Baseline search path: platform/gtk -> platform/wk2 -> generic
Using Debug build
Pixel tests disabled
Regular timeout: 30000, slow test timeout: 150000
Command line: /app/webkit/WebKitBuild/Debug/bin/WebKitTestRunner -

Found 1 test; running 1, skipping 0.
                        
Running 1 test

Running 1 WebKitTestRunner.     

[1/1] http/tests/mime/asan-crash-downloading-file.html failed unexpectedly (WebKitTestRunner crashed [pid=39])
                        
0 tests ran as expected, 1 didn't:


ScriptError raised: Failed to run "['Tools/Scripts/run-minibrowser', '--debug', '--gtk', u'file:///app/webkit/WebKitBuild/Debug/layout-test-results/results.html']" exit_code: 127 cwd: /app/webkit
Traceback (most recent call last):
  File "/app/webkit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 85, in main
    run_details = run(port, options, args, stderr)
  File "/app/webkit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 447, in run
    run_details = manager.run(args)
  File "/app/webkit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 268, in run
    return self._end_test_run(start_time, end_time, initial_results, retry_results, enabled_pixel_tests_in_retry)
  File "/app/webkit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 323, in _end_test_run
    self._port.show_results_html_file(results_path)
  File "/app/webkit/Tools/Scripts/webkitpy/port/gtk.py", line 229, in show_results_html_file
    self._run_script("run-minibrowser", [path.abspath_to_uri(self.host.platform, results_filename)])
  File "/app/webkit/Tools/Scripts/webkitpy/port/base.py", line 1488, in _run_script
    output = self._executive.run_command(run_script_command, cwd=self.webkit_base(), decode_output=decode_output, env=env)
  File "/app/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 412, in run_command
    (error_handler or self.default_error_handler)(script_error)
  File "/app/webkit/Tools/Scripts/webkitpy/common/system/abstractexecutive.py", line 97, in default_error_handler
    raise error
ScriptError: Failed to run "['Tools/Scripts/run-minibrowser', '--debug', '--gtk', u'file:///app/webkit/WebKitBuild/Debug/layout-test-results/results.html']" exit_code: 127 cwd: /app/webkit
Command '['flatpak', 'build', '--die-with-parent', '--bind-mount=/run/host//tmp=/tmp', '--bind-mount=/app/webkit=/home/mcatanzaro/Projects/WebKit', '--bind-mount=/app/webkit/WebKitBuild/Release=/home/mcatanzaro/Projects/WebKit/WebKitBuild/GTK/Release', '--env=LANG=en_US.UTF-8', '--env=WEBKIT_TOP_LEVEL=/app/', '--env=WAYLAND_DISPLAY=wayland-0', '--env=TEST_RUNNER_INJECTED_BUNDLE_FILENAME=/app/webkit/lib/libTestRunnerInjectedBundle.so', '--env=DISPLAY=:0', '--share=ipc', '--socket=wayland', '--share=network', '--socket=pulseaudio', '--system-talk-name=org.freedesktop.GeoClue2', '--filesystem=host', '--socket=system-bus', '--talk-name=org.freedesktop.Flatpak', '--env=GST_PRESET_PATH=/app/share/gstreamer-1.0/presets/', '/home/mcatanzaro/Projects/WebKit/WebKitBuild/GTK/FlatpakTreeRelease', 'sh', '/run/host//tmp/tmpjjJ3aK']' returned non-zero exit status 254

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #55)
> I tried running layout tests without much luck. WebKitTestRunner is
> crashing. There's no obvious way to get a coredump for the crash.

Reported bug #187218.