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.
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
deps: update ICU to 72.1 #45068
Merged
Merged
deps: update ICU to 72.1 #45068
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this will break the sharedlib ICU builds which will be using the previous format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what happened. Should I make two branches depending on the ICU version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think technically the tests could be branched on the cldr version but in practice the ICU version is probably sufficient? At least we don't/haven't updated cldr independently of ICU version bumps up until now.
cc @srl295
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is always the case if you link with a backlevel ICU. I assume sharedlib = local system icu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cldr doesn't usually update separate from icu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which change? Given that you added the test, do you think we need to consider the ICU 72.1 update a semver-major change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but should we concern about user-provided deps in our tests? If the version is heavily mismatched with bundled one, it might lead to test failures in other places as well anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reviewed part, and the rest of changes in this file in this PR: https://github.com/nodejs/node/pull/45068/files#diff-a6095e09d738de6d1c78d336fac497210d041350d1ece89ef04491ff22bbf4b4L125-R135
No, the opposite: I consider changes in this file in this PR to be
semver-patch, since it only slightly changes formatting in a few locales.However, if the test would fail for a more serious reason (for example, adding a global cache for ICU that overrides environment variables even in freshly spawned process), I think it would be safer to not backport it.
The purpose of this test is to detect if something like that happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an argument for not having such tests.
I'm trying to understand what the intent of testing is… is it to verify that ICU is integrated? Or that ICU's data never changes?
Because the data changes frequently. Some examples from the past few months: Sierra Leone revaluates its currency. Croatia switches to the Euro. Turkey is now Türkiye. Date formats change due to linguistic and geopolitical requests. Kazakhistan switches to Latin script. Etc…. the output here is not generally suitable for regression tests that are expected to pass in the future without modification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLDR 42 was released yesterday. here is the delta chart with some of the data changes. To give an idea of the magnitude of what changes from release to release.