tools: python3 compat for inspector code generator by bnoordhuis · Pull Request #29340 · nodejs/node · GitHub

The code generator takes a dict and turns it into a namedtuple. The dict
contains the key "async", which is a keyword in python 3, and rejected
by the namedtuple constructor. Rename it to "_async" to avoid the clash.

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

Should we upstream this?

@targos I'm unclear on whether tools/inspector_protocol is an upstream dependency or not. We pull done upstream changes but we also have numerous local changes according to the history.

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

There's another code generator in deps/v8/third_party/inspector_protocol that suffers from the same issue, so yes, I guess some upstreaming needs to be done. I'll look into it.

Nice! Elegant. If we are only using one version of code_generator.py then my preference be to leave the other one untouched so that we can keep an accurate track of our real dependencies as discussed at #29196 (comment)

I believe the other one is also used so it's going to need the same fixes. As to why there are two (they're very similar), I don't know.

We seem to have lots of redundant dependencies because we vendor-in and not pip install. Like the conversation at #28555 (comment)

I think I now see something that has been bugging me for a while about our Travis builds... Try this:

Start with a clean branch of master
Edit line 131 of deps/v8/third_party/inspector_protocol/code_generator.py to read
- sys.stderr.write("Deps: Failed to parse config file: %s\n\n" % exc)
Edit line 112 of tools/inspector_protocol/code_generator.py
- sys.stderr.write("Tools: Failed to parse config file: %s\n\n" % exc)
Now that the error messages are marked, commit and push and watch the Travis tests.
On the Python 3.7 run there is neither Deps: nor Tools: on the async error message.

This PR: https://travis-ci.com/nodejs/node/jobs/228507267#L1684
The experiment above: https://travis-ci.com/cclauss/node/jobs/228611533#L1676

I believe that this is because Travis CI is not building and testing the branch that you want to test but is instead building and testing master. This is why I have been submitting "small moves" pull requests that only a modify single file. Without the changes being checked into master, the Python 3 Travis CI tests are leading you astray. Am I crazy?

EDIT: Forgive the above... I was mistaken.

I did prove that it is the deps file that we want to modify and not the tools file. Thus #29346

I believe the other one is also used so it's going to need the same fixes. As to why there are two (they're very similar), I don't know.

One of the things #22680 proposed was to remove one of the two copies of the build files for the inspector protocol but it was argued against by the V8 inspector folks.

I did prove that it is the deps file that we want to modify and not the tools file. Thus #29346

@cclauss So this PR should not land as-is?

No. This is a PR that we should close because modifies the wrong file. This fix was the right one but the file was the wrong one.

#29346 modifies the correct file and therefore allows us to close #29326.

Change deps/v8/third_party/inspector_protocol/code_generator.py instead of and tools/inspector_protocol/code_generator.py.

Both code generators are in use. The changes to the one in deps/ need to be upstreamed first however.

Minor (conventional) preference for async_ instead of _async https://dbader.org/blog/meaning-of-underscores-in-python

I opened https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351 upstream.
@bnoordhuis I hope you don't mind, as I copied your changes.

Please modify this code to be aligned with the changes made in response to comments on https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351

@cclaus Done.

The code generator takes a dict and turns it into a namedtuple. The dict contains the key "async", which is a keyword in python 3.7, and rejected by the namedtuple constructor. Rename it to "async_" to avoid the clash. Fixes: nodejs#29326

This looks great and aligns with the changes that have now landed upstream:
https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351

This file also exists at deps/v8/third_party/inspector_protocol/code_generator.py so we also need to update that file before we can close #29326

Original commit message: Make code generator python3.7 compatible (async keyword). Change-Id: Ifcd8b8cb1de60a007c7bbd4564d7869e83cb7109 Fixes: nodejs#29548 Refs: - nodejs#29548 (comment) - nodejs#29520 - nodejs#29340 - https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351 - https://chromium.googlesource.com/deps/inspector_protocol/+/35c6d4d0d80b42d81bd00bcb1eb2b1093c80ed0a

Original commit message: Make code generator python3.7 compatible (async keyword). Change-Id: Ifcd8b8cb1de60a007c7bbd4564d7869e83cb7109 Fixes: #29548 Refs: - #29548 (comment) - #29520 - #29340 - https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351 - https://chromium.googlesource.com/deps/inspector_protocol/+/35c6d4d0d80b42d81bd00bcb1eb2b1093c80ed0a PR-URL: #29585 Refs: #29520 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The code generator takes a dict and turns it into a namedtuple. The dict contains the key "async", which is a keyword in python 3.7, and rejected by the namedtuple constructor. Rename it to "async_" to avoid the clash. Fixes: #29326 PR-URL: #29340 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Original commit message: Make code generator python3.7 compatible (async keyword). Change-Id: Ifcd8b8cb1de60a007c7bbd4564d7869e83cb7109 Fixes: #29548 Refs: - #29548 (comment) - #29520 - #29340 - https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351 - https://chromium.googlesource.com/deps/inspector_protocol/+/35c6d4d0d80b42d81bd00bcb1eb2b1093c80ed0a PR-URL: #29585 Refs: #29520 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The code generator takes a dict and turns it into a namedtuple. The dict contains the key "async", which is a keyword in python 3.7, and rejected by the namedtuple constructor. Rename it to "async_" to avoid the clash. Fixes: #29326 PR-URL: #29340 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Original commit message: Make code generator python3.7 compatible (async keyword). Change-Id: Ifcd8b8cb1de60a007c7bbd4564d7869e83cb7109 Fixes: #29548 Refs: - #29548 (comment) - #29520 - #29340 - https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351 - https://chromium.googlesource.com/deps/inspector_protocol/+/35c6d4d0d80b42d81bd00bcb1eb2b1093c80ed0a PR-URL: #29585 Refs: #29520 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Original commit message: Make code generator python3.7 compatible (async keyword). Change-Id: Ifcd8b8cb1de60a007c7bbd4564d7869e83cb7109 Fixes: nodejs#29548 Refs: - nodejs#29548 (comment) - nodejs#29520 - nodejs#29340 - https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351 - https://chromium.googlesource.com/deps/inspector_protocol/+/35c6d4d0d80b42d81bd00bcb1eb2b1093c80ed0a PR-URL: nodejs#29585 Refs: nodejs#29520 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Original commit message: Make code generator python3.7 compatible (async keyword). Change-Id: Ifcd8b8cb1de60a007c7bbd4564d7869e83cb7109 Fixes: #29548 Refs: - #29548 (comment) - #29520 - #29340 - https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351 - https://chromium.googlesource.com/deps/inspector_protocol/+/35c6d4d0d80b42d81bd00bcb1eb2b1093c80ed0a PR-URL: #29585 Refs: #29520 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Original commit message: Make code generator python3.7 compatible (async keyword). Change-Id: Ifcd8b8cb1de60a007c7bbd4564d7869e83cb7109 Fixes: nodejs#29548 Refs: - nodejs#29548 (comment) - nodejs#29520 - nodejs#29340 - https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351 - https://chromium.googlesource.com/deps/inspector_protocol/+/35c6d4d0d80b42d81bd00bcb1eb2b1093c80ed0a PR-URL: nodejs#29585 Refs: nodejs#29520 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Original commit message: Make code generator python3.7 compatible (async keyword). Change-Id: Ifcd8b8cb1de60a007c7bbd4564d7869e83cb7109 Fixes: #29548 Refs: - #29548 (comment) - #29520 - #29340 - https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/1781351 - https://chromium.googlesource.com/deps/inspector_protocol/+/35c6d4d0d80b42d81bd00bcb1eb2b1093c80ed0a Backport-PR-URL: #30109 PR-URL: #29585 Refs: #29520 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Aug 27, 2019

bnoordhuis mentioned this pull request Aug 27, 2019

'async' is a reserved word in Python >= 3.7 #29326

Closed

cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Aug 27, 2019

cclauss mentioned this pull request Aug 27, 2019

deps: python3 compat for inspector code generator #29346

Closed

cclauss requested changes Aug 29, 2019

View reviewed changes

cclauss mentioned this pull request Aug 29, 2019

build: ongoing list of actions for Python 3 compatibility #25789

Closed

63 tasks

This was referenced Sep 5, 2019

build: test Python 3.7 on Travis CI #29451

Closed

tools/test.py is not yet Python 3 compatible #29246

Closed

cclauss requested changes Sep 9, 2019

View reviewed changes

bnoordhuis force-pushed the fix29326 branch from f9b635f to c83ad6b Compare September 10, 2019 09:09

tools: python3 compat for inspector code generator

b2d25ce

The code generator takes a dict and turns it into a namedtuple. The dict contains the key "async", which is a keyword in python 3.7, and rejected by the namedtuple constructor. Rename it to "async_" to avoid the clash. Fixes: nodejs#29326

bnoordhuis force-pushed the fix29326 branch from c83ad6b to b2d25ce Compare September 10, 2019 09:10

targos approved these changes Sep 10, 2019

View reviewed changes

cclauss approved these changes Sep 10, 2019

View reviewed changes

sam-github mentioned this pull request Sep 16, 2019

deps: V8: cherry-pick 35c6d4d #29585

Closed

4 tasks

BridgeAR mentioned this pull request Sep 24, 2019

v12.11.0 proposal #29695

Merged

hashlash mentioned this pull request Mar 21, 2020

Fixed #31380 -- Added deployment system check for DJANGO_ALLOW_ASYNC_UNSAFE environment variable. django/django#12596

Merged

tools: python3 compat for inspector code generator by bnoordhuis · Pull Request #29340 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: python3 compat for inspector code generator - #29340

tools: python3 compat for inspector code generator#29340bnoordhuis wants to merge 1 commit intonodejs:masternodejs/node:masterfrom bnoordhuis:fix29326bnoordhuis/io.js:fix29326Copy head branch name to clipboard

Conversation

bnoordhuis commented Aug 27, 2019 • edited by cclauss Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 27, 2019

Uh oh!

targos commented Aug 27, 2019

Uh oh!

bnoordhuis commented Aug 27, 2019

Uh oh!

nodejs-github-bot commented Aug 27, 2019

Uh oh!

bnoordhuis commented Aug 27, 2019

Uh oh!

cclauss commented Aug 27, 2019

Uh oh!

bnoordhuis commented Aug 27, 2019

Uh oh!

cclauss commented Aug 27, 2019

Uh oh!

cclauss commented Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Aug 27, 2019

Uh oh!

Trott commented Aug 29, 2019

Uh oh!

cclauss commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cclauss 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!

bnoordhuis commented Aug 29, 2019

Uh oh!

cclauss commented Aug 29, 2019

Uh oh!

targos commented Sep 3, 2019

Uh oh!

cclauss left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Sep 10, 2019

Uh oh!

cclauss left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tools: python3 compat for inspector code generator#29340
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:fix29326

bnoordhuis commented Aug 27, 2019 •
edited by cclauss
Loading

cclauss commented Aug 27, 2019 •
edited
Loading

cclauss commented Aug 29, 2019 •
edited
Loading

cclauss left a comment •
edited
Loading