Dependency v8 is not Python 3 compatible #24512

cclauss · 2018-11-20T07:14:41Z

The open bug reports on chromium.org:

@bmsdave commit to upstream: #24512 (comment)

Trott · 2018-11-26T00:17:26Z

PR has landed so I assume this can be closed. Comment (or re-open if GitHub allows) if I'm wrong about that! Thanks!!!

refack · 2018-11-26T01:17:48Z

PR has landed so I assume this can be closed.

The files in deps/v8 and tools/inspector_protocol + jinja2 + markupsafe where excluded from #24486. So this is still an issue.

refack · 2018-11-26T01:20:28Z

/CC @nodejs/v8 @nodejs/v8-inspector

FTR on 1.1.2020 Python 2 hits it's EOL https://pythonclock.org/

bmsdave · 2018-11-28T12:55:29Z

If you do not mind.
I would like to try to take up this task.
I'll try it this weekend. If I have any problems, I'll write.

hashseed · 2018-11-28T13:10:59Z

I definitely don't mind any contributions here. Do you have a list of scripts required by Node.js that need to be migrated?

cclauss · 2018-11-28T14:34:50Z

There are now less that 400 days until the end of life of Python 2 (aka legacy Python). The v8 repo is just 1.4% Python but that is all legacy Python and at least 76 files need to be modified just to fix the print statement which is merely the start of a Python 3 port. v8 is a venerable codebase and I often hear that it has been a godsend to the JavaScript community but its Python code needs to be modernized, removed, or replaced with JavaScript, Go, etc. in the days remain until Python 2 end of life.

I would recommend that someone uses http://python-future.org to do the following:

Run v8 tests on Python 2.7 and make sure they pass
Drop commitment to support for all Python versions < 2.7 (they are EOL for 5+ years anyway)
Run futurize -f libfuturize.fixes.fix_print_with_import -w . at the root of the v8 repo (print() function and new style imports for Python 3 v8/v8#26)
Run v8 tests on Python 2.7 and make sure they pass
Run futurize --stage1 -w . at the root of the v8 repo
Run v8 tests on Python 2.7 and make sure they pass
Run v8 tests on Python 3.7 in allow_failures mode and fix as you go...

bmsdave · 2018-11-28T15:15:20Z

Do you have a list of scripts required by Node.js that need to be migrated?

@hashseed I don't have that list. I was planning on finding it through the codebase. But if you provide it, it will be very cool.

@cclauss thank you very much for your clear recommendations. I will try to do it and provide for consideration.

alexkozy · 2018-11-28T16:12:19Z

@bmsdave any changes to inspector_protocol should be upstreamed to inspector_protocol repo first. I can help you with it as soon as your are ready.

cclauss · 2018-12-12T22:15:48Z

@ak239 @bmsdave @hashseed Some low hanging fruit in the upstream inspector_protocol repo.

flake8 testing of https://chromium.googlesource.com/deps/inspector_protocol on Python 3.7.1

./check_protocol_compatibility.py:478:46: E999 SyntaxError: invalid syntax
            print "  Public changes since %s:" % version
                                             ^
./code_generator.py:43:18: F821 undefined name 'xrange'
        for i in xrange(len(keys)):
                 ^
./pdl.py:162:51: E999 SyntaxError: invalid syntax
        print 'Error in %s:%s, illegal token: \t%s' % (file_name, i, line)
                                                  ^
2     E999 SyntaxError: invalid syntax
1     F821 undefined name 'xrange'
3

cclauss · 2018-12-14T04:13:08Z

The bug reports (and proposed fixes) on chromium.org:

Please star these issues and fixes.

cclauss · 2019-01-19T06:15:31Z

Any progress on this issues?

bmsdave · 2019-01-21T08:56:44Z

I think we are now waiting for the adoption of changes from Dmitry Gozman:
https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1358520
https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1387524

Is there anything else you need to do before Dmitry accepts the changes?

cclauss · 2019-01-21T09:13:23Z

There is nothing that I need to do. On January 18th, Dmitry Gozman passed the ball on 1358520 to Johannes Henkel who gave a positive review.

bmsdave · 2019-01-21T09:57:13Z

I'm sorry, I misspelled it.
Is there anything else I need to do before Dmitry accepts the changes? :)

As I understand it, I can try to build the entire node.js + v8 + inspector_protocol chain locally and test the functionality of the tools in python2.7 + python3 with specified edits?

cclauss · 2019-01-21T10:09:12Z

Let's get inspector_protocol to a good state first because v8 is a much bigger problem and given v8/v8#26, I doubt that v8 will be successfully ported to Py3 in the days remaining before Py2 end of life.

hayd · 2019-01-21T12:37:11Z

@cclauss what do you mean "given v8/v8#26"? Did you try going through the chromium review instead of GitHub as suggested?

That said... it seems there's little/no buy-in from Google/v8 to push py3 changes through, and they would need to be pushed through...

