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

tools: allow icutrim.py to run on python2 by mhdawson · Pull Request #46263 · nodejs/node · GitHub

/ node Public

tools: allow icutrim.py to run on python2 - #46263

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
mhdawson:fix-icu-trim
Jan 21, 2023
ConversationCommits (2)ChecksFiles changed
Merged

tools: allow icutrim.py to run on python2#46263
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
mhdawson:fix-icu-trim

Conversation

mhdawson commented Jan 18, 2023

Copy link
Copy Markdown
Member

Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would allow us to simplify future backports as currently the files are the same across lines.

Signed-off-by: Michael Dawson mdawson@devrus.com

tools: allow icutrim.py to run on python2
1714f3d 
Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would
allow us to simplify future backports as currently
the files are the same across lines.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
nodejs-github-bot added i18n-api Issues and PRs related to the i18n implementation. icu Issues and PRs related to the ICU dependency. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Jan 18, 2023
mhdawson mentioned this pull request Jan 18, 2023
Closed
targos suggested changes Jan 19, 2023
View reviewed changes

targos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think we should land changes for Python 2 on the main branch. I'd approve a PR against v14.x-staging though.

bnoordhuis reviewed Jan 19, 2023
View reviewed changes
Comment thread tools/icu/icutrim.py Outdated
Comment on lines +319 to +322
pat = re.compile(bytes(r"^Item ([^ ]+) depends on missing item ([^ ]+).*", 'utf-8'))
if (sys.version_info.major < 3):
pat = re.compile(bytes(r"^Item ([^ ]+) depends on missing item ([^ ]+).*").encode('utf-8'))
else:
pat = re.compile(bytes(r"^Item ([^ ]+) depends on missing item ([^ ]+).*", 'utf-8'))

bnoordhuis Jan 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

python[23]-agnostic, which probably also addresses @targos's objection:

pat = re.compile(br"^Item ([^ ]+) depends on missing item ([^ ]+).*")

mhdawson commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

Richard came up with a suggestion that we just set the test job to build with python 3, given that as an option to get the small-icu testing going on14.x.

Given that 14.x goes EOL in a few months and we've not had complaints about the issue I'm happy to just close this or to submit a PR against 14.x. I'll close unless I hear suggestions we should fix in the 14.x stream.

richardlau commented Jan 19, 2023

Copy link
Copy Markdown
Member

FWIW the problem this would fix was reported in #40209.
I have no objection to landing a Node.js 14 only fix, but I'd be equally okay with not fixing it and leaving as a known issue with a workaround (use Python 3) at this stage in Node.js 14's lifecycle.

bnoordhuis commented Jan 20, 2023

Copy link
Copy Markdown
Member

The change I suggested should fix the issue and makes it more pythonic to boot. I don't see any downsides to landing it.

mhdawson commented Jan 20, 2023
View reviewed changes
Comment thread tools/icu/icutrim.py Outdated

mhdawson commented Jan 20, 2023

Copy link
Copy Markdown
Member Author

Ok @targos pulled in Ben's suggested changing. Just validating locally with a small icu build that all is ok on main.

mhdawson commented Jan 20, 2023

Copy link
Copy Markdown
Member Author

Passed on my local run.

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 20, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 20, 2023

nodejs-github-bot commented Jan 20, 2023

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Jan 21, 2023
Open
14 tasks

nodejs-github-bot commented Jan 21, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 21, 2023

Copy link
Copy Markdown
Collaborator

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

lpinca added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 21, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 21, 2023
nodejs-github-bot merged commit 7ea2fc8 into nodejs:main Jan 21, 2023

nodejs-github-bot commented Jan 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in 7ea2fc8

This was referenced Jan 22, 2023
Open
Open
This was referenced Jan 24, 2023
Open
Open
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
tools: allow icutrim.py to run on python2
e75faff 
Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would
allow us to simplify future backports as currently
the files are the same across lines.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #46263
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno mentioned this pull request Feb 1, 2023
Merged
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
tools: allow icutrim.py to run on python2
56709e8 
Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would
allow us to simplify future backports as currently
the files are the same across lines.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #46263
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol mentioned this pull request Mar 3, 2023
Merged
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
tools: allow icutrim.py to run on python2
60d714e 
Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would
allow us to simplify future backports as currently
the files are the same across lines.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #46263
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
srl295 reviewed Apr 4, 2023
View reviewed changes

srl295 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

post merge LGTM

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

bnoordhuis bnoordhuis left review comments
srl295 srl295 left review comments
jasnell jasnell approved these changes
lpinca lpinca approved these changes
anonrig anonrig approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. i18n-api Issues and PRs related to the i18n implementation. icu Issues and PRs related to the ICU dependency. needs-ci PRs that need a full CI run. 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.

9 participants

Back | FazBrowse Home | New Git URL