Bug 230319 – rwt: Switch pywebsocket from the old tools/pywebsocket to the new tools/third_party/pywebsocket3

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

[Fujii Hironori]

rwt: Switch pywebsocket from the old tools/pywebsocket to the new tools/third_party/pywebsocket3

The upstream wpt removed tools/pywebsocket.
https://github.com/web-platform-tests/wpt/commit/da56e6cf601033599b04d36fbd0e1d092032b241

WIP patch is attachment#422172 [details].

[Fujii Hironori]

Created attachment 438284 [details]
WIP patch

[Sam Sneddon [:gsnedders]]

From Fujii on Slack:

> We have to update all *.py scripts in LayoutTests/http/tests/websocket/tests/hybi directory for python 3 and pywebsocket3.

[Fujii Hironori]

Created attachment 438899 [details]
WIP patch

[Fujii Hironori]

Created attachment 438911 [details]
WIP patch

[Fujii Hironori]

Created attachment 438923 [details]
WIP patch

[Radar WebKit Bug Importer]

<rdar://problem/83415801>

[Jonathan Bedard]

*** Bug 206546 has been marked as a duplicate of this bug. ***

[Jonathan Bedard]

Created attachment 452460 [details]
Patch

[Aakash Jain]

Cancelled https://ews-build.webkit.org/#/builders/70/builds/648 to speed up mac-wk2 ews queue.

[Don Olmstead]

Comment on attachment 452460 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:120
> -        if sys.version_info > (3, 0):
> -            python_interp = 'python2'
> +        if sys.version_info < (3, 0):
> +            python_interp = 'python3'

With this patch aren't we at the point where we should only be running Python 3?

[Chris Dumez]

(In reply to Don Olmstead from comment #10)
> Comment on attachment 452460 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452460&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:120
> > -        if sys.version_info > (3, 0):
> > -            python_interp = 'python2'
> > +        if sys.version_info < (3, 0):
> > +            python_interp = 'python3'
> 
> With this patch aren't we at the point where we should only be running
> Python 3?

Right, WPT has been requiring python3 for a while now.

[Don Olmstead]

Comment on attachment 452460 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:120
>>> +            python_interp = 'python3'
>> 
>> With this patch aren't we at the point where we should only be running Python 3?
> 
> Right, WPT has been requiring python3 for a while now.

For WinCairo and PlayStation builds I've been waiting to remove Python 2 from our Docker containers so the question was more once this lands can we run a buildbot doing builds and tests that only has Python 3?

And if that's the case maybe we should be more vocal when we find someone still running a particular script in Python 2.

[Jonathan Bedard]

(In reply to Don Olmstead from comment #12)
> Comment on attachment 452460 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452460&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:120
> >>> +            python_interp = 'python3'
> >> 
> >> With this patch aren't we at the point where we should only be running Python 3?
> > 
> > Right, WPT has been requiring python3 for a while now.
> 
> For WinCairo and PlayStation builds I've been waiting to remove Python 2
> from our Docker containers so the question was more once this lands can we
> run a buildbot doing builds and tests that only has Python 3?
> 
> And if that's the case maybe we should be more vocal when we find someone
> still running a particular script in Python 2.

Yes, and Apple won't be far behind. Monterey 12.3 has seeded and doesn't have Python 2. So everything will be Python 3 in short order. Plan is to land https://github.com/WebKit/WebKit/pull/160 Monday morning, which will invoke `run-webkit-tests` with Python 3 in post-commit infrastructure.

[Jonathan Bedard]

Created attachment 452786 [details]
Patch

[Jonathan Bedard]

Created attachment 452818 [details]
Patch

[Jonathan Bedard]

Created attachment 452894 [details]
Patch

[Fujii Hironori]

Comment on attachment 452894 [details]
Patch

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

> LayoutTests/ChangeLog:1
> [phone]  Jonathan Bedard  <[e-mail]>

gmail.com

[Jonathan Bedard]

Created attachment 452913 [details]
Patch

[Jonathan Bedard]

Created attachment 452978 [details]
Patch

[Jonathan Bedard]

Created attachment 452984 [details]
Patch

[Jonathan Bedard]

Created attachment 453024 [details]
Patch

[Jonathan Bedard]

Created attachment 453061 [details]
Patch

[Jonathan Bedard]

Created attachment 453117 [details]
Patch

[Jonathan Bedard]

Created attachment 453119 [details]
Patch

[Jonathan Bedard]

Created attachment 453141 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #25)
> Created attachment 453141 [details]
> Patch

I want to make sure EWS is green before landing this change. I also want to make sure our Build Sheriffs are prepared to handle any fallout this may cause on queues we don't have EWS for.

[Roy Reapor]

Comment on attachment 453141 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:119
> +            python_interp = 'python3'

Can we use sys.executable here or the full path?

> LayoutTests/http/tests/websocket/tests/hybi/broken-utf8_wsh.py:10
> +    payload = b'This text should be ignored. \xff'  # '\xff' will never appear in UTF-8 encoded data.

extra space?

[Jonathan Bedard]

Comment on attachment 453141 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:119
>> +            python_interp = 'python3'
> 
> Can we use sys.executable here or the full path?

Short answer is "no". In general, we do use `sys.executable` (as you see in the above line), but we also need to always run this particular server with Python 3. We could try and determine the full path from sys.executable, but that assumes that Python 2 and 3 are installed in the same location, which isn't generally true. Ultimately, we should almost always be using sys.executable in practice since we are changing every run-webkit-tests invocation to Python 3 in https://bugs.webkit.org/show_bug.cgi?id=226658.

>> LayoutTests/http/tests/websocket/tests/hybi/broken-utf8_wsh.py:10
>> +    payload = b'This text should be ignored. \xff'  # '\xff' will never appear in UTF-8 encoded data.
> 
> extra space?

Pep8 asks for two spaces before an inline comment: https://www.python.org/dev/peps/pep-0008/#inline-comments

[Jonathan Bedard]

Created attachment 453363 [details]
Patch for landing

[EWS]

Committed r290580 (247858@main): <https://commits.webkit.org/247858@main>

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