Bug 213431 – Migrate run-minibrowser to webkitpy

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Philippe Normand]

The perl script pulls into webkitdirs.pm which requires the "version" module. On Linux platforms this is not installed by default. This could be a good opportunity to migrate this script to webkitpy.

The perl version would be renamed to old-run-minibrowser and would be used by the mac port, until its specific command-line options are ported to the python script.

[Philippe Normand]

Created attachment 402387 [details]
Patch

[Philippe Normand]

Created attachment 402388 [details]
Patch

[Philippe Normand]

Created attachment 402394 [details]
Patch

[Philippe Normand]

Comment on attachment 402394 [details]
Patch

Needs to pass-through options to wpe/gtk browsers.

[Philippe Normand]

Created attachment 402416 [details]
Patch

[Philippe Normand]

Created attachment 402417 [details]
Patch

[Sam Weinig]

Comment on attachment 402417 [details]
Patch

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

> Tools/ChangeLog:10
> +        The perl version was renamed to old-run-minibrowser and remains used by the Mac ports.
> +        run-minibrowser is now a Python script hooking into webkitpy and the Port infrastructure.
> +        Both WPE and GTK ports will now use the pure python implementation.

What are the benefits of the python based version? Is the idea that those of us using the Mac port should now be calling old-run-minibrowser? Or is it just something run-minibrowser now calls when running on the Mac?

[Philippe Normand]

Comment on attachment 402417 [details]
Patch

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

>> Tools/ChangeLog:10
>> +        Both WPE and GTK ports will now use the pure python implementation.
> 
> What are the benefits of the python based version? Is the idea that those of us using the Mac port should now be calling old-run-minibrowser? Or is it just something run-minibrowser now calls when running on the Mac?

The advantage of the python based version is that the barrier to entry is lower. By default in most linux distros, the Perl version module isn't installed, which makes webkitdirs and the scripts that depend on it useless. The Python version runs out of the box without installing additional packages.

On Mac run-minibrowser will indeed call old-run-minibrowser. So this patch keeps backward compatibility.

[Jonathan Bedard]

Comment on attachment 402417 [details]
Patch

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

>>> Tools/ChangeLog:10
>>> +        Both WPE and GTK ports will now use the pure python implementation.
>> 
>> What are the benefits of the python based version? Is the idea that those of us using the Mac port should now be calling old-run-minibrowser? Or is it just something run-minibrowser now calls when running on the Mac?
> 
> The advantage of the python based version is that the barrier to entry is lower. By default in most linux distros, the Perl version module isn't installed, which makes webkitdirs and the scripts that depend on it useless. The Python version runs out of the box without installing additional packages.
> 
> On Mac run-minibrowser will indeed call old-run-minibrowser. So this patch keeps backward compatibility.

I'm actually in favor of scrapping the perl version completely, even for Apple ports. I understand if you need someone that has a Mac handy to complete that, though. Simulators are not obvious to interact with.

> Tools/Scripts/run-minibrowser:-1
> -#!/usr/bin/env perl

My general comment here is that run-minibrowser is basically a special case of run-webkit-app. Seems that Tools/Scripts/webkitpy/minibrowser/run_minibrowser.py should be called Tools/Scripts/webkitpy/minibrowser/run_webkit_app.py and the top-level script basically sets the path of the binary to be run to MiniBrowser.

I know this would make sense on Apple ports, where any app using WebKit could be run with your built WebKit by setting DYLD_FRAMEWORK and DYLD_LIBRARY paths. I don't have much experience with other ports, so it may not make sense.

> Tools/Scripts/run-minibrowser:23
> +from webkitpy.common import multiprocessing_bootstrap

We shouldn't need this

> Tools/Scripts/run-minibrowser:31
> +multiprocessing_bootstrap.run('webkitpy', 'minibrowser', 'run_minibrowser.py')

We shouldn't need this. The multiprocessing_bootstrap stuff is 1) a bad way to do multiprocessing (not your fault, I'm looking to replace it) but 2) only needed if we intend to have worker processes.

> Tools/Scripts/webkitpy/minibrowser/run_minibrowser.py:33
> +    # Convert options to argparse, so that we can use parse_known_args() which is not supported in optparse.

Can we add a FIXME here? Really, we should be using argparse options generally.

> Tools/Scripts/webkitpy/port/base.py:1384
> +        return subprocess.call(run_script_command, cwd=self.webkit_base())

Let's add a FIXME here. Apple ports should also use native Python.

[Philippe Normand]

Comment on attachment 402417 [details]
Patch

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

>> Tools/Scripts/run-minibrowser:-1
>> -#!/usr/bin/env perl
> 
> My general comment here is that run-minibrowser is basically a special case of run-webkit-app. Seems that Tools/Scripts/webkitpy/minibrowser/run_minibrowser.py should be called Tools/Scripts/webkitpy/minibrowser/run_webkit_app.py and the top-level script basically sets the path of the binary to be run to MiniBrowser.
> 
> I know this would make sense on Apple ports, where any app using WebKit could be run with your built WebKit by setting DYLD_FRAMEWORK and DYLD_LIBRARY paths. I don't have much experience with other ports, so it may not make sense.

Well, on Linux with the Flatpak SDK we actually do something similar deep under the hood. The `flatpakutils.run_in_sandbox_if_available()` below runs MiniBrowser in a dedicated sandbox with LD_LIBRARY_PATH and similar vars set.

I don't mind renaming to Tools/Scripts/webkitpy/minibrowser/run_webkit_app.py :)

>> Tools/Scripts/run-minibrowser:31
>> +multiprocessing_bootstrap.run('webkitpy', 'minibrowser', 'run_minibrowser.py')
> 
> We shouldn't need this. The multiprocessing_bootstrap stuff is 1) a bad way to do multiprocessing (not your fault, I'm looking to replace it) but 2) only needed if we intend to have worker processes.

OK. I started off from run_webkit_tests.py and clearly didn't clean enough here :)

>> Tools/Scripts/webkitpy/minibrowser/run_minibrowser.py:33
>> +    # Convert options to argparse, so that we can use parse_known_args() which is not supported in optparse.
> 
> Can we add a FIXME here? Really, we should be using argparse options generally.

I will link to https://bugs.webkit.org/show_bug.cgi?id=213463

>> Tools/Scripts/webkitpy/port/base.py:1384
>> +        return subprocess.call(run_script_command, cwd=self.webkit_base())
> 
> Let's add a FIXME here. Apple ports should also use native Python.

I will link to https://bugs.webkit.org/show_bug.cgi?id=213432

[Philippe Normand]

Created attachment 402472 [details]
Patch

[Jonathan Bedard]

Comment on attachment 402472 [details]
Patch

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

> Tools/Scripts/webkitpy/minibrowser/run_webkit_app.py:30
> +    option_parser = argparse.ArgumentParser(prog="run-minibrowser", usage="%(prog)s [options] [url]", add_help=False)

Let's add a FIXME here.

Ultimately, I think that this function should accept an optional name of the binary being run, which would actually change what we feed to argparse. That way, we can replace some other scripts (like run-safari) with Python versions that share code.

> Tools/Scripts/webkitpy/port/wpe.py:143
> +        command = [miniBrowser, ]

Shouldn't need the trailing ,

[Philippe Normand]

Comment on attachment 402472 [details]
Patch

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

>> Tools/Scripts/webkitpy/minibrowser/run_webkit_app.py:30
>> +    option_parser = argparse.ArgumentParser(prog="run-minibrowser", usage="%(prog)s [options] [url]", add_help=False)
> 
> Let's add a FIXME here.
> 
> Ultimately, I think that this function should accept an optional name of the binary being run, which would actually change what we feed to argparse. That way, we can replace some other scripts (like run-safari) with Python versions that share code.

Actually I think the prog arg can be removed. By default the parser will then use sys.argv[0], which should fit for run-minibrowser and others :)

[Philippe Normand]

Created attachment 402959 [details]
Patch

[EWS]

Committed r263625: <https://trac.webkit.org/changeset/263625>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402959 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64845044>

[Radar WebKit Bug Importer]

<rdar://problem/64845045>

[John Wilander]

This seems to have broken run-minibrowser, at least on shipping macOS.

AttributeError raised: 'list' object has no attribute 'startswith'
Traceback (most recent call last):
  File "/Users/***/WebKit/Tools/Scripts/webkitpy/minibrowser/run_webkit_app.py", line 59, in main
    return port.run_minibrowser(args)
  File "/Users/***/WebKit/Tools/Scripts/webkitpy/port/base.py", line 1372, in run_minibrowser
    return self._run_script(["old-run-minibrowser", ] + args)
  File "/Users/***/WebKit/Tools/Scripts/webkitpy/port/base.py", line 1486, in _run_script
    run_script_command = [self.path_to_script(script_name)]
  File "/Users/***/WebKit/Tools/Scripts/webkitpy/port/base.py", line 717, in path_to_script
    return self._webkit_finder.path_to_script(script_name)
  File "/Users/***/WebKit/Tools/Scripts/webkitpy/common/webkit_finder.py", line 64, in path_to_script
    return self._filesystem.join("Tools", "Scripts", script_name)
  File "/Users/***/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py", line 162, in join
    return os.path.join(*comps)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/posixpath.py", line 68, in join
    if b.startswith('/'):
AttributeError: 'list' object has no attribute 'startswith'

[John Wilander]

Pinged Phillipe over email.

[John Wilander]

(In reply to John Wilander from comment #19)
> Pinged Phillipe over email.

Philippe.

[Philippe Normand]

Created attachment 403306 [details]
Fix attempt

Hi John, thanks for the ping and my apologies for the break. Can you check if this patch fixes the issue?

[Philippe Normand]

I confirm it works. Sending patch to https://bugs.webkit.org/show_bug.cgi?id=213876