Summary: | Migrate run-minibrowser to webkitpy | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||||||||
Component: | Tools / Tests | Assignee: | Philippe Normand <pnormand> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | ews-watchlist, glenn, jbedard, sam, simon.fraser, webkit-bug-importer, wilander | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Philippe Normand
2020-06-20 09:17:05 PDT
Created attachment 402387 [details]
Patch
Created attachment 402388 [details]
Patch
Created attachment 402394 [details]
Patch
Comment on attachment 402394 [details]
Patch
Needs to pass-through options to wpe/gtk browsers.
Created attachment 402416 [details]
Patch
Created attachment 402417 [details]
Patch
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 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 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 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 Created attachment 402472 [details]
Patch
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 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 :) Created attachment 402959 [details]
Patch
Committed r263625: <https://trac.webkit.org/changeset/263625> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402959 [details]. 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' Pinged Phillipe over email. (In reply to John Wilander from comment #19) > Pinged Phillipe over email. Philippe. 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?
I confirm it works. Sending patch to https://bugs.webkit.org/show_bug.cgi?id=213876 |