cclauss · 2019-01-21T12:40:08Z

I meant the latter (no buy-in), not the former (I did not re-try).

hayd · 2019-01-21T13:13:44Z

@cclauss please consider resubmitting/retrying (not on GitHub). It seems like this discussion has the potential of being much more fruitful on the v8 mailing list https://v8.dev/docs/contribute

cclauss · 2019-01-28T18:37:29Z

Forward progress on upstream inspector_protocol.
https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1358520 has been merged.

cclauss · 2019-10-02T20:27:06Z

We are backsliding: v8/v8#29 (comment)

12101111 · 2019-10-13T07:17:51Z

Will those changes be backported to v10 LTS?

bmsdave · 2019-10-17T09:30:08Z

I apologize for the long absence. I'm ready to continue.
I send PR from @AgentJ08 to https://chromium-review.googlesource.com/c/v8/v8/+/1864942

bmsdave · 2019-10-18T13:50:26Z

It looks like all the requirements are done:
https://travis-ci.com/bmsdave/v8/builds/132539826

hayd · 2019-10-18T16:36:08Z

Passing flake is not passing tests. Is this part of v8's CI? Premature to close IMO.

cclauss · 2019-10-18T18:13:03Z

@hayd Do you have a set of test cases that fail?

cclauss · 2019-10-27T12:19:43Z

Reopening. We need to vendor in a new copy of deps/v8

https://travis-ci.com/nodejs/node/jobs/249904779#L252-L257

bmsdave · 2019-10-28T09:26:38Z

How can we solve this problem?

Will updating v8 to node.js solve it?
Or can I make changes to this part https://github.com/nodejs/node/tree/master/deps/v8?

cclauss · 2019-10-28T09:32:02Z

I think we need @targos and/or @Trott to weigh in on that.

targos · 2019-10-28T09:44:53Z

I don't really consider this a problem, because we don't use the scripts that are not compatible with Python 3.
If it is fixed at the V8 level (https://github.com/v8/v8), we will inherit that when we upgrade V8.
The next update will be to version 7.9: #30020

cclauss · 2019-10-28T10:16:51Z

https://github.com/v8/v8 passes these tests.

targos · 2019-10-28T11:06:14Z

It's good that V8 itself is now compatible but other V8-related tools are not.

See https://ci.nodejs.org/job/node-test-commit-v8-linux/2590/nodes=benchmark-ubuntu1604-intel-64,v8test=v8test/console

08:09:01 Failed to fetch file gs://chromium-clang-format/942fc8b1789144b8071d3fc03ff0fcbe1cf81ac8 for v8/buildtools/linux64/clang-format. [Err: gsutil requires python 2.6 or 2.7.
08:09:01 ]
08:09:01 0> Failed to fetch file gs://chromium-clang-format/942fc8b1789144b8071d3fc03ff0fcbe1cf81ac8 for v8/buildtools/linux64/clang-format, skipping. [Err: gsutil requires python 2.6 or 2.7.

By looking at https://github.com/GoogleCloudPlatform/gsutil, gsutil seems to support Python 3 so maybe the version used by depot_tools is too old?

aduh95 · 2020-04-28T16:58:34Z

Should this issue stay open? I believe this issue is outdated as Node.js build chain is compatible with Python 3.

jasnell · 2020-04-28T17:03:44Z

@nodejs/v8 @nodejs/build ... what do you think? There doesn't seem to be reason to keep this open?

cclauss · 2020-04-28T17:13:24Z

This test would crash on Python 3 but that seem like a simple fix that is not on the critical path.

./test/debugging/wasm/gdb-server/gdb_rsp.py:35:14: F821 undefined name 'xrange'
    for i in xrange(int(timeout_in_seconds / poll_time_in_seconds)):
             ^
./test/debugging/wasm/gdb-server/gdb_rsp.py:67:8: F821 undefined name 'sys'
    if sys.platform == 'win32':
       ^

targos · 2020-04-28T17:21:40Z

The issue is still valid. V8 build tools are not yet all compatible with Python 3:

$ gclient sync
Warning: Running gclient on Python 3. 
If you encounter any issues, please file a bug on crbug.com under the Infra>SDK component.
Syncing projects: 100% (28/28), done.                                                

________ running 'vpython v8/third_party/depot_tools/update_depot_tools_toggle.py --disable' in '/home/mzasso/git/chromium/v8'
[E2020-04-28T19:19:27.955163+02:00 229437 0 annotate.go:241] goroutine 1:
[E2020-04-28T19:19:27.955189+02:00 229437 0 annotate.go:241] #0 go.chromium.org/luci/vpython/python/find.go:93 - python.Find()
[E2020-04-28T19:19:27.955199+02:00 229437 0 annotate.go:241]   annotation #0:
[E2020-04-28T19:19:27.955205+02:00 229437 0 annotate.go:241]     reason: no Python found
[E2020-04-28T19:19:27.955210+02:00 229437 0 annotate.go:241]   annotation #1:
[E2020-04-28T19:19:27.955215+02:00 229437 0 annotate.go:241]     reason: could not find appropriate executable for: "python2.7"

I don't know if we should keep this open though, there's nothing we can do about it in Node.js.

mmarchini · 2020-04-28T17:25:31Z

IMO that should be tracked upstream, unless Node.js folks are actively working with V8 and Chromium to make the tools Python 3-compatible. Otherwise, as @targos mentioned, there's nothing we can do on our side.

It's also not a huge deal since it only affects folks building V8 through make-v8, which is limited to a subset of collaborators working on V8 upgrades on Node.js.

cclauss · 2020-04-28T17:32:26Z

@targos, What happens when you add the 3??

vpython3 v8/third_party/depot_tools/update_depot_tools_toggle.py --disable' in '/home/mzasso/git/chromium/v8'
       ^

targos · 2020-04-28T17:34:25Z

@cclauss nothing is printed, exits with code 0

ryzokuken · 2020-04-29T10:52:34Z

Agreed with @targos and @mmarchini that this issue should be tracked upstream.

Trott closed this as completed Nov 26, 2018

refack reopened this Nov 26, 2018

refack added v8 engine Issues and PRs related to the V8 dependency. inspector Issues and PRs related to the V8 inspector protocol python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. labels Nov 26, 2018

This was referenced Dec 7, 2018

test: do not lint macros files #24885

Closed

Can we pip install jinja2 and markupsafe instead of vendoring them in? #24891

Closed

This was referenced Dec 19, 2018

Support for Python 3 nodejs/node-gyp#1337

Closed

Suggestion: New Python & GYP strategic initiative or working group nodejs/TSC#642

Closed

hayd mentioned this issue Jan 5, 2019

Port tools/*.py to python 3 denoland/deno#464

Closed

ry mentioned this issue Sep 9, 2019

Support python3 denoland/deno#2893

Closed

cclauss added the Hacktoberfest label Oct 2, 2019

AgentJ08 mentioned this issue Oct 3, 2019

Updated functions for python3 v8/v8#35

Closed

cclauss closed this as completed Oct 18, 2019

cclauss reopened this Oct 27, 2019

targos mentioned this issue Oct 28, 2019

V8 CI is not Python 3 compatible #30152

Closed

Trott removed the Hacktoberfest label Nov 3, 2019

This was referenced Jan 16, 2020

test: add promises metadata to postmortem test #31357

Closed

"Error: Command 'download_from_google_storage" on node-test-commit-v8-linux nodejs/build#2135

Closed

cclauss closed this as completed Apr 29, 2020

Dependency v8 is not Python 3 compatible #24512

Dependency v8 is not Python 3 compatible #24512

Comments

cclauss commented Nov 20, 2018 • edited Loading

Trott commented Nov 26, 2018

refack commented Nov 26, 2018

refack commented Nov 26, 2018

bmsdave commented Nov 28, 2018

hashseed commented Nov 28, 2018

cclauss commented Nov 28, 2018 • edited Loading

bmsdave commented Nov 28, 2018

alexkozy commented Nov 28, 2018

cclauss commented Dec 12, 2018

cclauss commented Dec 14, 2018 • edited Loading

cclauss commented Jan 19, 2019

bmsdave commented Jan 21, 2019

cclauss commented Jan 21, 2019 • edited Loading

bmsdave commented Jan 21, 2019

cclauss commented Jan 21, 2019 • edited Loading

hayd commented Jan 21, 2019

cclauss commented Jan 21, 2019 • edited Loading

hayd commented Jan 21, 2019

cclauss commented Jan 28, 2019

cclauss commented Oct 2, 2019

12101111 commented Oct 13, 2019

bmsdave commented Oct 17, 2019

bmsdave commented Oct 18, 2019

hayd commented Oct 18, 2019

cclauss commented Oct 18, 2019

cclauss commented Oct 27, 2019

bmsdave commented Oct 28, 2019

cclauss commented Oct 28, 2019

targos commented Oct 28, 2019

cclauss commented Oct 28, 2019

targos commented Oct 28, 2019

aduh95 commented Apr 28, 2020 • edited Loading

jasnell commented Apr 28, 2020

cclauss commented Apr 28, 2020

targos commented Apr 28, 2020

mmarchini commented Apr 28, 2020

cclauss commented Apr 28, 2020

targos commented Apr 28, 2020

ryzokuken commented Apr 29, 2020

cclauss commented Nov 20, 2018 •

edited

Loading

cclauss commented Nov 28, 2018 •

edited

Loading

cclauss commented Dec 14, 2018 •

edited

Loading

cclauss commented Jan 21, 2019 •

edited

Loading

cclauss commented Jan 21, 2019 •

edited

Loading

cclauss commented Jan 21, 2019 •

edited

Loading

aduh95 commented Apr 28, 2020 •

edited

Loading