Skip to content

deps: V8: fix Temporal with system-icu - #65992

Open
srl295 wants to merge 2 commits into
nodejs:mainfrom
srl295:temporal-issue-62676
Open

srl295 wants to merge 2 commits into
nodejs:mainfrom
srl295:temporal-issue-62676

Conversation

@srl295

@srl295 srl295 commented Sep 11, 2026

Copy link
Copy Markdown
Member

Fixes: #62676

Original commit message:

build: Fix Temporal compilation with system or no ICU

- remove reference to internal udatamem.h header file

See: https://unicode-org.atlassian.net/browse/ICU-23400
See: Node: https://github.com/nodejs/node/issues/62676
See: V8: https://chromium-review.googlesource.com/c/v8/v8/+/8281487

Bug: 8281487

Refs: srl295/v8@cd89fe6

See: https://unicode-org.atlassian.net/browse/ICU-23400
See: Node: #62676
See: V8: https://chromium-review.googlesource.com/c/v8/v8/+/8281487

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Sep 11, 2026
@srl295 srl295 self-assigned this Sep 11, 2026
@srl295 srl295 added wip Issues and PRs that are still a work in progress. deprecations Issues and PRs related to deprecations. and removed deprecations Issues and PRs related to deprecations. labels Sep 11, 2026
@srl295
srl295 requested a review from aduh95 September 11, 2026 22:07
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@legendecas legendecas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This check can be removed (in a new commit):

node/configure.py

Lines 2264 to 2266 in 6a7f94c

case 'system-icu':
warn('Temporal support disabled when compiling with a shared ICU library')
options.v8_disable_temporal_support = True

@srl295

srl295 commented Sep 17, 2026

Copy link
Copy Markdown
Member Author

This check can be removed (in a new commit):

node/configure.py

Lines 2264 to 2266 in 6a7f94c

case 'system-icu':
warn('Temporal support disabled when compiling with a shared ICU library')
options.v8_disable_temporal_support = True

Good catch. Thanks.

I will be setting a new v8 option there anyway, so makes sense. Hope to have a new patch tomorrow.

@legendecas

Copy link
Copy Markdown
Member

Thank you, I verified this locally and node works with a brew provided icu4c shared library.

@aduh95

aduh95 commented Sep 18, 2026

Copy link
Copy Markdown
Contributor
ICU Builds test-temporal-with-zoneinfo
No ICU (--without-intl) 1
Shared (--with=intl=system-icu)
Small ICU (--with-intl=small-icu) 2
Full ICU (default)

Footnotes

  1. requires some manual edits

    As described in #62676 (comment), the make_temporal_zoneinfo_cpp GN rule has not been integrated in our GYP files:

    python deps/v8/tools/include-file-as-bytes.py deps/crates/vendor/zoneinfo64-v0_3/src/data/zoneinfo64.res deps/v8/src/objects/builtins-temporal-zoneinfo64-data.cc zoneinfo64_static_data
    echo '#include "builtins-temporal-zoneinfo64-data.cc"' >> deps/v8/src/objects/js-temporal-zoneinfo64.cc
    rm -r out && nix-shell --pure --arg icu null --arg withTemporal true --arg devTools '[]' --arg benchmarkTools '[]' --run 'make build-ci -j16' && ./node -e 'assert.strictEqual(Temporal.Instant.from("1969-07-20T20:17:00Z").toString(), "1969-07-20T20:17:00Z")'
    
  2. Temporal error: Internal error: Failed to load timezone info.
    $ rm -r out && nix-shell --pure --argstr icu small --arg withTemporal true --arg devTools '[]' --arg benchmarkTools '[]' --run 'make build-ci -j16' && ./node -e 'assert.strictEqual(Temporal.Instant.from("1969-07-20T20:17:00Z").toString(), "1969-07-20T20:17:00Z")'
    
    assert.strictEqual(Temporal.Instant.from("1969-07-20T20:17:00Z").toString(), "1969-07-20T20:17:00Z")
                                                                     ^
    
    Error: Temporal error: Internal error: Failed to load timezone info.
        at Instant.toString (<anonymous>)
        at [eval]:1:66
        at runScriptInThisContext (node:internal/vm:219:10)
        at node:internal/process/execution:485:12
        at [eval]-wrapper:6:24
        at runScriptInContext (node:internal/process/execution:483:60)
        at evalFunction (node:internal/process/execution:317:30)
        at evalTypeScript (node:internal/process/execution:329:3)
        at node:internal/main/eval_string:71:3
    
    Node.js v27.0.0-pre
    

