[v20.x] deps: V8: backport build fixes for Xcode 16.3 by Bo98 · Pull Request #58342 · nodejs/node

Bo98 · 2025-05-15T05:09:16Z

Node 20.19.2 currently fails to build from source since Xcode 16.3 (and LLVM Clang 18). This pull request backports three v8 commits that fix the issue:

deps: V8: cherry-pick third_party/zlib@646b7f569718
- The bundled zlib had code that assumed fdopen is a definition if TARGET_OS_MAC is defined, but fdopen has never been a definition in the macOS SDK. The latest Clang compiler now defines TARGET_OS_MAC globally, triggering this code and causing a compile error.
- Cherry-pick of https://chromium.googlesource.com/chromium/src/third_party/zlib/+/646b7f569718921d7d4b5b8e22572ff6c76f2596
- Note that git-node doesn't do backports of third_party repos so I did this one manually
deps: V8: cherry-pick 182d9c05e78b
- std::basic_string<uint16_t> is invalid C++ without a custom std::char_traits. This is now enforced in the latest libc++.
- Cherry-pick of v8/v8@182d9c0
deps: V8: cherry-pick 1a3ecc2483b2
- Cherry-picking the fix above alone (v8/v8@182d9c0) regresses older versions of Xcode. Ensure we don't carry that regression by backporting a change that was already cherry-picked to Node 21 and later.
- Backport of Node commit 4ad3479, which is a cherry-pick of v8/v8@1a3ecc2

All of the above commits are already included in Node 22 and later, hence the 20.x-only PR.

nodejs-github-bot · 2025-05-15T05:09:22Z

Review requested:

@nodejs/gyp
@nodejs/security-wg
@nodejs/v8-update

marco-ippolito · 2025-05-16T13:30:40Z

I confirm this PR fixes the compilation on my machine:

marcoippolito@marcos-MacBook-Pro-3 node % clang -v
Apple clang version 17.0.0 (clang-1700.0.13.3)
Target: arm64-apple-darwin24.4.0
Thread model: posix

Currently the CI for v20.x is broken so errors are unrelated

nodejs-github-bot · 2025-06-04T06:26:38Z

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

nodejs-github-bot · 2025-06-04T06:26:52Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6594/

nodejs-github-bot · 2025-06-04T06:27:43Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6594/

nodejs-github-bot · 2025-06-04T06:27:53Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6594/

nodejs-github-bot · 2025-06-04T07:53:13Z

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

Bo98 · 2025-06-04T17:14:56Z

CI probably needs a cherry-pick of 996a774

nodejs-github-bot · 2025-06-05T05:47:22Z

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

Bo98 · 2025-06-05T06:49:22Z

