Bug 213431 - Migrate run-minibrowser to webkitpy
Summary: Migrate run-minibrowser to webkitpy
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: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-20 09:17 PDT by Philippe Normand
Modified: 2020-07-02 01:08 PDT (History)
7 users (show)

See Also:


Attachments
Patch (15.42 KB, patch)
2020-06-20 10:36 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (15.34 KB, patch)
2020-06-20 10:40 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (16.35 KB, patch)
2020-06-20 11:02 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (16.24 KB, patch)
2020-06-21 03:40 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (17.65 KB, patch)
2020-06-21 04:31 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (16.60 KB, patch)
2020-06-22 08:02 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (16.59 KB, patch)
2020-06-27 09:35 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Fix attempt (588 bytes, patch)
2020-07-01 11:58 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2020-06-20 09:17:05 PDT
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.
Comment 1 Philippe Normand 2020-06-20 10:36:10 PDT
Created attachment 402387 [details]
Patch
Comment 2 Philippe Normand 2020-06-20 10:40:44 PDT
Created attachment 402388 [details]
Patch
Comment 3 Philippe Normand 2020-06-20 11:02:06 PDT
Created attachment 402394 [details]
Patch
Comment 4 Philippe Normand 2020-06-20 11:56:00 PDT
Comment on attachment 402394 [details]
Patch

Needs to pass-through options to wpe/gtk browsers.
Comment 5 Philippe Normand 2020-06-21 03:40:37 PDT
Created attachment 402416 [details]
Patch
Comment 6 Philippe Normand 2020-06-21 04:31:52 PDT
Created attachment 402417 [details]
Patch
Comment 7 Sam Weinig 2020-06-21 10:02:12 PDT
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?
Comment 8 Philippe Normand 2020-06-21 10:16:02 PDT
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.
Comment 9 Jonathan Bedard 2020-06-22 06:55:58 PDT
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.
Comment 10 Philippe Normand 2020-06-22 07:52:27 PDT
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
Comment 11 Philippe Normand 2020-06-22 08:02:17 PDT
Created attachment 402472 [details]
Patch
Comment 12 Jonathan Bedard 2020-06-24 06:49:00 PDT
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 ,
Comment 13 Philippe Normand 2020-06-24 08:12:13 PDT
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 :)
Comment 14 Philippe Normand 2020-06-27 09:35:25 PDT
Created attachment 402959 [details]
Patch
Comment 15 EWS 2020-06-27 10:00:53 PDT
Committed r263625: <https://trac.webkit.org/changeset/263625>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402959 [details].
Comment 16 Radar WebKit Bug Importer 2020-06-27 10:01:16 PDT
<rdar://problem/64845044>
Comment 17 Radar WebKit Bug Importer 2020-06-27 10:01:21 PDT
<rdar://problem/64845045>
Comment 18 John Wilander 2020-07-01 11:31:48 PDT
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'
Comment 19 John Wilander 2020-07-01 11:33:22 PDT
Pinged Phillipe over email.
Comment 20 John Wilander 2020-07-01 11:33:44 PDT
(In reply to John Wilander from comment #19)
> Pinged Phillipe over email.

Philippe.
Comment 21 Philippe Normand 2020-07-01 11:58:06 PDT
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?
Comment 22 Philippe Normand 2020-07-02 01:08:13 PDT
I confirm it works. Sending patch to https://bugs.webkit.org/show_bug.cgi?id=213876