Skip to content

build: fix Windows small-icu build with ClangCL - #64263

Closed
moluopro wants to merge 1 commit into
nodejs:mainfrom
moluopro:fix-win-small-icu-genccode-arch
Closed

build: fix Windows small-icu build with ClangCL#64263
moluopro wants to merge 1 commit into
nodejs:mainfrom
moluopro:fix-win-small-icu-genccode-arch

Conversation

@moluopro

@moluopro moluopro commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

When building the static libnode library with small-icu on Windows, the genccode step fails under ClangCL because the target CPU architecture is not passed to the tool.

The Windows full-ICU branch already passes -c <(target_arch) to genccode when clang==1, while preserving the existing MSVC command. The small-ICU branch did not have the same conditional handling, so it hit ICU's architecture check under ClangCL.

This change makes the Windows small-ICU branch consistent with the full-ICU branch:

  • Pass -c <(target_arch) when clang==1.
  • Keep the existing genccode command unchanged for the non-ClangCL MSVC path.

Reproduction

$env:config_flags = '--v8-disable-temporal-support'
.\vcbuild.bat static nonpm small-icu no-cctest openssl-no-asm x64

Windows builds for Node.js 24+ use ClangCL, and the build fails with:

genccode
genccode: CPU architecture must be set for Clang-CL
Microsoft.CppCommon.targets(254,5): error MSB8066: Custom build for '..\..\deps\icu-tmp\icudt78l.dat;..\..\out\Release\\obj\global_intermediate\icutmp\icudt78l.dat' exited with code 1. [...\tools\icu\icudata.vcxproj]

@moluopro
build,win: fix small-icu build with ClangCL
63c4939 
Signed-off-by: moluopro <moluopro@qq.com>
@nodejs-github-bot

nodejs-github-bot commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added i18n-api Issues and PRs related to Node.js internationalization support. icu Issues and PRs related to the ICU dependency. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Jul 2, 2026
StefanStojanovic
StefanStojanovic approved these changes Jul 3, 2026
View reviewed changes

@StefanStojanovic StefanStojanovic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it locally and it worked.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2026
@nodejs-github-bot

nodejs-github-bot commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

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

@trivikr

trivikr commented Aug 21, 2026

Copy link
Copy Markdown
Member

This needs a rebase, and resolving conflicts in tools/icu/icu-generic.gyp

@moluopro

moluopro commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks, but could you clarify why #65095, which was opened later to address the same issue, was merged before this already-approved PR?

@trivikr

trivikr commented Aug 22, 2026

Copy link
Copy Markdown
Member

#65095, which was opened later to address the same issue, was merged before this already-approved PR?

I looked into #65095. It's possible that collaborators prioritized it's review since a bug report was linked to it?
There can be other reasons. I came across this PR while looking into approved PRs and posted what needs to be done to get this to merged.

@trivikr

trivikr commented Aug 22, 2026

Copy link
Copy Markdown
Member

Closing since it's fixed in #65095

@trivikr trivikr closed this Aug 22, 2026
realthunder added a commit to realthunder/v8-embed-feedstock that referenced this pull request Sep 2, 2026
@realthunder @claude
Windows trial, round 2: the C++ standard for ninja, the CPU for genccode
8f3542f 
Both routes advanced and each hit one thing.

conda-clang-cl (ninja) compiled 500 edges with clang-cl 21.1.8 -- ICU,
the host tools, the builtins library found and on the link line -- and
stopped in v8_libbase at v8config.h: "C++20 or later required." Two
causes, both needed fixing:

- gyp's ninja generator appends CFLAGS, CXXFLAGS and LDFLAGS from the
  environment after its own flags, and conda's clang-cl activation
  exports /std:c++17 in CXXFLAGS, which then outranked whatever V8 said.
  bld.bat now strips /std:, -std and -fuse-ld= from those, and clears
  LDFLAGS: the `-Xlinker /DEFAULTLIB:...` there is clang-driver syntax
  that link.exe answered with LNK4044, and patch 0104 already links the
  builtins library in a form it understands. build.sh strips -std= for
  the same reason.
- Even with a clean environment there was no /std: at all: with clang,
  common.gypi expresses the standard as the MSBuild-only property
  LanguageStandard (stdcpp20), and gyp's ninja emulation has no case for
  it. Patch 0106 adds one, per language -- C files get /std:c11, not
  c++20, which clang-cl would refuse where cl.exe ignores it. Verified
  on Windows ninja files generated here (gyp's ninja-win flavour, with
  its registry probes stubbed): cflags_cc = /TP /std:c++20, cflags_c =
  /std:c11, and the MSVC branch's -std:c++20 gone.

vs-clang-cl (MSBuild) built ICU's tools with Visual Studio's clang-cl
22.1.3 and stopped writing the ICU data object:

  genccode: CPU architecture must be set for Clang-CL

A genccode compiled by clang will not infer the machine type from its
own macros, and node's small-icu action for Windows never passed -c.
Node's Windows builds are full-icu, so nobody hit it until now; the fix
is nodejs/node#64263, after 26.6.0, and patch 0107 is that one line. The
ninja route runs the same action and would have reached it too.

Also caught before it could fail: v8config.h guards on __cplusplus, which
cl.exe reports as 199711L unless given /Zc:__cplusplus. The consumer test
compiles with cl.exe on Windows, so it gets the flag.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KzqRnuUy9dpMik6sxVaKXd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StefanStojanovic StefanStojanovic StefanStojanovic approved these changes

Assignees

No one assigned

Labels

i18n-api Issues and PRs related to Node.js internationalization support. icu Issues and PRs related to the ICU dependency. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@moluopro @nodejs-github-bot @trivikr @StefanStojanovic @richardlau