-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
Conversation
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>
There was a problem hiding this 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.
tools/icu/icutrim.py
Outdated
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')) |
There was a problem hiding this comment.
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 ([^ ]+).*")
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. |
FWIW the problem this would fix was reported in #40209. |
The change I suggested should fix the issue and makes it more pythonic to boot. I don't see any downsides to landing it. |
Ok @targos pulled in Ben's suggested changing. Just validating locally with a small icu build that all is ok on main. |
Passed on my local run. |
Landed in 7ea2fc8 |
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>
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>
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
post merge LGTM
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