Bug 211249

Created attachment 398089 [details]
patch

Comment on attachment 398089 [details]
patch

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

Just a heads up: webkitpy EWS will run both Python 2 and Python 3 tests, so as long as the Python code you are changing is tested, you can trust EWS.

> Tools/Scripts/run-benchmark:-1
> -#!/usr/bin/env python

We aren't ready to take the dive of only supporting Python 3 yet, let's keep these she-bangs 'python'.

We still have machines without Python 3, so we're holding off transitioning existing scripts.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder.py:1
> +#!/usr/bin/env python3

Ditto.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder.py:6
> +import urllib.request

Let's make sure we have Python 2 compatible imports as well...if you grep for urllib imports in webkitpy, you'll see us using two different import calls, one for Python 2 and one for Python 3.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_results_unittest.py:1
> +#!/usr/bin/env python3

Ditto.

Created attachment 398201 [details]
patch

(In reply to Jonathan Bedard from comment #2)
> Comment on attachment 398089 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398089&action=review
> 
> Just a heads up: webkitpy EWS will run both Python 2 and Python 3 tests, so
> as long as the Python code you are changing is tested, you can trust EWS.
> 
> > Tools/Scripts/run-benchmark:-1
> > -#!/usr/bin/env python
> 
> We aren't ready to take the dive of only supporting Python 3 yet, let's keep
> these she-bangs 'python'.
> 
> We still have machines without Python 3, so we're holding off transitioning
> existing scripts.
> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder.py:1
> > +#!/usr/bin/env python3
> 
> Ditto.
> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder.py:6
> > +import urllib.request
> 
> Let's make sure we have Python 2 compatible imports as well...if you grep
> for urllib imports in webkitpy, you'll see us using two different import
> calls, one for Python 2 and one for Python 3.
> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_results_unittest.py:1
> > +#!/usr/bin/env python3
> 
> Ditto.

Thanks for your kindly review. I just uploaded a new version compatible with both python versions.

(In reply to Jonathan Bedard from comment #2)
> Comment on attachment 398089 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398089&action=review
> 
> Just a heads up: webkitpy EWS will run both Python 2 and Python 3 tests, so
> as long as the Python code you are changing is tested, you can trust EWS.
> 
> > Tools/Scripts/run-benchmark:-1
> > -#!/usr/bin/env python
> 
> We aren't ready to take the dive of only supporting Python 3 yet, let's keep
> these she-bangs 'python'.
> 
> We still have machines without Python 3, so we're holding off transitioning
> existing scripts.
> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder.py:1
> > +#!/usr/bin/env python3
> 
> Ditto.
> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder.py:6
> > +import urllib.request
> 
> Let's make sure we have Python 2 compatible imports as well...if you grep
> for urllib imports in webkitpy, you'll see us using two different import
> calls, one for Python 2 and one for Python 3.
> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_results_unittest.py:1
> > +#!/usr/bin/env python3
> 
> Ditto.

Could you take a look again?

Comment on attachment 398201 [details]
patch

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

Before landing, I need to do some brief testing on Apple's side since there is some mission-critical Apple Internal infrastructure that uses this code.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:-145
> -        if arg_type == types.ListType and len(a) and (type(a[0]) == types.StringType or type(a[0]) == types.UnicodeType):

Seems like we were treating unicode as a string type in Python 2 also

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:79
> +        print(("\t%s" % plan))

Is this change actually needed? It would surprise me if it is.

> Tools/Scripts/webkitpy/style/checker.py:723
> +            python3_paths = ['Tools/resultsdbpy', 'Tools/Scripts/webkitpy/benchmark_runner']

We shouldn't add it here, since this file is also a Python 2 path. resultsdbpy is somewhat unique in that it is exclusively Python 3 (since it doesn't need to be run locally) and so we need to run it through a different style checker since it contains syntax illegal in Python 2.

(In reply to Jonathan Bedard from comment #6)
> Comment on attachment 398201 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398201&action=review
> 
> Before landing, I need to do some brief testing on Apple's side since there
> is some mission-critical Apple Internal infrastructure that uses this code.
> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:-145
> > -        if arg_type == types.ListType and len(a) and (type(a[0]) == types.StringType or type(a[0]) == types.UnicodeType):
> 
> Seems like we were treating unicode as a string type in Python 2 also
> 
> > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:79
> > +        print(("\t%s" % plan))
> 
> Is this change actually needed? It would surprise me if it is.

not at all. Sorry don't check it before.

> 
> > Tools/Scripts/webkitpy/style/checker.py:723
> > +            python3_paths = ['Tools/resultsdbpy', 'Tools/Scripts/webkitpy/benchmark_runner']
> 
> We shouldn't add it here, since this file is also a Python 2 path.
> resultsdbpy is somewhat unique in that it is exclusively Python 3 (since it
> doesn't need to be run locally) and so we need to run it through a different
> style checker since it contains syntax illegal in Python 2.
ok.

Created attachment 398506 [details]
patch

(In reply to Jonathan Bedard from comment #6)
> Comment on attachment 398201 [details]
...
> 
> > Tools/Scripts/webkitpy/style/checker.py:723
> > +            python3_paths = ['Tools/resultsdbpy', 'Tools/Scripts/webkitpy/benchmark_runner']
> 
> We shouldn't add it here, since this file is also a Python 2 path.
> resultsdbpy is somewhat unique in that it is exclusively Python 3 (since it
> doesn't need to be run locally) and so we need to run it through a different
> style checker since it contains syntax illegal in Python 2.

Not adding this in the python3_paths, I got this error in the style checker:

  ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder.py:15:  No name 'request' in module 'urllib'  [pylint/E0611] [5]


Who I should I proceed to ignore this error?

(In reply to Pablo Saavedra from comment #9)
> (In reply to Jonathan Bedard from comment #6)
> > Comment on attachment 398201 [details]
> ...
> > 
> > > Tools/Scripts/webkitpy/style/checker.py:723
> > > +            python3_paths = ['Tools/resultsdbpy', 'Tools/Scripts/webkitpy/benchmark_runner']
> > 
> > We shouldn't add it here, since this file is also a Python 2 path.
> > resultsdbpy is somewhat unique in that it is exclusively Python 3 (since it
> > doesn't need to be run locally) and so we need to run it through a different
> > style checker since it contains syntax illegal in Python 2.
> 
> Not adding this in the python3_paths, I got this error in the style checker:
> 
>   ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder.py:15: 
> No name 'request' in module 'urllib'  [pylint/E0611] [5]
> 
> 
> Who I should I proceed to ignore this error?

Just ignore it, I think we have a pretty good case for getting rid of the E0611 because of this. This sort of style error was pretty common when converting webkitpy to Python 3.

Doesn't break anything for python 2, but I tried running jetstream2 with Python 3 and starting the web server didn't work. That's not necessarily a deal-breaker, since I know we have some other issues with web servers and Python 3, but I want to make sure the current patch is compatible with whatever run-benchmark incantation you are using.

(In reply to Jonathan Bedard from comment #11)
> Doesn't break anything for python 2, but I tried running jetstream2 with
> Python 3 and starting the web server didn't work. That's not necessarily a
> deal-breaker, since I know we have some other issues with web servers and
> Python 3, but I want to make sure the current patch is compatible with
> whatever run-benchmark incantation you are using.

It's working in my local Linux environment (for Firefox, Epiphany and Chrome at least). Did you see something relevant in the logs output related the HTTP Server?

It's psutil, we might need to add that to our auto-installers, (probably working for you locally because you have it installed for some other reason).

That should be a different change, though, r+ing.

(In reply to Jonathan Bedard from comment #13)
> It's psutil, we might need to add that to our auto-installers, (probably
> working for you locally because you have it installed for some other reason).
> 
> That should be a different change, though, r+ing.

Ok, great. Created https://bugs.webkit.org/show_bug.cgi?id=211514 to address this.

Committed r261231: <https://trac.webkit.org/changeset/261231>

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

<rdar://problem/62931875>