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: python3 compat for inspector code generator #29340

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Aug 27, 2019

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.

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

@targos
Copy link
Member

targos commented Aug 27, 2019

Should we upstream this?

@bnoordhuis
Copy link
Member Author

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

@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Aug 27, 2019
@nodejs-github-bot
Copy link
Collaborator

@bnoordhuis
Copy link
Member Author

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.

@cclauss
Copy link
Contributor

cclauss commented Aug 27, 2019

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)

@bnoordhuis
Copy link
Member Author

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.

@cclauss
Copy link
Contributor

cclauss commented Aug 27, 2019

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

@cclauss
Copy link
Contributor

cclauss commented Aug 27, 2019

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

@richardlau
Copy link
Member

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.

@Trott
Copy link
Member

Trott commented Aug 29, 2019

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?

@cclauss
Copy link
Contributor

cclauss commented Aug 29, 2019

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.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

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

@bnoordhuis
Copy link
Member Author

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

@cclauss
Copy link
Contributor

cclauss commented Aug 29, 2019

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

@targos
Copy link
Member

targos commented Sep 3, 2019

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.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

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

@bnoordhuis
Copy link
Member Author

@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
Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

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

@sam-github sam-github mentioned this pull request Sep 16, 2019
4 tasks
sam-github added a commit to sam-github/node that referenced this pull request Sep 16, 2019
cclauss pushed a commit that referenced this pull request Sep 18, 2019
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>
targos pushed a commit that referenced this pull request Sep 20, 2019
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>
targos pushed a commit that referenced this pull request Sep 20, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
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>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
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>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Sep 27, 2019
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Oct 7, 2019
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>
targos pushed a commit that referenced this pull request Oct 8, 2019
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>
targos pushed a commit to targos/node that referenced this pull request Oct 25, 2019
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>
targos pushed a commit to targos/node that referenced this pull request Dec 5, 2019
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>
MylesBorins pushed a commit to targos/node that referenced this pull request Jan 7, 2020
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>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants