Skip to content

ICU: backport ICU-23110 (percent formatRange scaled twice on approximately path) #374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 2 commits into
base: main
Choose a base branch
from
Open

ICU: backport ICU-23110 (percent formatRange scaled twice on approximately path) #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9cfb1f5
ICU: backport ICU-23110 (percent formatRange scaled twice on approxim…
robobun Jul 29, 2026
46c558e
Dockerfile.{android,freebsd}: install patch
robobun Jul 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ RUN --mount=type=tmpfs,target=/icu \
tar -xf /icu.tgz --strip-components=1 && \
rm /icu.tgz && \
patch -p1 < /icu-bun/udata-decompress-hook.patch && \
patch -p1 < /icu-bun/icu-23110-numrange-percent.patch && \
cd source && \
./configure --enable-static --disable-shared --disable-layoutex --disable-layout --with-data-packaging=static --disable-samples --disable-debug --disable-tests --disable-extras --disable-icuio && \
make -j$(nproc) && \
Expand Down
7 changes: 5 additions & 2 deletions Dockerfile.android
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ ARG NDK_VERSION

RUN apt-get update && apt-get install -y --no-install-recommends \
wget unzip xz-utils ca-certificates \
cmake ninja-build make git \
cmake ninja-build make git patch \
ruby ruby-getoptlong perl python3 rsync file cpio \
lsb-release software-properties-common gnupg \
&& rm -rf /var/lib/apt/lists/*
Expand Down Expand Up @@ -90,6 +90,7 @@ ARG ICU_VERSION
ARG ICU_RELEASE_TAG
ARG ICU_SHA256
ADD --checksum=sha256:${ICU_SHA256} https://github.com/unicode-org/icu/releases/download/${ICU_RELEASE_TAG}/icu4c-${ICU_VERSION}-src.tgz /icu.tgz
COPY icu/icu-23110-numrange-percent.patch /icu-bun/

# Host build (tools only — no LTO, fast).
RUN mkdir -p /icu-host && cd /icu-host \
Expand All @@ -116,7 +117,9 @@ RUN --mount=type=tmpfs,target=/icu \
export CFLAGS="${CROSS_FLAGS} ${DEFAULT_CFLAGS} ${MARCH_FLAG} -Os -std=c17 ${LTO_FLAG}" && \
export CXXFLAGS="${CROSS_FLAGS} ${DEFAULT_CFLAGS} ${MARCH_FLAG} -Os -std=c++20 -fno-exceptions ${LTO_FLAG} -fno-c++-static-destructors" && \
export LDFLAGS="${CROSS_FLAGS} --rtlib=compiler-rt -fuse-ld=lld" && \
cd /icu && tar -xf /icu.tgz --strip-components=1 && rm /icu.tgz && cd source && \
cd /icu && tar -xf /icu.tgz --strip-components=1 && rm /icu.tgz && \
patch -p1 < /icu-bun/icu-23110-numrange-percent.patch && \
cd source && \
/icu-host/source/bin/icupkg -l data/in/icudt75l.dat | grep -E '\.(cnv|spp|cfu)$|^cnvalias\.icu$|^translit/|^rbnf/|^unames\.icu$' | grep -vE '^rbnf/(root|res_index|ja|zh|zh_Hant)\.res$' > data/in/rm.lst && \
/icu-host/source/bin/icupkg --auto_toc_prefix -r data/in/rm.lst data/in/icudt75l.dat data/in/icudt75l_filtered.dat && \
mv -f data/in/icudt75l_filtered.dat data/in/icudt75l.dat && \
Expand Down
7 changes: 5 additions & 2 deletions Dockerfile.freebsd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ ARG FREEBSD_ARCH

RUN apt-get update && apt-get install -y --no-install-recommends \
wget unzip xz-utils ca-certificates \
cmake ninja-build make git \
cmake ninja-build make git patch \
ruby ruby-getoptlong perl python3 rsync file cpio \
lsb-release software-properties-common gnupg \
&& rm -rf /var/lib/apt/lists/*
Expand Down Expand Up @@ -80,6 +80,7 @@ ARG ICU_VERSION
ARG ICU_RELEASE_TAG
ARG ICU_SHA256
ADD --checksum=sha256:${ICU_SHA256} https://github.com/unicode-org/icu/releases/download/${ICU_RELEASE_TAG}/icu4c-${ICU_VERSION}-src.tgz /icu.tgz
COPY icu/icu-23110-numrange-percent.patch /icu-bun/

# Host build (tools only — no LTO, fast).
RUN mkdir -p /icu-host && cd /icu-host \
Expand All @@ -106,7 +107,9 @@ RUN --mount=type=tmpfs,target=/icu \
export CFLAGS="${CROSS_FLAGS} ${DEFAULT_CFLAGS} ${MARCH_FLAG} -Os -std=c17 ${LTO_FLAG}" && \
export CXXFLAGS="${CROSS_FLAGS} ${DEFAULT_CFLAGS} ${MARCH_FLAG} -Os -std=c++20 -fno-exceptions ${LTO_FLAG} -fno-c++-static-destructors" && \
export LDFLAGS="${CROSS_FLAGS} -fuse-ld=lld" && \
cd /icu && tar -xf /icu.tgz --strip-components=1 && rm /icu.tgz && cd source && \
cd /icu && tar -xf /icu.tgz --strip-components=1 && rm /icu.tgz && \
patch -p1 < /icu-bun/icu-23110-numrange-percent.patch && \
cd source && \
/icu-host/source/bin/icupkg -l data/in/icudt75l.dat | grep -E '\.(cnv|spp|cfu)$|^cnvalias\.icu$|^translit/|^rbnf/|^unames\.icu$' | grep -vE '^rbnf/(root|res_index|ja|zh|zh_Hant)\.res$' > data/in/rm.lst && \
/icu-host/source/bin/icupkg --auto_toc_prefix -r data/in/rm.lst data/in/icudt75l.dat data/in/icudt75l_filtered.dat && \
mv -f data/in/icudt75l_filtered.dat data/in/icudt75l.dat && \
Expand Down
1 change: 1 addition & 0 deletions Dockerfile.musl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ RUN --mount=type=tmpfs,target=/icu \
tar -xf /icu.tgz --strip-components=1 && \
rm /icu.tgz && \
patch -p1 < /icu-bun/udata-decompress-hook.patch && \
patch -p1 < /icu-bun/icu-23110-numrange-percent.patch && \
cd source && \
./configure --enable-static --disable-shared --with-data-packaging=static --disable-samples --disable-debug --disable-tests && \
make -j$(nproc) && \
Expand Down
3 changes: 2 additions & 1 deletion Dockerfile.windows
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,8 @@ ADD --checksum=sha256:${ICU_SHA256} \
# The udata.cpp patch adds the weak per-item decompression hook (see the
# stage comment above); it lands in the cross-built sicuuc.lib.
RUN mkdir -p /icu-src && tar -xzf /icu-src.tgz -C /icu-src --strip-components=1 && rm /icu-src.tgz && \
patch -p1 -d /icu-src < /icu-bun/udata-decompress-hook.patch
patch -p1 -d /icu-src < /icu-bun/udata-decompress-hook.patch && \
patch -p1 -d /icu-src < /icu-bun/icu-23110-numrange-percent.patch
Comment on lines +209 to +210

@claude claude Bot Jul 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Note that the native Windows build path (windows-release.ps1 → build-icu.ps1) — still used by CI for the bun-webkit-windows-arm64-debug artifact — downloads icu4c-73_2-src.tgz and builds via MSBuild without applying any .patch to the ICU C++ source, so that artifact will still show formatRange(0.5, 0.5) → "~5,000%". This is a pre-existing gap (udata-decompress-hook.patch is also skipped there) and only affects a debug-only artifact, so probably fine as a follow-up rather than blocking this PR.

Extended reasoning...

What this is

The PR applies icu/icu-23110-numrange-percent.patch in every Dockerfile that builds ICU from source — Dockerfile, Dockerfile.musl, Dockerfile.android, Dockerfile.freebsd, and Dockerfile.windows — exactly as the description says. However, there is one more ICU-from-source build path in the repo that CI still exercises: the native Windows runner job, which goes windows-release.ps1 → build-icu.ps1. That path is not touched by this PR, so the artifact it produces will still contain unpatched ICU 73.2 with the double-scaling bug.

Code path

In .github/workflows/build-reusable.yml:

  • Lines 151–153 note that most Windows variants moved to Dockerfile.windows, but the native windows job is kept for arm64-debug, and windows-release.ps1 "remains for building on a real Windows machine".
  • Lines 247–254: the windows job matrix has a single entry — runner: windows-11-arm, build-type: Debug, label: bun-webkit-windows-arm64-debug.
  • Line 298: that job runs ./windows-release.ps1 -Platform ARM64.
  • Lines 712/806/894: the resulting bun-webkit-windows-arm64-debug artifact is downloaded and published to the autobuild-{sha} release alongside all the Docker-built artifacts.

In build-icu.ps1:

  • Line 32: downloads icu4c-73_2-src.tgz from the upstream release.
  • Line 85 onward: the only "patching" is text replacement on .vcxproj / .props / makedata.mak build-config files. No .patch from icu/ is ever applied to source/i18n/numrange_impl.cpp or numrange_impl.h.

So sicuin.lib in bun-webkit-windows-arm64-debug is compiled from unmodified ICU 73.2 numrange_impl.cpp.

Why nothing existing prevents it

Dockerfile.windows (lines 209–210) does apply the patch, but that file only produces the cross-compiled x64 / arm64 release/LTO/asan artifacts. The native windows job doesn't use Docker at all — it invokes build-icu.ps1 directly on the runner, and that script has never had a step that reads icu/*.patch.

Step-by-step proof

  1. CI runs the windows job on windows-11-arm → ./windows-release.ps1 -Platform ARM64.
  2. windows-release.ps1 calls build-icu.ps1, which fetches icu4c-73_2-src.tgz and extracts it.
  3. build-icu.ps1 edits vcxproj/props for /MT static, then runs MSBuild. source/i18n/numrange_impl.cpp is untouched, so it still contains data.quantity1.resetExponent(); fApproximatelyFormatter.preProcess(data.quantity1, ...) on the approximately path.
  4. The resulting sicuin.lib is packed into bun-webkit-windows-arm64-debug.tar.gz and published.
  5. A Bun built against that artifact evaluates new Intl.NumberFormat("en", {style:"percent"}).formatRange(0.5, 0.5): JSC calls unumrf_formatDoubleRange(0.5, 0.5), ICU's formatterImpl1.preProcess scales 0.5 → 50, the identity check picks UNUM_IDENTITY_FALLBACK_APPROXIMATELY, formatApproximately calls fApproximatelyFormatter.preProcess on the same already-scaled quantity → 5000, and the output is "~5,000%" instead of "~50%".

Impact

Low. Only the bun-webkit-windows-arm64-debug artifact is affected — a Debug build, not something end users install. All release/LTO Windows artifacts come from Dockerfile.windows and are correctly patched. Also, this is a pre-existing infrastructure gap: build-icu.ps1 already skips udata-decompress-hook.patch, so "ICU source patches are not applied on the native Windows path" predates this PR rather than being introduced by it. The PR description scoped itself to "every Dockerfile that builds ICU from source", and it delivers exactly that.

Fix

As a follow-up (not blocking): have build-icu.ps1 apply icu/*.patch after extraction — e.g. git apply (git is on the runner), or patch.exe from Git for Windows, or a targeted PowerShell -replace on numrange_impl.cpp/.h. Alternatively, note the limitation in the PR / patch header so the arm64-debug divergence is documented.


# Stage 1: host build — only the tools matter.
RUN mkdir -p /icu-host && cd /icu-host && \
Expand Down
56 changes: 56 additions & 0 deletions icu/icu-23110-numrange-percent.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
Backport of ICU-23110 (https://unicode-org.atlassian.net/browse/ICU-23110,
upstream commit 757be359). NumberRangeFormatterImpl::formatApproximately
re-ran preProcess on data.quantity1 after the caller had already preProcessed
it, so any multiplier (scale/100 for style:"percent") was applied twice on
the identity/approximately path. Fixed upstream in ICU 78.1; drop this patch
once the bundled ICU is >= 78.

--- a/source/i18n/numrange_impl.cpp
+++ b/source/i18n/numrange_impl.cpp
@@ -160,6 +160,8 @@ void NumberRangeFormatterImpl::format(UFormattedNumberRangeData& data, bool equa
return;
}

+ DecimalQuantity quantityBackup(data.quantity1);
+
MicroProps micros1;
MicroProps micros2;
formatterImpl1.preProcess(data.quantity1, micros1, status);
@@ -216,7 +218,7 @@ void NumberRangeFormatterImpl::format(UFormattedNumberRangeData& data, bool equa
UNUM_IDENTITY_RESULT_EQUAL_BEFORE_ROUNDING):
case identity2d(UNUM_IDENTITY_FALLBACK_APPROXIMATELY_OR_SINGLE_VALUE,
UNUM_IDENTITY_RESULT_EQUAL_AFTER_ROUNDING):
- formatApproximately(data, micros1, micros2, status);
+ formatApproximately(data, quantityBackup, micros1, micros2, status);
break;

case identity2d(UNUM_IDENTITY_FALLBACK_APPROXIMATELY_OR_SINGLE_VALUE,
@@ -248,15 +250,15 @@ void NumberRangeFormatterImpl::formatSingleValue(UFormattedNumberRangeData& data


void NumberRangeFormatterImpl::formatApproximately (UFormattedNumberRangeData& data,
+ DecimalQuantity quantity,
MicroProps& micros1, MicroProps& micros2,
UErrorCode& status) const {
if (U_FAILURE(status)) { return; }
if (fSameFormatters) {
// Re-format using the approximately formatter:
MicroProps microsAppx;
- data.quantity1.resetExponent();
- fApproximatelyFormatter.preProcess(data.quantity1, microsAppx, status);
- int32_t length = NumberFormatterImpl::writeNumber(microsAppx.simple, data.quantity1, data.getStringRef(), 0, status);
+ fApproximatelyFormatter.preProcess(quantity, microsAppx, status);
+ int32_t length = NumberFormatterImpl::writeNumber(microsAppx.simple, quantity, data.getStringRef(), 0, status);
length += microsAppx.modInner->apply(data.getStringRef(), 0, length, status);
length += microsAppx.modMiddle->apply(data.getStringRef(), 0, length, status);
microsAppx.modOuter->apply(data.getStringRef(), 0, length, status);
--- a/source/i18n/numrange_impl.h
+++ b/source/i18n/numrange_impl.h
@@ -64,6 +64,7 @@ class NumberRangeFormatterImpl : public UMemory {
UErrorCode& status) const;

void formatApproximately(UFormattedNumberRangeData& data,
+ DecimalQuantity quantity,
MicroProps& micros1, MicroProps& micros2,
UErrorCode& status) const;

Loading