(and 8287f67 - I initially missed that there's two tests failing)

marco-ippolito · 2025-06-05T06:58:22Z

please rebase from v20.x-staging

Original commit message: [zlib][build] Remove fdopen #defines in zutil.h. The latest version of Clang changed what macros it predefines on Apple targets, causing errors about predefined macros in zlib. See: madler/zlib@4bd9a71 Bug: 1519899 Change-Id: Ie75ef4078f2c86d89ba6c036ddd13e768a40ccbb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5237020 Reviewed-by: Adenilson Cavalcanti <cavalcantii@chromium.org> Commit-Queue: Hans Wennborg <hans@chromium.org> Cr-Commit-Position: refs/heads/main@{#1253252} NOKEYCHECK=True GitOrigin-RevId: 2f39ac8d0a414dd65c0e1d5aae38c8f97aa06ae9 Refs: https://chromium.googlesource.com/chromium/src/third_party/zlib/+/646b7f569718921d7d4b5b8e22572ff6c76f2596

Original commit message: Define UChar as char16_t We used to have UChar defined as uint16_t which does not go along with STL these days if you try to have an std::basic_string<> of it, as there are no standard std::char_traits<> specialization for uint16_t. This switches UChar to char16_t where practical, introducing a few compatibility shims to keep CL size small, as (1) this would likely have to be back-ported and (2) crdtp extensively uses uint16_t for wide chars. Bug: b:296390693 Change-Id: I66a32d8f0050915225b187de56896c26dd76163d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4789966 Reviewed-by: Jaroslav Sevcik <jarin@chromium.org> Commit-Queue: Jaroslav Sevcik <jarin@chromium.org> Auto-Submit: Andrey Kosyakov <caseq@chromium.org> Cr-Commit-Position: refs/heads/main@{#89559} Refs: v8/v8@182d9c0

Original commit message: Fix build issue, remove unneeded include uchar.h. Follow the conversation on: https://groups.google.com/g/v8-dev/c/nsbshwlmP3c. The `uchar.h` include is not necessary. It was added to get the definition of char16_t but that's an intrinsic type in C++. Change-Id: I0aaa11dba0be3ccad15b9e421f8bae71450d443b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4823404 Reviewed-by: Omer Katz <omerkatz@chromium.org> Commit-Queue: Eric Leese <leese@chromium.org> Reviewed-by: Eric Leese <leese@chromium.org> Cr-Commit-Position: refs/heads/main@{#89787} Refs: v8/v8@1a3ecc2 PR-URL: nodejs#49639 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot · 2025-06-05T08:24:51Z

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

nodejs-github-bot · 2025-06-05T08:25:19Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6596/

nodejs-github-bot · 2025-06-05T08:27:20Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6596/

nodejs-github-bot · 2025-06-05T08:29:11Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6596/

marco-ippolito · 2025-06-05T11:42:42Z

Landed in ff210ca7f3f9...7b142892c3ed

Original commit message: [zlib][build] Remove fdopen #defines in zutil.h. The latest version of Clang changed what macros it predefines on Apple targets, causing errors about predefined macros in zlib. See: madler/zlib@4bd9a71 Bug: 1519899 Change-Id: Ie75ef4078f2c86d89ba6c036ddd13e768a40ccbb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5237020 Reviewed-by: Adenilson Cavalcanti <cavalcantii@chromium.org> Commit-Queue: Hans Wennborg <hans@chromium.org> Cr-Commit-Position: refs/heads/main@{#1253252} NOKEYCHECK=True GitOrigin-RevId: 2f39ac8d0a414dd65c0e1d5aae38c8f97aa06ae9 Refs: https://chromium.googlesource.com/chromium/src/third_party/zlib/+/646b7f569718921d7d4b5b8e22572ff6c76f2596 PR-URL: #58342 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Original commit message: Define UChar as char16_t We used to have UChar defined as uint16_t which does not go along with STL these days if you try to have an std::basic_string<> of it, as there are no standard std::char_traits<> specialization for uint16_t. This switches UChar to char16_t where practical, introducing a few compatibility shims to keep CL size small, as (1) this would likely have to be back-ported and (2) crdtp extensively uses uint16_t for wide chars. Bug: b:296390693 Change-Id: I66a32d8f0050915225b187de56896c26dd76163d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4789966 Reviewed-by: Jaroslav Sevcik <jarin@chromium.org> Commit-Queue: Jaroslav Sevcik <jarin@chromium.org> Auto-Submit: Andrey Kosyakov <caseq@chromium.org> Cr-Commit-Position: refs/heads/main@{#89559} Refs: v8/v8@182d9c0 PR-URL: #58342 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Original commit message: Fix build issue, remove unneeded include uchar.h. Follow the conversation on: https://groups.google.com/g/v8-dev/c/nsbshwlmP3c. The `uchar.h` include is not necessary. It was added to get the definition of char16_t but that's an intrinsic type in C++. Change-Id: I0aaa11dba0be3ccad15b9e421f8bae71450d443b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4823404 Reviewed-by: Omer Katz <omerkatz@chromium.org> Commit-Queue: Eric Leese <leese@chromium.org> Reviewed-by: Eric Leese <leese@chromium.org> Cr-Commit-Position: refs/heads/main@{#89787} Refs: v8/v8@1a3ecc2 PR-URL: #49639 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #58342 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Original commit message: [zlib][build] Remove fdopen #defines in zutil.h. The latest version of Clang changed what macros it predefines on Apple targets, causing errors about predefined macros in zlib. See: madler/zlib@4bd9a71 Bug: 1519899 Change-Id: Ie75ef4078f2c86d89ba6c036ddd13e768a40ccbb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5237020 Reviewed-by: Adenilson Cavalcanti <cavalcantii@chromium.org> Commit-Queue: Hans Wennborg <hans@chromium.org> Cr-Commit-Position: refs/heads/main@{#1253252} NOKEYCHECK=True GitOrigin-RevId: 2f39ac8d0a414dd65c0e1d5aae38c8f97aa06ae9 Refs: https://chromium.googlesource.com/chromium/src/third_party/zlib/+/646b7f569718921d7d4b5b8e22572ff6c76f2596 PR-URL: #58342 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Original commit message: Define UChar as char16_t We used to have UChar defined as uint16_t which does not go along with STL these days if you try to have an std::basic_string<> of it, as there are no standard std::char_traits<> specialization for uint16_t. This switches UChar to char16_t where practical, introducing a few compatibility shims to keep CL size small, as (1) this would likely have to be back-ported and (2) crdtp extensively uses uint16_t for wide chars. Bug: b:296390693 Change-Id: I66a32d8f0050915225b187de56896c26dd76163d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4789966 Reviewed-by: Jaroslav Sevcik <jarin@chromium.org> Commit-Queue: Jaroslav Sevcik <jarin@chromium.org> Auto-Submit: Andrey Kosyakov <caseq@chromium.org> Cr-Commit-Position: refs/heads/main@{#89559} Refs: v8/v8@182d9c0 PR-URL: #58342 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Original commit message: Fix build issue, remove unneeded include uchar.h. Follow the conversation on: https://groups.google.com/g/v8-dev/c/nsbshwlmP3c. The `uchar.h` include is not necessary. It was added to get the definition of char16_t but that's an intrinsic type in C++. Change-Id: I0aaa11dba0be3ccad15b9e421f8bae71450d443b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4823404 Reviewed-by: Omer Katz <omerkatz@chromium.org> Commit-Queue: Eric Leese <leese@chromium.org> Reviewed-by: Eric Leese <leese@chromium.org> Cr-Commit-Position: refs/heads/main@{#89787} Refs: v8/v8@1a3ecc2 PR-URL: #49639 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #58342 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Original commit message: [zlib][build] Remove fdopen #defines in zutil.h. The latest version of Clang changed what macros it predefines on Apple targets, causing errors about predefined macros in zlib. See: madler/zlib@4bd9a71 Bug: 1519899 Change-Id: Ie75ef4078f2c86d89ba6c036ddd13e768a40ccbb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5237020 Reviewed-by: Adenilson Cavalcanti <cavalcantii@chromium.org> Commit-Queue: Hans Wennborg <hans@chromium.org> Cr-Commit-Position: refs/heads/main@{#1253252} NOKEYCHECK=True GitOrigin-RevId: 2f39ac8d0a414dd65c0e1d5aae38c8f97aa06ae9 Refs: https://chromium.googlesource.com/chromium/src/third_party/zlib/+/646b7f569718921d7d4b5b8e22572ff6c76f2596 PR-URL: #58342 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Original commit message: Define UChar as char16_t We used to have UChar defined as uint16_t which does not go along with STL these days if you try to have an std::basic_string<> of it, as there are no standard std::char_traits<> specialization for uint16_t. This switches UChar to char16_t where practical, introducing a few compatibility shims to keep CL size small, as (1) this would likely have to be back-ported and (2) crdtp extensively uses uint16_t for wide chars. Bug: b:296390693 Change-Id: I66a32d8f0050915225b187de56896c26dd76163d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4789966 Reviewed-by: Jaroslav Sevcik <jarin@chromium.org> Commit-Queue: Jaroslav Sevcik <jarin@chromium.org> Auto-Submit: Andrey Kosyakov <caseq@chromium.org> Cr-Commit-Position: refs/heads/main@{#89559} Refs: v8/v8@182d9c0 PR-URL: #58342 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Original commit message: Fix build issue, remove unneeded include uchar.h. Follow the conversation on: https://groups.google.com/g/v8-dev/c/nsbshwlmP3c. The `uchar.h` include is not necessary. It was added to get the definition of char16_t but that's an intrinsic type in C++. Change-Id: I0aaa11dba0be3ccad15b9e421f8bae71450d443b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4823404 Reviewed-by: Omer Katz <omerkatz@chromium.org> Commit-Queue: Eric Leese <leese@chromium.org> Reviewed-by: Eric Leese <leese@chromium.org> Cr-Commit-Position: refs/heads/main@{#89787} Refs: v8/v8@1a3ecc2 PR-URL: #49639 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #58342 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v20.x v8 engine Issues and PRs related to the V8 dependency. labels May 15, 2025

marco-ippolito approved these changes May 15, 2025
View reviewed changes

mertcanaltin approved these changes May 15, 2025
View reviewed changes

marco-ippolito force-pushed the v20.x-xcode16.3 branch from a56d782 to 42c8a36Compare June 3, 2025 13:32

marco-ippolito approved these changes Jun 3, 2025
View reviewed changes

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 4, 2025

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 4, 2025

zmodemand others added 3 commits June 5, 2025 08:07

Bo98 force-pushed the v20.x-xcode16.3 branch from 42c8a36 to 7b59afaCompare June 5, 2025 07:07

marco-ippolito approved these changes Jun 5, 2025
View reviewed changes

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 5, 2025

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 5, 2025

marco-ippolito closed this Jun 5, 2025

marco-ippolito mentioned this pull request Jun 5, 2025
v20.19.3 proposal #58588
Merged

Uh oh!

Uh oh!

Conversation

Bo98 commented May 15, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented May 15, 2025

Uh oh!

marco-ippolito commented May 16, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 4, 2025

Uh oh!

nodejs-github-bot commented Jun 4, 2025

Uh oh!

nodejs-github-bot commented Jun 4, 2025

Uh oh!

nodejs-github-bot commented Jun 4, 2025

Uh oh!

nodejs-github-bot commented Jun 4, 2025

Uh oh!

Bo98 commented Jun 4, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 5, 2025

Uh oh!

Bo98 commented Jun 5, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marco-ippolito commented Jun 5, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 5, 2025

Uh oh!

nodejs-github-bot commented Jun 5, 2025

Uh oh!

nodejs-github-bot commented Jun 5, 2025

Uh oh!

nodejs-github-bot commented Jun 5, 2025

Uh oh!

marco-ippolito commented Jun 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Bo98 commented May 15, 2025•
edited
Loading

marco-ippolito commented May 16, 2025•
edited
Loading

Bo98 commented Jun 4, 2025•
edited
Loading

Bo98 commented Jun 5, 2025•
edited
Loading

marco-ippolito commented Jun 5, 2025•
edited
Loading