Dependency v8 is not Python 3 compatible

cclauss commented 5 years ago

The open bug reports on chromium.org:

@bmsdave commit to upstream: https://github.com/nodejs/node/issues/24512#issuecomment-460586513

Trott commented 5 years ago

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 commented 5 years ago

PR has landed so I assume this can be closed.

The files in deps/v8 and tools/inspector_protocol + jinja2 + markupsafe where excluded from https://github.com/nodejs/node/pull/24486. So this is still an issue.

refack commented 5 years ago

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

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

bmsdave commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 (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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@ak239 @bmsdave @hashseed Some low hanging fruit in the upstream __inspector_protocol [repo__](https://chromium.googlesource.com/deps/inspector_protocol/).

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 commented 5 years ago

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

Please star these issues and fixes.

cclauss commented 5 years ago

Any progress on this issues?

bmsdave commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Let's get _inspectorprotocol 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 commented 5 years ago

@cclauss what do you mean "given https://github.com/v8/v8/pull/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 commented 5 years ago

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

hayd commented 5 years ago

@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 commented 5 years ago

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

cclauss commented 5 years ago

But the xrange() fix in https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1387524 was not included.

bmsdave commented 5 years ago

This task include fix with xrange(): https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1440581 It looks like it will soon pass

bmsdave commented 5 years ago

As I understand it, the next step is to add changes to the v8. @cclauss, you have not yet transferred edits from https://github.com/v8/v8/pull/26 in chromium-review?

cclauss commented 5 years ago

That is correct @bmsdave. It would be great if you could work on that part of the puzzle.

bmsdave commented 5 years ago

@cclauss OK. I got this.

thefourtheye commented 5 years ago

@bmsdave Please let me know if you need any help. I also started looking at V8's Python scripts, I am parking it now.

bmsdave commented 5 years ago

I tried to figure out python3 compatibility. It turned out to be quite a few dependencies of depot_tools are not python 3 compatible. for example boto. I'm not going to try to do anything about it yet. But it seems that later it may complicate the process of updating to python3.
I run flake8 check. Reports can be found here. -> https://travis-ci.com/bmsdave/v8/builds (thanks @cclauss !)
@thefourtheye could you advise me on ways to check python code? Apparently launch tools/run-tests.py weakly covers python code and, i think, is not a trusted source that python2 is all right.

cclauss commented 5 years ago

From https://pypi.org/project/boto ... Boto3, the next version of Boto, is now stable and recommended for general use. It can be used side-by-side with Boto in the same project, so it is easy to start using Boto3 in your existing projects as well as new projects. Going forward, API updates and all new feature work will be focused on Boto3.

cclauss commented 5 years ago

You can make the following two issues disappear by adding "from __future__ import print_function" to the top of the file (before other imports).

$ flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics --builtins="gdb,offset,slot"
./tools/locs.py:128:25: E999 SyntaxError: invalid syntax
        build_dir), file=sys.stderr)
                        ^
./tools/avg.py:198:57: E999 SyntaxError: invalid syntax
    print("{:<{}}".format("", self.last_status_len), end="\r")
                                                        ^

cclauss commented 5 years ago

__print_function__ also here...

./tools/gen-inlining-tests.py:457:14: E999 SyntaxError: invalid syntax
  return print(*args, file=FILE)
             ^

bmsdave commented 5 years ago

Something went wrong here =|

I run git cl presubmit -v with changes https://github.com/bmsdave/v8/tree/b651ebc4b73f6425fe8557fc13415dd6f3593df1

and got brocken tests:

Running presubmit commit checks ...
Running /opt/node/tmp/v8/PRESUBMIT.py
No changes in C/C++ files detected. Skipping check
No changes in Torque files detected. Skipping check
Total violating files: 0
Running /opt/node/tmp/v8/infra/testing/PRESUBMIT.py
Running /opt/node/tmp/v8/tools/PRESUBMIT.py
Running /opt/node/tmp/v8/tools/clusterfuzz/PRESUBMIT.py
Running /opt/node/tmp/v8/tools/mb/PRESUBMIT.py
Running /opt/node/tmp/v8/tools/release/PRESUBMIT.py
Running /opt/node/tmp/v8/tools/testrunner/PRESUBMIT.py

** Presubmit Messages **
unittests/predictable_wrapper_test.py (0.35s)

unittests/v8_presubmit_test.py (0.21s)

./v8_foozzie_test.py (0.27s)

Pylint (3 files using ['--disable=all', '--enable=cyclic-import']) (0.86s)

./test_scripts.py (0.26s)

testproc/variant_unittest.py (0.16s)

** Presubmit ERRORS **
OWNERS check failed: this CL has no Gerrit change number, so we can't check it for approvals.

bmsdave@gmail.com is not in AUTHORS file. If you are a new contributor, please visit
https://www.chromium.org/developers/contributing-code and read the "Legal" section
If you are a chromite, verify the contributor signed the CLA.

unittests/run_perf_test.py (0.30s) failed
ERROR

======================================================================
ERROR: setUpClass (__main__.PerfTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "unittests/run_perf_test.py", line 101, in setUpClass
    import run_perf
  File "/opt/node/tmp/v8/tools/run_perf.py", line 105, in <module>
    from past.builtins import basestring
ImportError: No module named past.builtins

----------------------------------------------------------------------
Ran 0 tests in 0.003s

FAILED (errors=1)

unittests/run_tests_test.py (0.21s) failed
ERROR

======================================================================
ERROR: setUpClass (__main__.SystemTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "unittests/run_tests_test.py", line 150, in setUpClass
    from testrunner import standard_runner
  File "/opt/node/tmp/v8/tools/testrunner/standard_runner.py", line 16, in <module>
    from . import base_runner
  File "/opt/node/tmp/v8/tools/testrunner/base_runner.py", line 27, in <module>
    from testrunner.local import testsuite
  File "/opt/node/tmp/v8/tools/testrunner/local/testsuite.py", line 37, in <module>
    from ..objects.testcase import TestCase
  File "/opt/node/tmp/v8/tools/testrunner/objects/testcase.py", line 29, in <module>
    from past.builtins import cmp
ImportError: No module named past.builtins

----------------------------------------------------------------------
Ran 0 tests in 0.048s

FAILED (errors=1)

Pylint (3 files using ['--disable=cyclic-import'] on 4 cores) (3.31s) failed
************* Module mb
W: 15, 0: Redefining built-in 'cmp' (redefined-builtin)

./mb_unittest.py (0.17s) failed
Traceback (most recent call last):
  File "./mb_unittest.py", line 14, in <module>
    import mb
  File "/opt/node/tmp/v8/tools/mb/mb.py", line 15, in <module>
    from past.builtins import cmp
ImportError: No module named past.builtins

mb_validate (0.15s) failed
Traceback (most recent call last):
  File "mb.py", line 15, in <module>
    from past.builtins import cmp
ImportError: No module named past.builtins

local/testsuite_unittest.py (0.19s) failed
Traceback (most recent call last):
  File "local/testsuite_unittest.py", line 16, in <module>
    from testrunner.local.testsuite import TestSuite
  File "/opt/node/tmp/v8/tools/testrunner/local/testsuite.py", line 37, in <module>
    from ..objects.testcase import TestCase
  File "/opt/node/tmp/v8/tools/testrunner/objects/testcase.py", line 29, in <module>
    from past.builtins import cmp
ImportError: No module named past.builtins

local/statusfile_unittest.py (0.17s) failed
Traceback (most recent call last):
  File "local/statusfile_unittest.py", line 11, in <module>
    from . import statusfile
ValueError: Attempted relative import in non-package

local/pool_unittest.py (0.18s) failed
testAdd (__main__.PoolTest) ... ERROR
testException (__main__.PoolTest) ... ERROR
testNormal (__main__.PoolTest) ... ERROR

======================================================================
ERROR: testAdd (__main__.PoolTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "local/pool_unittest.py", line 49, in testAdd
    pool = Pool(3)
  File "/opt/node/tmp/v8/tools/testrunner/local/pool.py", line 127, in __init__
    self.work_queue = Queue()
NameError: global name 'Queue' is not defined

======================================================================
ERROR: testException (__main__.PoolTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "local/pool_unittest.py", line 35, in testException
    pool = Pool(3)
  File "/opt/node/tmp/v8/tools/testrunner/local/pool.py", line 127, in __init__
    self.work_queue = Queue()
NameError: global name 'Queue' is not defined

======================================================================
ERROR: testNormal (__main__.PoolTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "local/pool_unittest.py", line 25, in testNormal
    pool = Pool(3)
  File "/opt/node/tmp/v8/tools/testrunner/local/pool.py", line 127, in __init__
    self.work_queue = Queue()
NameError: global name 'Queue' is not defined

----------------------------------------------------------------------
Ran 3 tests in 0.001s

FAILED (errors=3)

Presubmit checks took 7.5s to calculate.

Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.

I can not yet figure out where the dependencies come from, since globally I have future installed, but I still get ImportError: No module named past.builtins

cclauss commented 5 years ago

pip install future will give you past.

try:
  from queue import Queue  # Python 3
except NameError:
  from Queue import Queue  # Python 2

cclauss commented 5 years ago

python3 -m pip install future python2 -m pip install future

bmsdave commented 5 years ago

Yes. I have the future package installed locally. But the error remains. If i run tests manually -> all pass successfully. But with git cl presubmit -v -> the tests do not pass. I have an assumption that this command runs in some virtual environment.

bmsdave@bmsdave:/opt/node/tmp/v8$ git cl presubmit -v
Running presubmit commit checks ...
Running /opt/node/tmp/v8/PRESUBMIT.py
[I2019-02-05 09:34:35,937 30658 140701642540864 driver.py] Generating grammar tables from /usr/lib/python2.7/lib2to3/Grammar.txt
[I2019-02-05 09:34:35,958 30658 140701642540864 driver.py] Generating grammar tables from /usr/lib/python2.7/lib2to3/PatternGrammar.txt
No changes in C/C++ files detected. Skipping check
No changes in Torque files detected. Skipping check
Total violating files: 0
[I2019-02-05 09:34:36,083 30658 140701642540864 presubmit_canned_checks.py] Valid authors are *@google.com, *@chromium.org, *@sdesigns.com, *@arm.com, *@palm.com, *@igalia.com, *@joyent.com, *@bloomberg.net, *@nvidia.com, *@blackberry.com, *@opera.com, *@intel.com, *@microsoft.com, *@mips.com, *@imgtec.com, *@wavecomp.com, *@loongson.cn, *@codeaurora.org, *@homejinni.com, *@*ibm.com, *@*.samsung.com, *@joyent.com, *@rt-rk.com, *@amazon.com, *@st.com, *@yandex-team.ru, *@strongloop.com, *@fb.com, *@oculus.com, *@vewd.com, *@groupon.com, *@meteor.com, *@cloudflare.com, deftly@gmail.com, abdulla.kamar@gmail.com, knu@freebsd.org, alessandro@leaningtech.com, akodat@rocketsoftware.com, alexbl@freebsd.org, homm86@gmail.com, avassalotti@gmail.com, alexis@janeasystems.com, eui-sang.lim@samsung.com, andreas.anyuru@gmail.com, andrew@ishiboo.com, anvaka@gmail.com, anna@addaleax.net, ant.bikineev@gmail.com, bangfu.tao@samsung.com, d1.shelton@samsung.com, ben@npmjs.com, ben@meteor.com, info@bnoordhuis.nl, demoneaux@gmail.com, bertbelder@gmail.com, burcujdogan@gmail.com, caitpotter88@gmail.com, craig.schlenter@gmail.com, cwhan.tunz@gmail.com, hichris123@gmail.com, chris@gameclosure.com, cjihrig@gmail.com, kodandersson@gmail.com, daniel.bevenius@gmail.com, dnljms@gmail.com, diaoyuanjie@gmail.com, domfarolino@gmail.com, dtc-v8@scieneer.com, dusan.m.milosavljevic@gmail.com, erich.ocean@me.com, evan.lucas@help.com, fedor@indutny.com, haimuiba@gmail.com, fdmanana@gmail.com, franziska.hinkelmann@gmail.com, ggarside@gmail.com, ngg@ngg.hu, me@gus.host, ryumiel@company100.net, henrique.ferreiro@gmail.com, mkhrfm@gmail.com, honggyu.kp@gmail.com, me@rreverser.com, ioseb.dzmanashvili@gmail.com, impinball@gmail.com, jaime@janeasystems.com, jandemooij@gmail.com, jan.krems@gmail.com, saurik@saurik.com, g00gle@chilon.net, jasnell@gmail.com, jianghua.yjh@alibaba-inc.com, joel@jms.id.au, johan@bergstroem.nu, net147@gmail.com, jbriance@cisco.com, sejunho@gmail.com, kennyluck@csail.mit.edu, karl@skomski.com, bakkot@gmail.com, kris.selden@gmail.com, kyounga@alticast.com, loorongjie@gmail.com, luis.m.reis@gmail.com, lukezarko@gmail.com, me@mmalecki.com, saper@marcincieslak.com, marcin@mwiacek.com, mateusz.szczap@gmail.com, mat@mmarchini.me, mathias@qiwi.be, mjhanselman@gmail.com, msporleder@gmail.com, maxim@mazurok.com, maxim.mossienko@gmail.com, michi@icosahedron.de, mike@w3.org, mic.besace@gmail.com, floppymaster@gmail.com, mike@mikepennisi.com, dottedmag@dottedmag.net, milton.chiang@mediatek.com, m0609.shim@samsung.com, nikai@nikai.net, mail@nh2.me, nojvek@gmail.com, oleksandr.chekhovskyi@gmail.com, p.giarrusso@gmail.com, paroga@paroga.com, peter.rybin@gmail.com, pvarga@inf.u-szeged.hu, peter.wm.wong@gmail.com, plind44@gmail.com, phistuck@gmail.com, qingyan.liqy@alibaba-inc.com, qiuyi.zqy@alibaba-inc.com, rafal@krypa.net, ray@rayglover.net, refack@gmail.com, rene@exactcode.de, waldron.rick@gmail.com, rob@robwu.nl, rm@fingolfin.org, robert.nagy@gmail.com, ruben@bridgewater.de, ry@tinyclouds.org, thechargingvolcano@gmail.com, sander@leaningtech.com, strk@keybit.net, sanjoy@playingwithpointers.com, sanxiyn@gmail.com, stefan.penner@gmail.com, sledru@mozilla.com, brn@b6n.ch, teddy.katz@gmail.com, timothygu99@gmail.com, burnus@net-b.de, tniessen@tnie.de, usharma1998@gmail.com, costan@gmail.com, vladbph@gmail.com, develar@gmail.com, vovan@shutoff.ru, kingwenlu@gmail.com, wiktor.garbacz@gmail.com, xiaoyin.l@outlook.com, contact@yannic-bonenberger.com, ccyongwang@tencent.com, xwafish@gmail.com, xaxxon@gmail.com, kewpie.w.zp@gmail.com, admin@web-tinker.com
Running /opt/node/tmp/v8/infra/testing/PRESUBMIT.py
Running /opt/node/tmp/v8/tools/PRESUBMIT.py
Running /opt/node/tmp/v8/tools/clusterfuzz/PRESUBMIT.py
Running /opt/node/tmp/v8/tools/mb/PRESUBMIT.py
[I2019-02-05 09:34:37,780 30658 140701642540864 presubmit_canned_checks.py] Running pylint on 3 files
Running /opt/node/tmp/v8/tools/release/PRESUBMIT.py
Running /opt/node/tmp/v8/tools/testrunner/PRESUBMIT.py

** Presubmit Messages **
unittests/predictable_wrapper_test.py (0.33s)

./v8_foozzie_test.py (0.27s)

Pylint (3 files using ['--disable=all', '--enable=cyclic-import']) (0.63s)

./test_scripts.py (0.27s)

testproc/variant_unittest.py (0.17s)

local/pool_unittest.py (0.53s)

** Presubmit ERRORS **
OWNERS check failed: this CL has no Gerrit change number, so we can't check it for approvals.

bmsdave@gmail.com is not in AUTHORS file. If you are a new contributor, please visit
https://www.chromium.org/developers/contributing-code and read the "Legal" section
If you are a chromite, verify the contributor signed the CLA.

unittests/run_perf_test.py (0.30s) failed
ERROR

======================================================================
ERROR: setUpClass (__main__.PerfTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "unittests/run_perf_test.py", line 101, in setUpClass
    import run_perf
  File "/opt/node/tmp/v8/tools/run_perf.py", line 105, in <module>
    from past.builtins import basestring
ImportError: No module named past.builtins

----------------------------------------------------------------------
Ran 0 tests in 0.003s

FAILED (errors=1)

unittests/run_tests_test.py (0.22s) failed
ERROR

======================================================================
ERROR: setUpClass (__main__.SystemTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "unittests/run_tests_test.py", line 151, in setUpClass
    from testrunner import standard_runner
  File "/opt/node/tmp/v8/tools/testrunner/standard_runner.py", line 17, in <module>
    from . import base_runner
  File "/opt/node/tmp/v8/tools/testrunner/base_runner.py", line 28, in <module>
    from testrunner.local import testsuite
  File "/opt/node/tmp/v8/tools/testrunner/local/testsuite.py", line 37, in <module>
    from ..objects.testcase import TestCase
  File "/opt/node/tmp/v8/tools/testrunner/objects/testcase.py", line 30, in <module>
    from past.builtins import cmp
ImportError: No module named past.builtins

----------------------------------------------------------------------
Ran 0 tests in 0.050s

FAILED (errors=1)

unittests/v8_presubmit_test.py (0.18s) failed
Traceback (most recent call last):
  File "unittests/v8_presubmit_test.py", line 15, in <module>
    from v8_presubmit import FileContentsCache, CacheableSourceFileProcessor
  File "/opt/node/tmp/v8/tools/v8_presubmit.py", line 55, in <module>
    from testrunner.local import testsuite
  File "/opt/node/tmp/v8/tools/testrunner/local/testsuite.py", line 37, in <module>
    from ..objects.testcase import TestCase
  File "/opt/node/tmp/v8/tools/testrunner/objects/testcase.py", line 30, in <module>
    from past.builtins import cmp
ImportError: No module named past.builtins

Pylint (3 files using ['--disable=cyclic-import'] on 4 cores) (1.98s) failed
************* Module mb
E:1170, 0: invalid syntax (syntax-error)
************* Module mb_unittest
W: 67,32: Unused argument 'buffer_output' (unused-argument)
W: 67,22: Unused argument 'env' (unused-argument)
W: 82,21: Unused argument 'mode' (unused-argument)
W:545, 4: Attribute 'Run' defined outside __init__ (attribute-defined-outside-init)

./mb_unittest.py (0.16s) failed
Traceback (most recent call last):
  File "./mb_unittest.py", line 15, in <module>
    import mb
  File "/opt/node/tmp/v8/tools/mb/mb.py", line 1170
    print(*args, **kwargs)
          ^
SyntaxError: invalid syntax

mb_validate (0.15s) failed
  File "mb.py", line 1170
    print(*args, **kwargs)
          ^
SyntaxError: invalid syntax

local/testsuite_unittest.py (0.18s) failed
Traceback (most recent call last):
  File "local/testsuite_unittest.py", line 16, in <module>
    from testrunner.local.testsuite import TestSuite
  File "/opt/node/tmp/v8/tools/testrunner/local/testsuite.py", line 37, in <module>
    from ..objects.testcase import TestCase
  File "/opt/node/tmp/v8/tools/testrunner/objects/testcase.py", line 30, in <module>
    from past.builtins import cmp
ImportError: No module named past.builtins

local/statusfile_unittest.py (0.15s) failed
Traceback (most recent call last):
  File "local/statusfile_unittest.py", line 11, in <module>
    import statusfile
  File "/opt/node/tmp/v8/tools/testrunner/local/statusfile.py", line 35, in <module>
    from .variants import ALL_VARIANTS
ValueError: Attempted relative import in non-package

Presubmit checks took 6.4s to calculate.

Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.

queue was fixing with https://github.com/bmsdave/v8/commit/f01480af6d3e98f17472c7a9e2b00c1400be2d7b

bmsdave commented 5 years ago

I create request to review -> https://chromium-review.googlesource.com/c/v8/v8/+/1454477

I propose to transfer the discussion there.

bmsdave commented 5 years ago

The request 1454477 was splitted into these parts: https://chromium-review.googlesource.com/c/v8/v8/+/1470121 https://chromium-review.googlesource.com/c/v8/v8/+/1470101 https://chromium-review.googlesource.com/c/v8/v8/+/1470122 https://chromium-review.googlesource.com/c/v8/v8/+/1470123

Is is merged

cclauss commented 5 years ago

How do we close out this issue? The work has been done upstream.

targos commented 5 years ago

We'll open the PR to update V8 to version 7.4 (which contains the fixes) today

hayd commented 5 years ago

Just to clarify: all v8 build scripts can run and are tested in CI on py3 as well as py2? So it's not just syntax that's updated but code now can be successfully executed in either py2 or py3?

If so, fantastic work!

cclauss commented 5 years ago

I believe that most Node build machines are currently Python 2-only. See: nodejs/build#1674

hayd commented 5 years ago

Is it the case in v8 CI tooling is still py2 only? If not, have all v8 scripts actually been run in py3, or is it expected there are still many issues to uncover and overcome?

bmsdave commented 5 years ago

@hayd Unfortunately, there is still a lot of work. So far, we have only fixed almost all the problems pointed to by the static analyzer. But v8 CI tooling is still py2 only and before actually running on python3, it seems a long way off.

But we're getting closer)

cclauss commented 5 years ago

On the static analyzer side, this is getting tantalizingly close... Update at https://github.com/v8/v8/pull/29#issuecomment-478024054

cclauss commented 5 years ago

print() is a function in Python 3. execfile() was removed in Python 3.

flake8 testing of https://github.com/v8/v8 on Python 3.7.1

$ flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics

./tools/wasm-compilation-hints/wasm-objdump-compilation-hints.py:37:54: E999 SyntaxError: invalid syntax
          print "Custom section compilationHints with", num_hints, "hints:"
                                                     ^
./test/preparser/testcfg.py:69:5: F821 undefined name 'execfile'
    execfile(pathname, {"Test": Test, "Template": Template})
    ^
1     E999 SyntaxError: invalid syntax
1     F821 undefined name 'execfile'
2

@rryk @bmsdave @targos

cclauss commented 5 years ago

@bnoordhuis More upstreaming required to cover the basics of Python 3 compatibility.

cclauss commented 4 years ago

We are backsliding: https://github.com/v8/v8/pull/29#issuecomment-537490614

12101111 commented 4 years ago

Will those changes be backported to v10 LTS?

bmsdave commented 4 years ago

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 commented 4 years ago

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

hayd commented 4 years ago

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

cclauss commented 4 years ago

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

nodejs / node

Dependency v8 is not Python 3 compatible #24512