deps: port ICU ClangCL fix

[StefanStojanovic]

This is a change I landed in the ICU at unicode-org/icu#3023. It is needed to enable ClangCL compilation for Node.js on Windows and that's why I'm porting it manually now before the next ICU update. After this PR lands, I'll open a follow-up one changing the icu-generic.gyp to take advantage of the ICU changes.

cc @targos @nodejs/platform-windows

[nodejs-github-bot]

Review requested:

• @nodejs/security-wg

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[RafaelGSS]

LGTM on green CI.

[nodejs-github-bot]

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

[richardlau]

This isn't how we patch ICU. See https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-icu.md#floating-patches-to-icu and https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-icu.md#why-not-just-modify-the-icu-source-directly

[StefanStojanovic]

This isn't how we patch ICU. See https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-icu.md#floating-patches-to-icu and https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-icu.md#why-not-just-modify-the-icu-source-directly

I wasn't aware of this... Thanks (almost landed it, but luckily didn't), I'll adapt my changes to be according to the practice described.

[StefanStojanovic]

@richardlau regarding your previous comment on how ICU patches should be landed I have a proposal:

TLDR - I'd like to land it like this as doing it as you suggested is not straightforward

In more detail, I did what you suggested in my other PR. It also required changes in configure.py and icu-generic.gyp, but it doesn't compile on SmartOS because of a linking error (all other platforms pass). I assume that changes in the configuration phase are responsible for this as this patch covers folders never before used in an ICU patch. On the other hand, ICU patching hasn't been used in a very long time so I do not think investing time into making the patching better is worth it in the end. Also, my changes in the ICU are planned for v76.1, and that is the next major release. One more thing I'd like to add is that this is blocking us from moving forward with the ClangCL enablement process and V8 after v12.8, the version we are at currently, will no longer support MSVC. WIth all this taken into consideration, I'd like to ask to land this PR.

[targos]

Maybe we should just wait for ICU 76 to be released? @srl295 Do you know if it's going to happen soon?

[srl295]

Maybe we should just wait for ICU 76 to be released? @srl295 Do you know if it's going to happen soon?

The public page with the schedule is here, plan is October.

TLDR - I'd like to land it like this as doing it as you suggested is not straightforward

-1 from me on this, but let's keep talking.

In more detail, I did what you suggested in my #54502

I've been out so missed this, but I'll take a look.

It also required changes in configure.py and icu-generic.gyp, but it doesn't compile on SmartOS because of a linking error (all other platforms pass).

OK, let's take a look.

I assume that changes in the configuration phase are responsible for this as this patch covers folders never before used in an ICU patch.

Possibly.

On the other hand, ICU patching hasn't been used in a very long time so I do not think investing time into making the patching better is worth it in the end.

Strong disagree.

As I thought I noted elsewhere, people rebuild Node from their own modified copy of ICU. ICU may release a dot version. The patching machinery means that your changes don't get replaced by users in that situation.

Also, my changes in the ICU are planned for v76.1, and that is the next major release.

Yes, it'll be GA in a couple months (see link above). So waiting for that could be a way forward as well if you prefer.

One more thing I'd like to add is that this is blocking us from moving forward with the ClangCL enablement process and V8 after v12.8, the version we are at currently, will no longer support MSVC. WIth all this taken into consideration, I'd like to ask to land this PR.

I will review the other PR first.

[srl295]

(for the record) deps/icu-small should not be modified. Please close this PR in favor of #54502

[StefanStojanovic]

Closing this one since the other PR landed.