fix(intl): discover the installed ICU version at runtime instead of a hardcoded max#908
Merged
Conversation
… hardcoded max The Linux ICU loader looped a fixed `ICU_VERSION_MAX = 76 downto 70` range, so a newer ICU runtime (77+, e.g. on Ubuntu 25.10) was never found: `libicui18n.so` failed to load, all Intl returned empty, and DateTimeFormat fell back to printing raw numbers. Supporting a new ICU required bumping the constant by hand. Replace the hardcoded ceiling with runtime discovery: scan the standard library directories for the installed `libicui18n.so.<major>` files, take the newest major present, and try it (down to a `ICU_VERSION_MIN` compatibility floor), keeping the unversioned-SONAME fallback. There is no upper bound, so any future ICU release is picked up with no code change. LoadLibrary still resolves the final path through the dynamic linker; the scan only learns which majors exist. Adds source/shared/ICU.Test.pas covering the version parsing and the newest-present directory scan, including majors above the old 76 ceiling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ICU SONAME parsing and runtime major discovery, rewrites Linux loading to scan installed libraries, and expands tests for single-directory and directory-list lookup. ChangesICU Runtime Version Discovery
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
Suite TimingTest Runner (interpreted: 11,046 passed; bytecode: 11,046 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 437; bytecode: 437)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results437 benchmarks · PR vs same-runner Interpreted: 🟢 19 improved · 🔴 36 regressed · 382 unchanged · avg -0.6% Typical per-run noise (median variance): interpreted ±2.9%, bytecode ±2.6%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 14 unch. · avg -1.1% · Bytecode: 🔴 1, 13 unch. · avg -2.7%
arrays.js — Interp: 🔴 1, 18 unch. · avg -0.8% · Bytecode: 🔴 3, 16 unch. · avg -1.4%
async-await.js — Interp: 6 unch. · avg -0.5% · Bytecode: 6 unch. · avg +0.5%
async-generators.js — Interp: 2 unch. · avg -11.7% · Bytecode: 2 unch. · avg +7.0%
atomics.js — Interp: 6 unch. · avg +1.8% · Bytecode: 🔴 1, 5 unch. · avg +0.6%
base64.js — Interp: 🔴 1, 9 unch. · avg -0.7% · Bytecode: 10 unch. · avg +1.2%
classes.js — Interp: 🔴 2, 29 unch. · avg -1.2% · Bytecode: 🟢 1, 🔴 3, 27 unch. · avg -0.0%
closures.js — Interp: 11 unch. · avg -0.0% · Bytecode: 11 unch. · avg -0.8%
collections.js — Interp: 12 unch. · avg -0.9% · Bytecode: 🟢 1, 11 unch. · avg -2.0%
csv.js — Interp: 13 unch. · avg -0.9% · Bytecode: 13 unch. · avg +3.7%
destructuring.js — Interp: 🔴 3, 19 unch. · avg -1.8% · Bytecode: 🟢 1, 21 unch. · avg +0.1%
fibonacci.js — Interp: 🔴 2, 6 unch. · avg -2.1% · Bytecode: 🔴 1, 7 unch. · avg -1.6%
float16array.js — Interp: 🟢 1, 🔴 6, 25 unch. · avg -3.2% · Bytecode: 🔴 2, 30 unch. · avg -1.5%
for-in/for-in.js — Interp: 🔴 1, 2 unch. · avg -2.6% · Bytecode: 3 unch. · avg -1.9%
for-of.js — Interp: 7 unch. · avg -1.8% · Bytecode: 🔴 1, 6 unch. · avg -3.1%
generators.js — Interp: 4 unch. · avg +2.9% · Bytecode: 🟢 1, 3 unch. · avg +1.1%
intl.js — Interp: 🟢 1, 5 unch. · avg +3.8% · Bytecode: 6 unch. · avg +1.8%
iterators.js — Interp: 🟢 2, 🔴 2, 38 unch. · avg -1.3% · Bytecode: 🟢 11, 31 unch. · avg +5.0%
json.js — Interp: 🔴 1, 22 unch. · avg -0.8% · Bytecode: 🟢 1, 🔴 1, 21 unch. · avg +0.4%
jsx.jsx — Interp: 🟢 4, 17 unch. · avg +3.5% · Bytecode: 🟢 1, 🔴 4, 16 unch. · avg -2.2%
modules.js — Interp: 9 unch. · avg +1.8% · Bytecode: 🔴 1, 8 unch. · avg -2.0%
numbers.js — Interp: 🔴 1, 11 unch. · avg +2.1% · Bytecode: 🔴 1, 11 unch. · avg +0.9%
objects.js — Interp: 7 unch. · avg -4.3% · Bytecode: 🔴 2, 5 unch. · avg +0.8%
promises.js — Interp: 12 unch. · avg +1.3% · Bytecode: 🟢 1, 11 unch. · avg -1.6%
property-access.js — Interp: 🔴 2, 3 unch. · avg -3.9% · Bytecode: 🔴 1, 4 unch. · avg -2.2%
regexp.js — Interp: 11 unch. · avg -2.8% · Bytecode: 🔴 1, 10 unch. · avg -2.7%
strings.js — Interp: 🔴 1, 18 unch. · avg +1.0% · Bytecode: 🟢 1, 🔴 2, 16 unch. · avg +2.1%
temporal.js — Interp: 🟢 1, 5 unch. · avg +3.6% · Bytecode: 6 unch. · avg +1.1%
tsv.js — Interp: 🟢 1, 8 unch. · avg +0.6% · Bytecode: 9 unch. · avg +0.0%
typed-arrays.js — Interp: 🟢 2, 🔴 6, 14 unch. · avg -2.0% · Bytecode: 🟢 8, 🔴 7, 7 unch. · avg +15.1%
uint8array-encoding.js — Interp: 🟢 7, 🔴 2, 9 unch. · avg +10.6% · Bytecode: 🔴 8, 10 unch. · avg -13.0%
weak-collections.js — Interp: 🔴 5, 10 unch. · avg -11.5% · Bytecode: 🟢 3, 12 unch. · avg +7.1%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Each PR run also builds the |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+1 / -1)Newly failing (1):
Newly passing (1):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
The version parsing and "newest present" directory scan are pure string and
directory logic, but they were declared inside the Linux {$IFDEF} alongside the
loader, so ICU.Test could only exercise them on Linux — even though the logic
they test works anywhere.
Move ParseICUSoMajorVersion and HighestICUMajorVersionInDir out of the Linux
block and parameterise the scan by SONAME base, leaving only the actual library
loading (system-path scan + LoadLibrary) Linux-only. The directory scan now
enumerates entries and lets the parser decide what matches, rather than relying
on FindFirst wildcard semantics that differ across platforms. ICU.Test drops its
{$IFDEF LINUX} gating and runs on every platform.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/shared/ICU.pas`:
- Around line 116-124: DiscoverHighestICUMajorVersion currently only checks
ICU_SCAN_DIRS, so add linker-search-path discovery to make versioned ICU
installs visible to TryLoadLinuxICU. Update the ICU discovery flow around
DiscoverHighestICUMajorVersion, HighestICUMajorVersionInDir, and TryLoadLinuxICU
to also inspect linker paths such as those from LD_LIBRARY_PATH and include
those directories when searching for libicu*.so.<major> SONAMEs. Keep the
unversioned fallback, but ensure versioned libraries reachable only through
linker paths are found first.
In `@source/shared/ICU.Test.pas`:
- Around line 32-39: The temp fixture setup is swallowing I/O failures, so the
test can continue and fail later with a misleading assertion instead of the real
problem. Update TouchFile to raise when FileCreate fails, and change the setup
around ForceDirectories so directory creation is checked and any failure aborts
immediately. Use the existing TouchFile helper and the fixture setup block that
prepares the temp path/files to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a6b4050-e522-425a-a1bf-10e532909e67
📒 Files selected for processing (2)
source/shared/ICU.Test.passource/shared/ICU.pas
Address PR review: DiscoverHighestICUMajorVersion only scanned the fixed ICU_SCAN_DIRS, so an ICU reachable only through a dynamic-linker env path (LD_LIBRARY_PATH) was never found — its versioned SONAME was never tried, and the unversioned fallback does not cover a runtime-only `libicu*.so.<major>`. The previous range-loop happened to cover this because LoadLibrary searches linker paths by SONAME; the discovery rewrite lost it. Add a generic, cross-platform HighestICUMajorVersionInDirList that scans a separator-delimited directory list, and feed it LD_LIBRARY_PATH from the Linux discovery. The newest major found there is then loaded by SONAME through the linker as before. Verified end-to-end on x86_64 Linux: with ICU moved out of all standard dirs, Intl is unavailable without LD_LIBRARY_PATH and works with it. Also address the test-fixture review: ICU.Test now fails fast — TouchFile raises when FileCreate fails and directory creation goes through a MakeTempDir helper that raises on ForceDirectories failure — so a fixture I/O error reports the real problem instead of a misleading assertion. Adds a cross-platform test for the new directory-list scan. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/shared/ICU.Test.pas`:
- Around line 109-119: The multi-directory ICU test is using a path separator
that can collide with absolute temp paths on Windows, so the split logic breaks
before the intended case is exercised. Update the
`HighestICUMajorVersionInDirList` test fixture in `ICU.Test.pas` to use a
separator that cannot appear in `MakeTempDir` outputs (for example a test-only
delimiter like `|`) and pass that same delimiter consistently into the helper
call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c29f204-4072-4b16-bf11-c973cad34a79
📒 Files selected for processing (2)
source/shared/ICU.Test.passource/shared/ICU.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/shared/ICU.pas
Address PR review: TestHighestInDirListAcrossPaths built the list with ':' over absolute temp paths, which on Windows contain a drive-letter colon (C:\...) and would split there, breaking the test before it exercises the multi-directory logic. Use '|' — which cannot occur in a generated path — as the fixture separator. HighestICUMajorVersionInDirList takes the separator as a parameter, so the split logic under test is identical; the production caller still passes ':' for LD_LIBRARY_PATH. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
source/shared/ICU.pas) looped a hardcodedICU_VERSION_MAX = 76 downto 70, so a newer ICU runtime — 77+, as shipped by e.g. Ubuntu 25.10 — was never found:libicui18n.sofailed to load, allIntlreturned empty, andIntl.DateTimeFormatsilently fell back to printing the raw epoch number. Supporting each new ICU required bumping the constant by hand.LD_LIBRARY_PATHdirectories for the installedlibicui18n.so.<major>files, take the newest major present, and try it down to aICU_VERSION_MINcompatibility floor, keeping the unversioned-SONAME fallback. There is no upper bound, so any future ICU release loads with no code change.LoadLibrarystill resolves the final path through the dynamic linker; the scan only learns which majors exist.ParseICUSoMajorVersion(SONAME → major) andHighestICUMajorVersionInDir(newest<base>.<major>in a dir) — are platform-independent, so they live outside the Linux{$IFDEF}and are unit-tested on every platform. Only the system-path scan +LoadLibrarystay Linux-only. The directory scan enumerates entries and lets the parser decide what matches, rather than relying onFindFirstwildcard semantics that differ across platforms.source/shared/ICU.Test.pascovering the version parsing (libicui18n.so.77→ 77,…so.76.1→ 76, unversioned/garbage → 0) and the newest-present directory scan, including majors above the old 76 ceiling (74/76/77/100 → 100).Constraints / non-goals: the actual ICU loading is Linux-only — the macOS (
libicucore) and Windows (icu.dll) paths are untouched. No behavior change on systems where an ICU in 70–76 is installed. Discovered while investigating theintl402/.../formatToParts/compare-to-temporal.jsCI flake (that test's flakiness is a separate, contention-driven timeout, not this bug).Testing
Verified no regressions and confirmed the new feature or bugfix in end-to-end JavaScript/TypeScript tests
Updated documentation
Optional: Verified no regressions and confirmed the new feature or bugfix in native Pascal tests (if AST, scope, evaluator, or value types changed)
Optional: Verified no benchmark regressions or confirmed benchmark coverage for the change
ICU.Testpasses on both platforms (2/2 on macOS and on x86_64 Linux): version parsing and the newest-present scan, with fake majors 77 and 100 → 100 (proving the ceiling is gone). The string/directory logic is now tested everywhere, not just Linux.x86_64 Linux (Ubuntu, ICU 76): engine builds, Intl loads via runtime discovery,
Intl.DateTimeFormat(...).format/formatToPartsproduce correct output, andintl402/DateTimeFormat/prototype/formatToParts/compare-to-temporal.jspasses.LD_LIBRARY_PATHdiscovery verified end-to-end: with ICU 76 moved out of every standard dir,Intl.DateTimeFormatis unavailable with noLD_LIBRARY_PATH(returns the raw number) and works again withLD_LIBRARY_PATHpointing at the relocated libs.macOS:
./build.pas testrunner+ full JS suite 11046/11046; all Pascal unit tests build and pass../format.pas --checkclean.No documentation/version-range update needed — the supported ICU range was not documented (only "the
libicusystem package").