@srl295
srl295 force-pushed the temporal-issue-62676 branch from 8ebe149 to 57f5004 Compare September 19, 2026 02:51
srl295 added a commit to srl295/node that referenced this pull request Sep 19, 2026
Original commit message:

    build: Fix Temporal compilation with system or no ICU

    Add new build flag, v8_enable_temporal_systemicu, off by default
    When on, it will remove references to the internal ICU
    udatamem.h header file

    See: https://unicode-org.atlassian.net/browse/ICU-23400
    See: Node: nodejs#62676
    See: V8: https://chromium-review.googlesource.com/c/v8/v8/+/8281487

    Bug: 8281487

Refs: v8/v8@67771dd
PR-URL: nodejs#65992
Refs: nodejs#62676
@srl295

srl295 commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

@aduh95 @legendecas somehow the v8 configuration isn't getting propagated… can you look at my 2nd commit and see what i'm doing wrong?

@legendecas

Copy link
Copy Markdown
Member

node build does not use GN (it's used by electron). I think you have added the flag declaration in the features.gypi, could you also add a definition like the following definition as well?

['v8_enable_temporal_support==1', {
'defines': ['V8_TEMPORAL_SUPPORT',],
}],

like:

      ['v8_enable_temporal_systemicu==1', {
        'defines': ['V8_ENABLE_TEMPORAL_SYSTEMICU',],
      }],

@srl295

srl295 commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

node build does not use GN (it's used by electron). I think you have added the flag declaration in the features.gypi, could you also add a definition like the following definition as well?

['v8_enable_temporal_support==1', {
'defines': ['V8_TEMPORAL_SUPPORT',],
}],

like:

      ['v8_enable_temporal_systemicu==1', {
        'defines': ['V8_ENABLE_TEMPORAL_SYSTEMICU',],
      }],

Thanks, i will try that.

I assumed that the settings within v8 would automatically do that. In a sense we don't need v8_enable_temporal_systemicu but just set V8_ENABLE_TEMPORAL_SYSTEMICU but it's probably good to allow this fix to be reachable by other consumers of v8 (and maybe v8 itself eventually)

@srl295
srl295 force-pushed the temporal-issue-62676 branch from 57f5004 to 76dcac9 Compare September 19, 2026 16:42
@srl295
srl295 requested a review from legendecas September 19, 2026 17:42
@srl295

srl295 commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

OK, this worked for me also!

@legendecas

Copy link
Copy Markdown
Member

LGTM, please feel free to mark the PR as ready for review if you are ready.

@legendecas

Copy link
Copy Markdown
Member

Though, we should wait https://chromium-review.googlesource.com/c/v8/v8/+/8281487 to land first.

@srl295
srl295 marked this pull request as ready for review September 20, 2026 03:05
@legendecas

Copy link
Copy Markdown
Member

Original commit message:

    Fix Temporal compilation with system or no ICU

    build: Fix Temporal compilation with system or no ICU

    Add new build flag, v8_enable_temporal_systemicu, off by default When
    on, it will remove references to the internal ICU udatamem.h header file

    Obviates CLs 7769990 / 7831715

    See: https://unicode-org.atlassian.net/browse/ICU-23400 See: Node:
    nodejs#62676 See: V8:
    https://chromium-review.googlesource.com/c/v8/v8/+/7831715 See:
    Homebrew: Homebrew/homebrew-core#281707
    Change-Id: Ibc460c68189d32e154c53eddfcda56bc7bca4fc6

    Also includes:

    fix: fix for no-icu build

    thanks @aduh95
    nodejs#62676 (comment)

    NO_IFTTT=No Temporal via Bazel

    Change-Id: Ibc460c68189d32e154c53eddfcda56bc7bca4fc6
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8281487
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Commit-Queue: Chengzhong Wu <cwu631@bloomberg.net>
    Reviewed-by: Olivier Flückiger <olivf@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#110039}

Refs: v8/v8@9fb4444
@aduh95
aduh95 force-pushed the temporal-issue-62676 branch from 56f8157 to bdf6a65 Compare September 23, 2026 16:19
aduh95 pushed a commit to srl295/node that referenced this pull request Sep 23, 2026
…tem-icu

- also update configure.py to no longer disable Temporal!

PR-URL: nodejs#65992
srl295 added a commit to srl295/node that referenced this pull request Sep 23, 2026
…tem-icu

- also update configure.py to no longer disable Temporal!

PR-URL: nodejs#65992
Signed-off-by: Steven R. Loomis <srl295@gmail.com>
@srl295
srl295 force-pushed the temporal-issue-62676 branch from bdf6a65 to fd58427 Compare September 23, 2026 16:30
- switch needed to enable Temporal with system-icu
- also update configure.py to no longer disable Temporal

Signed-off-by: Steven R. Loomis <srl295@gmail.com>
@srl295
srl295 force-pushed the temporal-issue-62676 branch from fd58427 to 0f3a541 Compare September 23, 2026 16:35
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. labels Sep 23, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 23, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95 aduh95 changed the title deps: V8: hot-patch fix for Temporal cherry-pick cd89fe6f7acb deps: V8: fix Temporal with system-icu Sep 23, 2026
@srl295

srl295 commented Sep 23, 2026

Copy link
Copy Markdown
Member Author

FWIW, https://github.com/nodejs/node-core-utils/blob/main/docs/git-node.md#git-node-v8-backport-sha would auto handle the commit message.

That's what i used。

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@srl295

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as duplicate.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@srl295 srl295 added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 24, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 24, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Commit Queue failed

   ✖  GitHub CI is still running
   ✖  Last Jenkins CI still running

The pull request was removed from the Commit Queue and labeled commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. . After resolving the failure, remove that label and add commit-queue PRs queued for automated landing through the Commit Queue. to retry.

Full Commit Queue output
�[36m⠋�[39m Loading data for nodejs/node/pull/65992
�[36m⠋�[39m Loading data for nodejs/node/pull/65992
�[36m⠋�[39m Getting collaborator contacts from README of nodejs/node
�[36m⠋�[39m Getting PR from nodejs/node/pull/65992
�[36m⠋�[39m Getting reviews from nodejs/node/pull/65992
�[36m⠋�[39m Getting comments from nodejs/node/pull/65992
�[36m⠋�[39m Getting commits from nodejs/node/pull/65992
✔  Done loading data for nodejs/node/pull/65992
----------------------------------- PR info ------------------------------------
Title      deps: V8: fix Temporal with system-icu (#65992)
Author     Steven R. Loomis <srl295@gmail.com> (@srl295)
Branch     srl295:temporal-issue-62676 -> nodejs:main
Labels     build, v8 engine, author ready, needs-ci, commit-queue, commit-queue-rebase, dont-land-on-v22.x, dont-land-on-v24.x
Commits    2
 - deps: V8: backport 9fb4444bfc46
 - build: add `v8_enable_temporal_systemicu`
Committers 2
 - Antoine du Hamel <duhamelantoine1995@gmail.com>
 - Steven R. Loomis <srl295@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/65992
Fixes: https://github.com/nodejs/node/issues/62676
Refs: https://github.com/srl295/v8/commit/cd89fe6f7acb6f5d069f6cc741fa68303214e98f
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <richard.lau@ibm.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/65992
Fixes: https://github.com/nodejs/node/issues/62676
Refs: https://github.com/srl295/v8/commit/cd89fe6f7acb6f5d069f6cc741fa68303214e98f
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <richard.lau@ibm.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 11 Sep 2026 22:00:10 GMT
   ✔  Approvals: 4
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/65992#pullrequestreview-5293824414
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/65992#pullrequestreview-5294431525
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/65992#pullrequestreview-5294753838
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/65992#pullrequestreview-5295011082
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2026-09-23T21:54:35Z: https://ci.nodejs.org/job/node-test-pull-request/77814/
�[36m⠙�[39m Querying data for job/node-test-pull-request/77814/
�[36m⠙�[39m Querying data for job/node-test-pull-request/77814/
�[36m⠙�[39m Querying API for job/node-test-pull-request/77814/
✔  Build data downloaded
   ✖  Last Jenkins CI still running
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

View workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. build Issues and PRs related to Node.js builds or CI infrastructure. commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Temporal does not compile with shared ICU or no ICU

6 participants