nodejs / nodejs/Release

Policy for updating ICU in a release line

Open
#679 9 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
4.4k
Forks
675
Avg merge
22h 29m
Merged PRs (30d)
1

Description

Background

Started from https://github.com/nodejs/node/pull/38178#issuecomment-863116223 -- Node.js 14.17.1 was released with a V8 change that breaks compiling against a system ICU 65. We specify a minimum version of ICU in tools/icu/icu_versions.json and this was:

14.17.1 included https://github.com/nodejs/node/pull/38497 to update to ICU 69 but also included a V8 backport (https://github.com/nodejs/node/commit/40df0dcb59c6769eb013d40dc0ce5de66210f3bd) and it's this backport which has broken the compilation with ICU 65 (and probably 66 going by the commit message from the original V8 commit).

We had a brief chat in today's Release WG meeting (which was recorded (link tbc)) and the agreed action was that @richardlau would see if its possible to patch V8 to restore being able to compile against ICU 65 while also still being able to compile against ICU 69. If it's not (or proves too complicated) then we'll revert the ICU 69 update in 14.x.

Policy discussion

While we have a plan for the immediate issue on 14.x in the meeting we wanted to continue to the generalized policy discussion around ICU, which will continue in this issue.

I'd like to advocate a "we won't change the minimum ICU during a major" policy -- there are some considerations.

The minimum ICU is a combination of:

We have a test to ensure consistency for the declared minimum versions. V8 backports complicate things (like in the 14.x case) as they may come from upstream V8 releases where the minimum has been moved on and may introduce incompatible code.

Fixing the minimum ICU version may prevent V8 updates from landing, for example @targos mentioned that the V8 9.2 update will also bump the minimum ICU to 69. If we enact a "no minimum version bump" that might prevent updating Node.js 16 to V8 9.2.

Another possibility/addition to fixing the minimum ICU was to not update ICU at all in a release line. Are there any potential downsides? We'd still be able to update timezone information (documentation on how to do so added in https://github.com/nodejs/node/pull/30364).

For completion there was also some previous discussion relating to not updating ICU in a release line related to the full-icu package (https://github.com/nodejs/Release/issues/483) -- that should be less of a concern for official releases of Node.js 14 onwards as those are compiled with full-icu and don't require the external full-icu npm.

cc @nodejs/releasers @AdamMajer @nodejs/i18n-api

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading tools/icu/icu_versions.json, deps/v8/src/objects/intl-objects.h, and test/parallel/test-icu-minimum-version.js to understand the declared ICU requirements and consistency check. Done would be an agreed, documented policy for ICU updates within release lines, including how V8 backports and timezone updates are handled.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
build-system, release
Issue type
Documentation
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.