deps: backport ICU-22787 to fix ClangCL on Windows

[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.

This PR aims to do the same thing as #54448, but in the correct way.

NOTE: Since this is a patch to be made unnecessary by the next ICU release, I fixed issues with #include ... in patch files by copying .h files to the directories that need them (where .c files that include them are), rather than fixing everything cleanly, as that would require bigger changes in configure.py for a feature that was last used in 2019.

cc @richardlau

[nodejs-github-bot]

Review requested:

• @nodejs/gyp

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[StefanStojanovic]

The only failure is on SmartOS, and the reason is ld: fatal: symbol 'main' is multiply-defined. It is most likely related to the changes I introduced in the second commit. Since I have no experience with that platform, can someone from the @nodejs/platform-smartos please take a look and help me with this.

The ICU patching hasn't been used recently (and will probably not be used a lot in the future) and if fixing this for SmartOS proves to be hard, I'd probably revisit my initial approach and try to land it.

[bahamat]

The only failure is on SmartOS, and the reason is ld: fatal: symbol 'main' is multiply-defined.

@StefanStojanovic We'll take a look at it.

[danmcd]

The only failure is on SmartOS, and the reason is ld: fatal: symbol 'main' is multiply-defined.

@StefanStojanovic We'll take a look at it.

I saw an odd use of extern for one program's main. I pointed it as a code comment.

And in general, if you see a multiply-defined error, check every .o file you generate with: nm $OFILE | grep -w main and see if more than one shows GLOB as its scope. That'll help debug things. Some more context (what was being compiled; I see there are multiple program binaries getting generated), surrounding the multiply-defined error would be helpful.

[StefanStojanovic]

I saw an odd use of extern for one program's main. I pointed it as a code comment.

I do not see this comment, but I assume you're talking about the main function in genccode.c as that's the only one I see with extern. It is still unclear to me why would it cause problems only on SmartOS.

And in general, if you see a multiply-defined error, check every .o file you generate with: nm $OFILE | grep -w main and see if more than one shows GLOB as its scope. That'll help debug things. Some more context (what was being compiled; I see there are multiple program binaries getting generated), surrounding the multiply-defined error would be helpful.

Thanks for the clarification. As I said I do not have access to SmartOS and this build only fails there (other platforms are fine), so I couldn't debug it, or run any additional commands eg. nm. This is probably related to the way ICU patches are applied when made this way, because if I change code in deps/icu-small directly, SmartOS passes build, like all of the other platforms as seen in my other PR.

[tsoome]

The only failure is on SmartOS, and the reason is ld: fatal: symbol 'main' is multiply-defined. It is most likely related to the changes I introduced in the second commit. Since I have no experience with that platform, can someone from the @nodejs/platform-smartos please take a look and help me with this.

The ICU patching hasn't been used recently (and will probably not be used a lot in the future) and if fixing this for SmartOS proves to be hard, I'd probably revisit my initial approach and try to land it.

The multiply defined error is this:

(file /home/tsoome/node/out/Release/obj.target/genccode/tools/icu/patches/75/source/tools/genccode/genccode.o type=FUNC; file /home/tsoome/node/out/Release/obj.target/tools/icu/libicutools.a(pkgdata.o) type=FUNC);

So, it happens because both object files have symbol main, and indeed, this patch did add one into genccode.o. This error depends on compiler version used for linking, the check was introduced in gcc 10 (here, in my example I have gcc 14). The fix is to make sure there is no duplicate version (or you can suppress the warning, but then the question would be, which symbol is actually used to link the binary;)

[srl295]

Generally LGTM: it could be helpful to put a comment in the header of the patched files.

But you need to completely backout the .gyp changes relating to pkgdata. We setup node.js to not need pkgdata (and that's a good thing). Please don't re-add it, especially not adding the tool's main to the utility library. I've made requested changes.

[srl295]

No, pkgdata is a tool, it may not be part of this library.

Suggested change

'<@(icu_src_pkgdata)',

[srl295]

Suggested change

'pkgdata': 'tools/pkgdata',

maybe unneeded

[srl295]

pkgdata is not used by Node.js. It's fine to add this file for completeness, but please don't try to build pkgdata from Node.js (unless you have some other reason for doing so?)

[srl295]

NOTE: Since this is a patch to be made unnecessary by the next ICU release, I fixed issues with #include ... in patch files by copying .h files to the directories that need them (where .c files that include them are), rather than fixing everything cleanly, as that would require bigger changes in configure.py for a feature that was last used in 2019.

Let's not disparage a feature last used in 2019. (Edit: I think part of why it's not used as much is because people are doing a better job of upstreaming changes to ICU ahead of time. Also because of v8/icu/ecma402 changes, it's very likely that we could have critical things to patch with a v8 update when expectations change. )

Can you give an example of what you are talking about with the .h and .c files because I'm not following?

[nodejs-github-bot]

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

[bahamat]

@StefanStojanovic The GitHub status check for SmartOS is passing now. So it looks like our part, at least, is done. Let me know if there's anything else you need from me on this issue.

Thanks for getting us involved to help out.

[StefanStojanovic]

@StefanStojanovic The GitHub status check for SmartOS is passing now. So it looks like our part, at least, is done. Let me know if there's anything else you need from me on this issue.

Thanks for getting us involved to help out.

I apologize for forgetting this, but big thanks to the @nodejs/platform-smartos team and everyone else who helped as well for looking into the problem. As it turns out only SmartOS reported something that should be caught thus helping in debugging this issue.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/54502
✔  Done loading data for nodejs/node/pull/54502
----------------------------------- PR info ------------------------------------
Title      deps: backport ICU-22787 to fix ClangCL on Windows (#54502)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     StefanStojanovic:mefi-icu-2 -> nodejs:main
Labels     windows, build, tools, i18n-api, needs-ci, dependencies, icu
Commits    2
 - deps: backport ICU-22787 to fix ClangCL on Windows
 - build: add required ICU patch changes
Committers 1
 - StefanStojanovic <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54502
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54502
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 22 Aug 2024 16:48:15 GMT
   ✔  Approvals: 2
   ✔  - Steven R Loomis (@srl295): https://github.com/nodejs/node/pull/54502#pullrequestreview-2264008268
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/54502#pullrequestreview-2264011747
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-27T08:22:34Z: https://ci.nodejs.org/job/node-test-pull-request/61530/
- Querying data for job/node-test-pull-request/61530/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 54502
From https://github.com/nodejs/node
 * branch                  refs/pull/54502/merge -> FETCH_HEAD
✔  Fetched commits as d813634424fb..b2555ae36158
--------------------------------------------------------------------------------
[main 77dc502056] deps: backport ICU-22787 to fix ClangCL on Windows
 Author: StefanStojanovic <[email protected]>
 Date: Wed Aug 21 13:38:41 2024 +0200
 5 files changed, 4906 insertions(+)
 create mode 100644 tools/icu/patches/75/source/common/unicode/platform.h
 create mode 100644 tools/icu/patches/75/source/tools/genccode/genccode.c
 create mode 100644 tools/icu/patches/75/source/tools/pkgdata/pkgdata.cpp
 create mode 100644 tools/icu/patches/75/source/tools/toolutil/pkg_genc.cpp
 create mode 100644 tools/icu/patches/75/source/tools/toolutil/pkg_genc.h
[main ead39e8b86] build: add required ICU patch changes
 Author: StefanStojanovic <[email protected]>
 Date: Thu Aug 22 15:18:06 2024 +0200
 1 file changed, 111 insertions(+)
 create mode 100644 tools/icu/patches/75/source/tools/genccode/pkg_genc.h
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

deps: backport ICU-22787 to fix ClangCL on Windows

Floating patch for ICU 75.x

ICU Bug: https://unicode-org.atlassian.net/browse/ICU-22787

Backport of: unicode-org/icu#3023
PR-URL: #54502

Reviewed-By: Steven R Loomis <[email protected]>

Reviewed-By: Richard Lau <[email protected]>
[detached HEAD 44e735b145] deps: backport ICU-22787 to fix ClangCL on Windows

Author: StefanStojanovic <[email protected]>

Date: Wed Aug 21 13:38:41 2024 +0200

5 files changed, 4906 insertions(+)

create mode 100644 tools/icu/patches/75/source/common/unicode/platform.h

create mode 100644 tools/icu/patches/75/source/tools/genccode/genccode.c

create mode 100644 tools/icu/patches/75/source/tools/pkgdata/pkgdata.cpp

create mode 100644 tools/icu/patches/75/source/tools/toolutil/pkg_genc.cpp

create mode 100644 tools/icu/patches/75/source/tools/toolutil/pkg_genc.h

Rebasing (3/4)

Rebasing (4/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

build: add required ICU patch changes
Copied .h file to a patched folder to fix include issues.
PR-URL: #54502

Reviewed-By: Steven R Loomis <[email protected]>

Reviewed-By: Richard Lau <[email protected]>
[detached HEAD fd9ac6f4c2] build: add required ICU patch changes

Author: StefanStojanovic <[email protected]>

Date: Thu Aug 22 15:18:06 2024 +0200

1 file changed, 111 insertions(+)

create mode 100644 tools/icu/patches/75/source/tools/genccode/pkg_genc.h
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/10583839162

[nodejs-github-bot]

Landed in ebaabf6