deps: backport ICU fix for ARM64 Windows - #26090
Conversation
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
I think our builds have a patching mechanism for ICU: https://github.com/nodejs/node/blob/master/tools/icu/README.md cc @srl295 |
Sorry, something went wrong.
srl295
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
Wow, node core has a very diverse and idiosyncratic way of floating patches. Who volunteeres to document and automate the validation of those 🥴 |
Sorry, something went wrong.
it's because we build against multiple source ICUs. Anyway, I volunteer to document what I wrote above. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
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
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
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