FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v14.x] Backport Python 3.10 compatibility patches by richardlau · Pull Request #40689 · nodejs/node · GitHub

/ node Public

[v14.x] Backport Python 3.10 compatibility patches - #40689

Closed
richardlau wants to merge 3 commits into
nodejs:v14.x-stagingfrom
richardlau:v14.x-py3.10
ConversationCommits (3)ChecksFiles changed
Closed

[v14.x] Backport Python 3.10 compatibility patches#40689
richardlau wants to merge 3 commits into
nodejs:v14.x-stagingfrom
richardlau:v14.x-py3.10

Conversation

richardlau commented Nov 1, 2021

Copy link
Copy Markdown
Member

Backport of #40296.

Required to fix the Windows CI with v14.x-staging. Conflicts were due to v14.x supporting fallback to Python 2 if Python 3 is not available.

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v14.x v8 engine Issues and PRs related to the V8 dependency. labels Nov 1, 2021

This comment has been minimized.

Sign in to view

richardlau commented Nov 1, 2021

Copy link
Copy Markdown
Member Author

Apparently the jinja patches will need work to remain compatible with Python 2.
e.g. https://ci.nodejs.org/job/node-test-commit-linux/nodes=fedora-latest-x64/43511/console

11:51:43 Traceback (most recent call last):
11:51:43   File "../../deps/v8/third_party/inspector_protocol/code_generator.py", line 718, in <module>
11:51:43     main()
11:51:43   File "../../deps/v8/third_party/inspector_protocol/code_generator.py", line 601, in main
11:51:43     jinja_env = initialize_jinja_env(jinja_dir, config.protocol.output, config)
11:51:43   File "../../deps/v8/third_party/inspector_protocol/code_generator.py", line 190, in initialize_jinja_env
11:51:43     import jinja2
11:51:43   File "/home/iojs/build/workspace/node-test-commit-linux/deps/v8/third_party/jinja2/__init__.py", line 33, in <module>
11:51:43     from jinja2.environment import Environment, Template
11:51:43   File "/home/iojs/build/workspace/node-test-commit-linux/deps/v8/third_party/jinja2/environment.py", line 16, in <module>
11:51:43     from jinja2.defaults import BLOCK_START_STRING, \
11:51:43   File "/home/iojs/build/workspace/node-test-commit-linux/deps/v8/third_party/jinja2/defaults.py", line 32, in <module>
11:51:43     from jinja2.tests import TESTS as DEFAULT_TESTS
11:51:43   File "/home/iojs/build/workspace/node-test-commit-linux/deps/v8/third_party/jinja2/tests.py", line 13, in <module>
11:51:43     from collections.abc import Mapping
11:51:43 ImportError: No module named abc
11:51:43 make[2]: *** [tools/v8_gypfiles/v8_base_without_compiler.target.mk:30:

targos and others added 3 commits November 1, 2021 08:15
deps: V8: patch jinja2 for Python 3.10 compat
80b7c4a 
PR-URL: nodejs#40296
Fixes: nodejs#40294
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
tools: patch jinja2 for Python 3.10 compat
1007a6e 
PR-URL: nodejs#40296
Fixes: nodejs#40294
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
build: support Python 3.10.0
31c42ae 
PR-URL: nodejs#40296
Fixes: nodejs#40294
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot commented Nov 1, 2021 •
edited by richardlau
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40615/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/4364/

richardlau commented Nov 1, 2021

Copy link
Copy Markdown
Member Author

V8 builds are failing with
e.g. https://ci.nodejs.org/job/node-test-commit-v8-linux/4364/nodes=rhel7-s390x,v8test=v8test/console

12:32:18 ERROR at //testing/gmock/BUILD.gn:11:1: Dependency not allowed.
12:32:18 source_set("gmock") {
12:32:18 ^--------------------
12:32:18 The item //testing/gmock:gmock
12:32:18 can not depend on //third_party/googletest:gtest_config
12:32:18 because it is not in //third_party/googletest:gtest_config's visibility list: [
12:32:18   //third_party/googletest:*
12:32:18 ]
12:32:18 
12:32:18 make: *** [Makefile:272: v8] Error 1

I've tried runs on v14.x-staging and v14.x (e.g. without this PR) and the same failure is present 😞.
v14.x-staging: https://ci.nodejs.org/job/node-test-commit-v8-linux/4365/nodes=rhel7-s390x,v8test=v8test/console
v14.x: https://ci.nodejs.org/job/node-test-commit-v8-linux/4366/nodes=rhel7-s390x,v8test=v8test/console

targos commented Nov 1, 2021

Copy link
Copy Markdown
Member

Didn't we fix these errors already?

targos commented Nov 1, 2021

Copy link
Copy Markdown
Member

Or could it be a problem with the GN version that's in CI?

targos commented Nov 1, 2021

Copy link
Copy Markdown
Member

I think the last time V8 CI was executed for v14.x is in #39990: https://ci.nodejs.org/job/node-test-commit-v8-linux/4257/

richardlau commented Nov 1, 2021

Copy link
Copy Markdown
Member Author

Or could it be a problem with the GN version that's in CI?

Yes, most probably by the looks of things. It looks like s390x is the only platform failing this way. When compared to ppc64_le, s390x is on a newer version of gn:

$ ansible -m shell -a "/home/iojs/build-tools/gn --version" test-ibm-rhel7-s390x-?
test-ibm-rhel7-s390x-3 | CHANGED | rc=0 >>
1938 (0153d36)
test-ibm-rhel7-s390x-2 | CHANGED | rc=0 >>
1938 (0153d36)
test-ibm-rhel7-s390x-4 | CHANGED | rc=0 >>
1938 (0153d36)
test-ibm-rhel7-s390x-1 | CHANGED | rc=0 >>
1938 (0153d36)
$ ansible  -m shell -a "/home/iojs/build-tools/gn --version" test-osuosl-centos7-ppc64_le-?
test-osuosl-centos7-ppc64_le-4 | CHANGED | rc=0 >>
1930 (c0a2d23)
test-osuosl-centos7-ppc64_le-3 | CHANGED | rc=0 >>
1930 (c0a2d23)
test-osuosl-centos7-ppc64_le-1 | CHANGED | rc=0 >>
1930 (c0a2d23)
test-osuosl-centos7-ppc64_le-2 | CHANGED | rc=0 >>
1930 (c0a2d23)
$

Looking at the gn commit history

0153d36 Corrects the generation of .stamp file names. by Filip Filmar · 5 weeks ago
de86ec4 Escape spaces in "libs" by Nico Weber · 7 weeks ago
07e2e1b Fix typos in README.md by Ted Pudlik · 7 weeks ago
46b572c Fix configs visibility not always working by Tomasz Śniatowski · 8 weeks ago
69ec4fc C++ modernization improvements. by Brett Wilson · 3 months ago
32aa9d9 Minor style updates. by Brett Wilson · 3 months ago
45aa842 Enable GN to build & run on z/OS by Gaby Baghdadi · 6 months ago
eea3906 Fix a typo in the doc of "generated_file" by Dangyi Liu · 3 months ago
c0a2d23 Update doc/help to say --ninja-executable works for VS Flags by Shezan Baig · 4 months ago

https://gn.googlesource.com/gn/+/46b572ce4ceedfe57f4f84051bd7da624c98bf01 ("Fix configs visibility not always working") sounds like a plausible cause -- that is the newer gn is enforcing visibility rules that were previously not working. I'll try rolling back gn on the s390x machines.

richardlau commented Nov 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/4367/ (✔️)

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

richardlau commented Nov 1, 2021

Copy link
Copy Markdown
Member Author

Looks like Windows CI is timing out on similar tests to the timeouts on master (refs #40684).

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
This was referenced Nov 2, 2021
Open
Open
Open
This was referenced Nov 5, 2021
Open
Open

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Nov 23, 2021

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Nov 24, 2021
Open
45 tasks

nodejs-github-bot commented Nov 24, 2021 •
edited by richardlau
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41064/ (✔️)

richardlau commented Nov 24, 2021

Copy link
Copy Markdown
Member Author

Landed in 0682370...233279d.

richardlau closed this Nov 24, 2021
richardlau pushed a commit that referenced this pull request Nov 24, 2021
deps: V8: patch jinja2 for Python 3.10 compat
fc517cb 
PR-URL: #40296
Backport-PR-URL: #40689
Fixes: #40294
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit that referenced this pull request Nov 24, 2021
tools: patch jinja2 for Python 3.10 compat
f16d255 
PR-URL: #40296
Backport-PR-URL: #40689
Fixes: #40294
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit that referenced this pull request Nov 24, 2021
build: support Python 3.10.0
233279d 
PR-URL: #40296
Backport-PR-URL: #40689
Fixes: #40294
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau deleted the v14.x-py3.10 branch November 24, 2021 11:53
github-actions Bot mentioned this pull request Nov 25, 2021
Open
38 tasks
richardlau pushed a commit that referenced this pull request Nov 25, 2021
deps: V8: patch jinja2 for Python 3.10 compat
cd7c340 
PR-URL: #40296
Backport-PR-URL: #40689
Fixes: #40294
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit that referenced this pull request Nov 25, 2021
tools: patch jinja2 for Python 3.10 compat
6b7b2bb 
PR-URL: #40296
Backport-PR-URL: #40689
Fixes: #40294
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit that referenced this pull request Nov 25, 2021
build: support Python 3.10.0
da35612 
PR-URL: #40296
Backport-PR-URL: #40689
Fixes: #40294
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This was referenced Nov 26, 2021
Open
Open
richardlau mentioned this pull request Oct 28, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL