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: allow icutrim.py to run on python2 #46263

Merged
merged 2 commits into from
Jan 21, 2023

Conversation

mhdawson
Copy link
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

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 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
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

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.

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'))
Copy link
Member

Choose a reason for hiding this comment

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

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

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

@mhdawson
Copy link
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
Copy link
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
Copy link
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.

tools/icu/icutrim.py Outdated Show resolved Hide resolved
@mhdawson
Copy link
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
Copy link
Member Author

Passed on my local run.

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 20, 2023
@github-actions 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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca 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 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 nodejs-github-bot merged commit 7ea2fc8 into nodejs:main Jan 21, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 7ea2fc8

ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
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 ruyadorno mentioned this pull request Feb 1, 2023
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
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 juanarbol mentioned this pull request Mar 3, 2023
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
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>
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

post merge LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants