GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tools: Use print() function on both Python 2 and 3 by cclauss · Pull Request #24486 · nodejs/node

Conversation

@cclauss

@cclauss cclauss commented Nov 19, 2018 •
edited by addaleax
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. i18n-api Issues and PRs related to Node.js internationalization support. test Issues and PRs related to Node.js core tests and test infrastructure. tools Issues and PRs related to the tools directory. labels Nov 19, 2018
addaleax
addaleax approved these changes Nov 19, 2018
View reviewed changes
@addaleax

addaleax commented Nov 19, 2018

Copy link
Copy Markdown
Member

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…
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cclauss

cclauss commented Nov 19, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cclauss
cclauss force-pushed the tools-print-function branch from 6651436 to d0b33fb Compare November 19, 2018 11:55
@Trott

Trott commented Nov 19, 2018

Copy link
Copy Markdown
Member

@nodejs/python
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Nov 19, 2018
View reviewed changes
@refack

refack commented Nov 19, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack refack added the python PRs and issues that require attention from people who are familiar with Python. label Nov 19, 2018
refack
refack suggested changes Nov 19, 2018
View reviewed changes

@refack refack left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should exclude tools/GYP and tools/inspector_protocol

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@refack

refack commented Nov 19, 2018

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack refack self-assigned this Nov 19, 2018
@cclauss
cclauss force-pushed the tools-print-function branch from d0b33fb to 4021ecd Compare November 19, 2018 21:56
thefourtheye
thefourtheye reviewed Nov 20, 2018
View reviewed changes
Comment thread tools/inspector_protocol/ConvertProtocolToJSON.py Outdated

@thefourtheye thefourtheye Nov 20, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@refack refack Nov 20, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
thefourtheye reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@thefourtheye thefourtheye Nov 20, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@refack refack Nov 20, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cclauss Sorry I didn't notice this before.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cclauss cclauss Nov 20, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@refack refack Nov 20, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aslushnikov aslushnikov Nov 26, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@refack

refack commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack

refack commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack
refack approved these changes Nov 20, 2018
View reviewed changes
@refack

refack commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack refack mentioned this pull request Nov 20, 2018
build: simplify build of inspector protocol #22680
Closed
3 tasks done
@cclauss

cclauss commented Nov 20, 2018

Copy link
Copy Markdown
Contributor Author

Sounds like a good plan.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack refack added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Nov 20, 2018
@refack

refack commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

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

Reviewers please consider this for fast-tracking by 👍 .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
thefourtheye
thefourtheye approved these changes Nov 20, 2018
View reviewed changes
@thefourtheye

thefourtheye commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

@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 and srl295 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack refack removed the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Nov 20, 2018
@refack

refack commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

can we wait till @srl295 confirms?

ack.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cclauss

cclauss commented Nov 25, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request Nov 27, 2018
cclauss @targos
tools: use print() function on both Python 2 and 3
8bf67c8 
PR-URL: #24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
cclauss @rvagg
tools: use print() function on both Python 2 and 3
23d8152 
PR-URL: #24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 30, 2018
build: fix line length off by one error #24748
Closed
4 tasks done
danbev pushed a commit that referenced this pull request Dec 3, 2018
@BridgeAR @danbev
build: fix line length off by one error
3099647 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danbev pushed a commit that referenced this pull request Dec 3, 2018
@BridgeAR @danbev
build: add line break as soon tests are done
5b90902 
PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BridgeAR added a commit that referenced this pull request Dec 5, 2018
@BridgeAR
build: fix line length off by one error
e0e15da 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BridgeAR added a commit that referenced this pull request Dec 5, 2018
@BridgeAR
build: add line break as soon tests are done
6b71099 
PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
v11.4.0 proposal #24854
Merged
4 tasks done
This was referenced Dec 7, 2018
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#261
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#262
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#263
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#264
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#265
Closed
@srl295

srl295 commented Dec 18, 2018

Copy link
Copy Markdown
Member

@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.
thefourtheye reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
srl295
srl295 reviewed Dec 18, 2018
View reviewed changes

@srl295 srl295 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@thefourtheye

thefourtheye commented Dec 19, 2018

Copy link
Copy Markdown
Contributor

@srl295 Thanks for confirming 🙂
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@srl295

srl295 commented Dec 19, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
cclauss @refack
tools: use print() function on both Python 2 and 3
c7e78f7 
PR-URL: nodejs#24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
build: fix line length off by one error
6b3b868 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
build: add line break as soon tests are done
87881b4 
PR-URL: nodejs#24748
Refs: nodejs#24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 11, 2019
cclauss @BethGriggs
tools: use print() function on both Python 2 and 3
8a5b5e1 
PR-URL: #24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@BethGriggs BethGriggs added the land-on-v10.x label Feb 11, 2019
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
@BridgeAR @BethGriggs
build: add line break as soon tests are done
18d81c9 
PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
@BridgeAR @BethGriggs
build: fix line length off by one error
b72bc11 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@BethGriggs 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
cclauss @rvagg
tools: use print() function on both Python 2 and 3
5493261 
PR-URL: #24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@BridgeAR @rvagg
build: add line break as soon tests are done
5844a06 
PR-URL: #24748
Refs: #24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@BridgeAR @rvagg
build: fix line length off by one error
84353ac 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@refack refack removed their assignment Mar 11, 2019
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
cclauss @BaochengSu
tools: use print() function on both Python 2 and 3
32bfb79 
PR-URL: nodejs#24486
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
(cherry picked from commit b507783)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srl295 srl295 srl295 left review comments
@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
+3 more reviewers
@aslushnikov aslushnikov aslushnikov left review comments
@refack refack refack approved these changes
@thefourtheye thefourtheye thefourtheye approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

build Issues and PRs related to Node.js builds or CI infrastructure. i18n-api Issues and PRs related to Node.js internationalization support. python PRs and issues that require attention from people who are familiar with Python. test Issues and PRs related to Node.js core tests and test infrastructure. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@cclauss @addaleax @Trott @refack @thefourtheye @srl295 @jasnell @aslushnikov @BethGriggs @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.