tools: Use print() function on both Python 2 and 3 #24486

cclauss · Nov 19, 2018

A subset of #23669 to simplify the review process. @refack @addaleax

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

addaleax · 2018-11-19T11:15:51Z

addaleax approved these changes

Nov 19, 2018

View changes

addaleax · Nov 19, 2018

Is there any chance of the GYP patches being upstreamed? If not, it would be great to finally do the thing where we pull changes from our own fork of it…

cclauss · Nov 19, 2018

@addaleax Working on that in parallel. It would be a lot easier is we pip installed our Python dependencies instead of vendoring them in.

Trott · Nov 19, 2018

@nodejs/python

jasnell · 2018-11-19T14:24:11Z

jasnell approved these changes

Nov 19, 2018

View changes

refack · Nov 19, 2018

@cclauss thank you for making it easier to review.
IMHO you should exclude tools/gyp. For that I'm going to port your patch from nodejs/node-gyp#1335 since it has already received test coverage.
(I plan to do it VIA refack/GYP, once I get the GYP CI test suite revived)

refack · 2018-11-19T19:53:01Z

refack requested changes

Nov 19, 2018

View changes

Should exclude tools/GYP and tools/inspector_protocol

refack · Nov 19, 2018

P.S. I'm self-assigned this so I'll get notifications from Github, and so that I will not lose track of it and help steward it to completion.

thefourtheye · 2018-11-20T02:27:26Z

thefourtheye reviewed

Nov 20, 2018

View changes

thefourtheye · Nov 20, 2018

tools/inspector_protocol/ConvertProtocolToJSON.py

@@ -1,3 +1,4 @@
+from __future__ import print_function


Nit: This would be better if it followed the copyright notice.

This isn't our code. It should be patched upstream at https://chromium.googlesource.com/deps/inspector_protocol/

Ah, okay. Sure, this has to be updated in upstream then.

@cclauss Sorry I didn't notice this before.

I will remove inspector_protocol from this PR.

However this opens up a can of worms that I do not have a solution for. Chromium in general and v8 specifically are not on GitHub. Their GitHub mirror does not accept pull requests. 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 was a godsend to the JavaScript community but its Python code needs to be modernized, removed, or replaced with JavaScript, Go, etc. 407 days until Python 2 end of life. @hugovk your expert advise here please.

So they do accept PRs (which they call CLs) you just need to do it their way:
https://v8.dev/docs/contribute
As for inspector_protocol it's a sub project so submitting patches should be simpler.
/cc @aslushnikov @ak239

As for inspector_protocol it's a sub project so submitting patches should be simpler.

It's quite similar for both v8 and inspector-protocol.

For the inspector-protocol, check out these links:

repository: https://chromium.googlesource.com/deps/inspector_protocol/

how to upload a patch: https://www.chromium.org/developers/contributing-code#TOC-Uploading-a-change-for-review

refack · Nov 20, 2018

@srl295 where do the python scripts in tools/icu/ come from?

refack · Nov 20, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/18783/

refack · 2018-11-20T13:59:26Z

refack approved these changes

Nov 20, 2018

View changes

refack · Nov 20, 2018

I will remove inspector_protocol from this PR.

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.

