Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Nov 26, 2018

Conversation

@cclauss
Copy link
Contributor

commented 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

This comment has been minimized.

Copy link
Member

commented 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

This comment has been minimized.

Copy link
Contributor Author

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

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

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

@refack

This comment has been minimized.

Copy link
Member

commented 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 refack added the python label Nov 19, 2018

@refack
Copy link
Member

left a comment

Should exclude tools/GYP and tools/inspector_protocol

@refack

This comment has been minimized.

Copy link
Member

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

@refack refack self-assigned this Nov 19, 2018

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

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

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@refack

refack Nov 20, 2018

Member

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

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@refack

refack Nov 20, 2018

Member

@cclauss Sorry I didn't notice this before.

This comment has been minimized.

Copy link
@cclauss

cclauss Nov 20, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@refack

refack Nov 20, 2018

Member

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

This comment has been minimized.

Copy link
@aslushnikov

aslushnikov Nov 26, 2018

Contributor

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:

@refack

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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

@refack

This comment has been minimized.

@refack
refack approved these changes Nov 20, 2018
@refack

This comment has been minimized.

Copy link
Member

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

@refack refack referenced this pull request Nov 20, 2018
3 of 3 tasks complete
@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

Sounds like a good plan.

@refack refack added the fast-track label Nov 20, 2018

@refack

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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

Reviewers please consider this for fast-tracking by 👍 .

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented 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 refack removed the fast-track label Nov 20, 2018

@refack

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

can we wait till @srl295 confirms?

ack.

@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2018

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

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

targos added a commit that referenced this pull request Nov 27, 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>
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>
@BridgeAR BridgeAR referenced this pull request Nov 30, 2018
4 of 4 tasks complete
danbev added a commit that referenced this pull request Dec 3, 2018
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>
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>
BridgeAR added a commit that referenced this pull request Dec 5, 2018
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>
BridgeAR added a commit that referenced this pull request Dec 5, 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>
@BridgeAR BridgeAR referenced this pull request Dec 5, 2018
4 of 4 tasks complete
@srl295

This comment has been minimized.

Copy link
Member

commented 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
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

@srl295 Thanks for confirming 🙂

@srl295

This comment has been minimized.

Copy link
Member

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

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>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
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>
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>
BethGriggs added a commit that referenced this pull request Feb 11, 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>
BethGriggs added a commit that referenced this pull request Feb 12, 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>
BethGriggs added a commit that referenced this pull request Feb 12, 2019
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>
@BethGriggs BethGriggs referenced this pull request Feb 12, 2019
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>
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>
rvagg added a commit that referenced this pull request Feb 28, 2019
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>

@refack refack removed their assignment Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.