Policy for updating ICU in a release line
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:
- 65 in Node.js 14.0.0
- 67 in Node.js 14.6.0 as part of the V8 8.4 update https://github.com/nodejs/node/pull/34356
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:
- What is required by Node.js (specified in
tools/icu/icu_versions.json) - What is required by V8 (specified in
deps/v8/src/objects/intl-objects.h)
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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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