tools: Use print() function on both Python 2 and 3 by cclauss · Pull Request #24486 · nodejs/node · GitHub

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

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…

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

@nodejs/python

@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)

Should exclude tools/GYP and tools/inspector_protocol

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.

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

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

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

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)

Sounds like a good plan.

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

Reviewers please consider this for fast-tracking by 👍 .

@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?

can we wait till @srl295 confirms?

ack.

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

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>

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>

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

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>

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

@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.

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.

@srl295 Thanks for confirming 🙂

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).

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>

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>

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

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>

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

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>

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>

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

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>

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> (cherry picked from commit b507783)

nodejs-github-bot added build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Nov 19, 2018

addaleax approved these changes Nov 19, 2018

View reviewed changes

cclauss force-pushed the tools-print-function branch from 6651436 to d0b33fb Compare November 19, 2018 11:55

jasnell approved these changes Nov 19, 2018

View reviewed changes

refack added the python PRs and issues that require attention from people who are familiar with Python. label Nov 19, 2018

refack suggested changes Nov 19, 2018

View reviewed changes

refack self-assigned this Nov 19, 2018

cclauss force-pushed the tools-print-function branch from d0b33fb to 4021ecd Compare November 19, 2018 21:56

thefourtheye reviewed Nov 20, 2018

View reviewed changes

refack approved these changes Nov 20, 2018

View reviewed changes

refack mentioned this pull request Nov 20, 2018

build: simplify build of inspector protocol #22680

Closed

3 tasks

refack added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 20, 2018

thefourtheye approved these changes Nov 20, 2018

View reviewed changes

refack removed the fast-track PRs that do not need to wait for 48 hours to land. label Nov 20, 2018

BridgeAR mentioned this pull request Nov 30, 2018

build: fix line length off by one error #24748

Closed

4 tasks

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

build: add line break as soon tests are done

5b90902

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

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

build: add line break as soon tests are done

6b71099

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

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

srl295 reviewed Dec 18, 2018

View reviewed changes

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

build: add line break as soon tests are done

87881b4

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

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

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

build: add line break as soon tests are done

18d81c9

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

BethGriggs mentioned this pull request Feb 12, 2019

v10.15.3 proposal #26063

Merged

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

build: add line break as soon tests are done

5844a06

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

refack removed their assignment Mar 11, 2019

tools: Use print() function on both Python 2 and 3 by cclauss · Pull Request #24486 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

tools: Use print() function on both Python 2 and 3#24486refack merged 1 commit intonodejs:masternodejs/node:masterfrom cclauss:tools-print-functioncclauss/node:tools-print-functionCopy head branch name to clipboard

Conversation

cclauss commented Nov 19, 2018 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax commented Nov 19, 2018

Uh oh!

cclauss commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 19, 2018

Uh oh!

refack commented Nov 19, 2018

Uh oh!

refack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 19, 2018

Uh oh!

thefourtheye Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

refack Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

thefourtheye Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

refack Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

cclauss Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

refack Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

aslushnikov Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 20, 2018

Uh oh!

refack commented Nov 20, 2018

Uh oh!

refack commented Nov 20, 2018

Uh oh!

cclauss commented Nov 20, 2018

Uh oh!

refack commented Nov 20, 2018

Uh oh!

thefourtheye commented Nov 20, 2018

Uh oh!

refack commented Nov 20, 2018

Uh oh!

cclauss commented Nov 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srl295 commented Dec 18, 2018

Uh oh!

srl295 left a comment

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Dec 19, 2018

Uh oh!

srl295 commented Dec 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tools: Use print() function on both Python 2 and 3#24486
refack merged 1 commit into
nodejs:masterfrom
cclauss:tools-print-function

cclauss commented Nov 19, 2018 •
edited by addaleax
Loading

cclauss commented Nov 19, 2018 •
edited
Loading

refack left a comment •
edited
Loading

cclauss commented Nov 25, 2018 •
edited
Loading