@cclauss from the Node.js perspective, IMHO our first goal is to get the main build@test (a.k.a CI) workflow compatible with python3.
Since inspector_protocol is a code-gen tool we can workaround it by checking-in the generated code (#22680). We need to identify which other python scripts from V8 we use (via https://github.com/nodejs/node/blob/master/deps/v8/gypfiles/v8.gyp)

cclauss · Nov 20, 2018

Sounds like a good plan.

refack · Nov 20, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/18805/

Reviewers please consider this for fast-tracking by 👍 .

thefourtheye · 2018-11-20T15:35:21Z

thefourtheye approved these changes

Nov 20, 2018

View changes

thefourtheye · Nov 20, 2018

@srl295 where do the python scripts in tools/icu/ come from?

I looked at the git logs, it looks like its our own. Just to be sure, can we wait till @srl295 confirms?

refack · Nov 20, 2018

can we wait till @srl295 confirms?

ack.

cclauss · Nov 25, 2018

Should I break this into seven separate PRs to make it easier to review?


      tools: use print() function on both Python 2 and 3

PR-URL: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>


      tools: use print() function on both Python 2 and 3

PR-URL: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>


      build: fix line length off by one error

While running the test suite the progress bar shows former line
endings if the new line is shorter than the former line. The length
was calculated without the line ending. It is now an empty string
to prevent the off by one error instead of using extra whitespace.

PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>


      build: add line break as soon tests are done

PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>


      build: fix line length off by one error

While running the test suite the progress bar shows former line
endings if the new line is shorter than the former line. The length
was calculated without the line ending. It is now an empty string
to prevent the off by one error instead of using extra whitespace.

PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>


      build: add line break as soon tests are done

PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

srl295 · Dec 18, 2018

@refack sorry :( yes, they are 'our own'. I wrote them origianlly to be part of ICU, but the python scripts should be considered part of node.

Incidentally, ICU itself will require python for build-from-repo (not from tarball). At this point it will require python 2.7 or 3.

srl295 · 2018-12-18T21:41:42Z

srl295 reviewed

Dec 18, 2018

View changes

I really thought I +1'ed a similar change here. but anyway, post merge LGTM. There's no need to upstream ICU's .py files at this point.

thefourtheye · Dec 19, 2018

@srl295 Thanks for confirming 🙂

srl295 · Dec 19, 2018

But on this point ICU as of 2 days ago does actually have its own slicer— please see #25136 and comment on the upstream design. This would replace node's special code (and it runs on python 2.7 and 3).


      tools: use print() function on both Python 2 and 3

PR-URL: nodejs#24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>


      build: fix line length off by one error

While running the test suite the progress bar shows former line
endings if the new line is shorter than the former line. The length
was calculated without the line ending. It is now an empty string
to prevent the off by one error instead of using extra whitespace.

PR-URL: nodejs#24748
Refs: nodejs#24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>


      build: add line break as soon tests are done

PR-URL: nodejs#24748
Refs: nodejs#24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>


      tools: use print() function on both Python 2 and 3

PR-URL: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>


      build: add line break as soon tests are done

PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>


      build: fix line length off by one error

While running the test suite the progress bar shows former line
endings if the new line is shorter than the former line. The length
was calculated without the line ending. It is now an empty string
to prevent the off by one error instead of using extra whitespace.

PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>


      tools: use print() function on both Python 2 and 3

PR-URL: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>


      build: add line break as soon tests are done

PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>


      build: fix line length off by one error

While running the test suite the progress bar shows former line
endings if the new line is shorter than the former line. The length
was calculated without the line ending. It is now an empty string
to prevent the off by one error instead of using extra whitespace.

PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added build intl test tools labels Nov 19, 2018

cclauss force-pushed the cclauss:tools-print-function branch from 6651436 to d0b33fb Nov 19, 2018

refack added the python label Nov 19, 2018

refack self-assigned this Nov 19, 2018

cclauss force-pushed the cclauss:tools-print-function branch from d0b33fb to 4021ecd Nov 19, 2018

refack referenced this pull request Nov 20, 2018

build: simplify build of inspector protocol #22680

Closed

3 of 3 tasks complete

refack added the fast-track label Nov 20, 2018

refack removed the fast-track label Nov 20, 2018

cclauss deleted the cclauss:tools-print-function branch Nov 26, 2018

targos added a commit that referenced this pull request Nov 27, 2018

rvagg added a commit that referenced this pull request Nov 28, 2018

tools: use print() function on both Python 2 and 3
PR-URL: #24486 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

23d8152

BridgeAR referenced this pull request Nov 30, 2018

build: fix line length off by one error #24748

Closed

4 of 4 tasks complete

danbev added a commit that referenced this pull request Dec 3, 2018

build: add line break as soon tests are done
PR-URL: #24748 Refs: #24486 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Loading status checks…

5b90902

BridgeAR added a commit that referenced this pull request Dec 5, 2018

BridgeAR referenced this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 of 4 tasks complete

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

tools: use print() function on both Python 2 and 3
PR-URL: nodejs#24486 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

c7e78f7

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

build: add line break as soon tests are done
PR-URL: nodejs#24748 Refs: nodejs#24486 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

87881b4

BethGriggs added a commit that referenced this pull request Feb 11, 2019

BethGriggs added the land-on-v10.x label Feb 11, 2019

BethGriggs added a commit that referenced this pull request Feb 12, 2019

BethGriggs referenced this pull request Feb 12, 2019

v10.15.3 proposal #26063

Merged

rvagg added a commit that referenced this pull request Feb 28, 2019

tools: use print() function on both Python 2 and 3
PR-URL: #24486 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

5493261

rvagg added a commit that referenced this pull request Feb 28, 2019

build: add line break as soon tests are done
PR-URL: #24748 Refs: #24486 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

5844a06

rvagg added a commit that referenced this pull request Feb 28, 2019

refack removed their assignment Mar 11, 2019

nodejs/node

Join GitHub today

tools: Use print() function on both Python 2 and 3 #24486

Conversation

cclauss commented Nov 19, 2018 • edited by addaleax

Checklist

nodejs-github-bot added build intl test tools labels Nov 19, 2018

This comment has been minimized.

addaleax commented Nov 19, 2018

This comment has been minimized.

cclauss commented Nov 19, 2018 • edited

cclauss force-pushed the cclauss:tools-print-function branch from 6651436 to d0b33fb Nov 19, 2018

This comment has been minimized.

Trott commented Nov 19, 2018

This comment has been minimized.

refack commented Nov 19, 2018

refack added the python label Nov 19, 2018

refack left a comment • edited

This comment has been minimized.

refack commented Nov 19, 2018

refack self-assigned this Nov 19, 2018

cclauss force-pushed the cclauss:tools-print-function branch from d0b33fb to 4021ecd Nov 19, 2018

This comment has been minimized.

thefourtheye Nov 20, 2018

This comment has been minimized.

refack Nov 20, 2018

This comment has been minimized.

thefourtheye Nov 20, 2018

This comment has been minimized.

refack Nov 20, 2018

This comment has been minimized.

cclauss Nov 20, 2018

This comment has been minimized.

refack Nov 20, 2018

This comment has been minimized.

aslushnikov Nov 26, 2018

This comment has been minimized.

refack commented Nov 20, 2018

This comment has been minimized.

refack commented Nov 20, 2018

This comment has been minimized.

refack commented Nov 20, 2018

This comment has been minimized.

cclauss commented Nov 20, 2018

refack added the fast-track label Nov 20, 2018

This comment has been minimized.

refack commented Nov 20, 2018

This comment has been minimized.

thefourtheye commented Nov 20, 2018

refack removed the fast-track label Nov 20, 2018

This comment has been minimized.

refack commented Nov 20, 2018

This comment has been minimized.

cclauss commented Nov 25, 2018 • edited

cclauss deleted the cclauss:tools-print-function branch Nov 26, 2018

This comment has been minimized.

srl295 commented Dec 18, 2018

srl295 left a comment

This comment has been minimized.

thefourtheye commented Dec 19, 2018

This comment has been minimized.

srl295 commented Dec 19, 2018

BethGriggs added the land-on-v10.x label Feb 11, 2019

refack removed their assignment Mar 11, 2019

cclauss commented Nov 19, 2018 •

edited by addaleax

cclauss commented Nov 19, 2018 •

edited

cclauss force-pushed the cclauss:tools-print-function branch from `6651436` to `d0b33fb` Nov 19, 2018

refack left a comment •

edited

cclauss force-pushed the cclauss:tools-print-function branch from `d0b33fb` to `4021ecd` Nov 19, 2018

cclauss commented Nov 25, 2018 •

edited