-
Notifications
You must be signed in to change notification settings - Fork 30k
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
deps: backport ICU fix for ARM64 Windows #26090
Conversation
Please note that, while I would love this to be this easy, I'm happy to do this another way--like maybe bumping to ICU 64.1 when it comes out. |
I think our builds have a patching mechanism for ICU: https://github.com/nodejs/node/blob/master/tools/icu/README.md cc @srl295 |
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.
do not patch the ICU sources directly, as @richardlau pointed out.
copy the full file to node/tools/icu/patches/63/source/tools/toolutil/pkg_genc.cpp
and then make a change there. Try it out and it should apply both from command line build, and also (importantly) if you start configure --with-intl=full-icu --download=all
so that it re-applies to icu 63 sources.
Wow, node core has a very diverse and idiosyncratic way of floating patches. Who volunteeres to document and automate the validation of those 🥴 |
it's because we build against multiple source ICUs. Anyway, I volunteer to document what I wrote above. |
5e399ca
to
412973c
Compare
@richardlau - Thanks again for a pointer to process docs. @srl295 - Thanks for the description of how to add a patch to that folder and the process for testing it. It was straightforward to make the change. |
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'm going to approve, but a minor comment on the commit message (can be fixed at landing):
- please mention commit
unicode-org/icu@11e538b instead of the ICU PR specifically - please mention the ICU issue
https://unicode-org.atlassian.net/browse/ICU-20382
suggested wording:
ICU Issue: https://unicode-org.atlassian.net/browse/ICU-20382
ICU Commit: unicode-org/icu@11e538b
something like that.
ICU 63 as ingested by Node.js does not quite support ARM64 Windows because its OBJ file IMAGE_FILE_MACHINE_TYPE field logic defaults to x86 instead of Unknown. This change backports the ICU 64.1 fix for this. ICU Issue: https://unicode-org.atlassian.net/browse/ICU-20382 ICU Commit: unicode-org/icu@11e538b
412973c
to
d512fdb
Compare
I updated the commit message as requested so it's less trouble during landing. Also, the CI run scheduled for Linux has been queued for 6 days, so this push will hopefully successfully run that gauntlet. |
Landed in dec3dad. |
ICU 63 as ingested by Node.js does not quite support ARM64 Windows because its OBJ file IMAGE_FILE_MACHINE_TYPE field logic defaults to x86 instead of Unknown. This change backports the ICU 64.1 fix for this. ICU Issue: https://unicode-org.atlassian.net/browse/ICU-20382 ICU Commit: unicode-org/icu@11e538b PR-URL: #26090 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
ICU 63 as ingested by Node.js does not quite support ARM64 Windows because its OBJ file IMAGE_FILE_MACHINE_TYPE field logic defaults to x86 instead of Unknown. This change backports the ICU 64.1 fix for this. ICU Issue: https://unicode-org.atlassian.net/browse/ICU-20382 ICU Commit: unicode-org/icu@11e538b PR-URL: #26090 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
ICU 63 as ingested by Node.js does not quite support ARM64 Windows because its OBJ file IMAGE_FILE_MACHINE_TYPE field logic defaults to x86 instead of Unknown. This change backports the ICU 64.1 fix for this. ICU Issue: https://unicode-org.atlassian.net/browse/ICU-20382 ICU Commit: unicode-org/icu@11e538b PR-URL: #26090 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
For ICU to build as part of Node.js, a change to its object file
IMAGE_FILE_MACHINE_TYPE
logic is needed. This has been upstreamed as unicode-org/icu#412 and should land in ICU v64.1.This change backports this fix to ICU 63 in the Node.js tree.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes