1445465 - Update our in-tree ICU to 61

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, enhancement, P2)

Description

[Jeff Walden [:Waldo]]

There's a release candidate for 61 out now (accepting comments til March 21), and presumably the actual release will follow shortly.

It probably wouldn't be a bad idea to try importing the RC and make sure there's nothing too broken in it for our purposes, but I'm not sure I have the time to do it myself.

Comment 1

[André Bargull [:anba]]

I forgot to clobber (bug 1315397) the try-build [1], but I'm not sure clobber will help to fix the redefinition errors for double-conversion: There's double-conversion in mfbt and now also in ICU and I have no idea if the two double-conversion copies are compatible, e.g. we have a few local patches for it, so we may end up with two copies of it.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ad3b141b3b4117bfa59ef2136b3cb74b0a4282c

Comment 2

[Jeff Walden [:Waldo]]

I suppose we could prefix-rename our double_conversion import in mfbt, to at least allow the two to coexist.  It's probably too late in the ICU cycle to get upstream to add a configure option to link against/use a user-provided double_conversion library, right?  That might be a medium-term thing to pursue, perhaps?  Possibly?  Probably?

Comment 3

[André Bargull [:anba]]

It looks like ICU doesn't use the functions we've patched (ICU has them |#if 0|-ed out), so maybe it's possible to remove ICU's double-conversion files and then have local ICU patches to change the two ICU source files (i18n/digitlst.cpp and i18n/number_decimalquantity.cpp) which are using double-conversion to use the mfbt version.

Comment 4

[Steven R. Loomis]

(In reply to Jeff Walden [:Waldo] from comment #2)
> I suppose we could prefix-rename our double_conversion import in mfbt, to at
> least allow the two to coexist.  It's probably too late in the ICU cycle to
> get upstream to add a configure option to link against/use a user-provided
> double_conversion library, right?  That might be a medium-term thing to
> pursue, perhaps?  Possibly?  Probably?

This is slated to be fixed in ICU for *61* by putting double_conversion into the ICU namespace. I agree, a config option to link against a user provided library is separate. (sffc said there were local changes too, which should be upstreamed if possible).  Please file a linked ticket to https://ssl.icu-project.org/trac/ticket/13648 for, probably an option to use pkgconfig and/or a user provided path for a user double_conversion

Comment 5

[Steven R. Loomis]

(In reply to Steven R. Loomis from comment #4)
> (In reply to Jeff Walden [:Waldo] from comment #2)

forgot to add: thanks for the report :+1:

Comment 6

[Jeff Walden [:Waldo]]

No problem.  :-)

Our local double_conversion changes are generally silly, Mozilla-specific, non-upstreamable things.  It seems like  

* Mozilla-specific symbol visibility adjustments, for (I think?) cross-library linking
* use Mozilla's assertion mechanism, not the one the code would ordinarily use

definitely aren't upstreamable.  (Or at least I'm not sure how one could, in library code, support default-hidden-visibility with configurable, opt-in visibility on a per-symbol basis.  Not sure I've ever seen a generalized approach to this recurring issue in libraries.)  The remaining two

* always use <stdint.h> types rather than custom typedefs on Windows only (pre-MSVC 2010 compat goo)
* make DoubleToStringConverter::ToPrecision indicate whether the string it built up contains exponential notation

are perhaps upstreamable, tho in the former case maybe precluded by MSVC support requirements and in the latter case the reason for it is just very weird and maybe can't justify the API addition.

I'll file the separate ticket for some sort of user-provided library, sure.

Comment 7

[Jeff Walden [:Waldo]]

...or, oh, maybe you meant *your* upstream changes.  Generalized, importable libraries are hard.  :-(

Comment 8

[Steven R. Loomis]

(In reply to Jeff Walden [:Waldo] from comment #7)
> ...or, oh, maybe you meant *your* upstream changes.  Generalized, importable
> libraries are hard.  :-(

Yes, I meant ours.  Maybe they are the same changes? Or maybe not.

I think you answered my question: would you ever use a platform library version of double_conversion? 

> default-hidden-visibility with configurable, opt-in visibility on a per-symbol basis

that's part of why we rename symbols…

Comment 9

[Jeff Walden [:Waldo]]

If it were universally available across platforms, possibly?  The assertion mechanism thing is I think mostly a nice-to-have.  Symbol visibility wouldn't matter if we weren't controlling what happens inside our own library.  <stdint.h> is possibly just silliness.

The exponent-notation thing is the only quirk that would demand an "interesting" upstream fix.  Might be they'd take it, but it would be an API change, or at least API addition.  The reason we made that change at all is very Dumb.

I don't think we'd have a problem being required to massaging our local manipulations such that they appeased ICU.  Although doing that might require undoing icu-namespacification.  Or us manually mirroring whatever we provided, into a second namespace to appease ICU.

Comment 10

[Jeff Walden [:Waldo]]

I filed https://ssl.icu-project.org/trac/ticket/13651 for being able to use a user-provided double_conversion.

Comment 11

[André Bargull [:anba]]

Try with current ICU61 maint branch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97fb5409a45e3ef3b4511d892b1ca3e5d564e668

Comment 12

[Jeff Walden [:Waldo]]

ICU 61 is OUT.  Lookin' forward to the patch, which comment 11 *seems* to imply should not have any serious issues to overcome.

Comment 13

[André Bargull [:anba]]

Attachment: bug1445465-part1-remove-patch.patch

This patch is no longer needed now that ICU uses double-conversion internally.

Comment 14

[André Bargull [:anba]]

Attachment: bug1445465-part2-update-icu.patch

The actual update to ICU61 generated through the update script:

  ./update-icu.sh https://ssl.icu-project.org/repos/icu/tags/release-61-1/icu4c/

Comment 15

[André Bargull [:anba]]

Attachment: bug1445465-part3-update-tests.patch

js/src/tests/non262/Intl/NumberFormat/formatToParts.js
- update expected results after CLDR update


js/src/tests/non262/Intl/NumberFormat/StringBuffer.js
- update after switch to double-conversion
- also added a simplistic check to ensure the new result matches the output of Number.prototype.toFixed

Comment 16

[André Bargull [:anba]]

Attachment: bug1445465-part4-clobber.patch

Clobber for ICU update

Comment 17

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a77d0689dab11685e72ad607ab4e6bcb9d7b451

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 8962862 [details] [diff] [review]
bug1445465-part2-update-icu.patch

Review of attachment 8962862 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me 'cause there ain't much else to do.

::: intl/icu/source/data/curr/af.txt
@@ +391,5 @@
>              "Macaose pataca",
>          }
>          MRO{
>              "MRO",
> +            "Mauritaniese ouguiya (1973–2017)",

Noooooooooooooooooo I loved this five-voweled seven-letter word for Scrabble purposes

Comment 20

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a77d0689dab11685e72ad607ab4e6bcb9d7b451

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/661eb6a31761
Part 1: Remove ICU patch no longer needed with ICU 61. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8ff6d9ecca3
Part 2: Update in-tree ICU to release 61.1. rs=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/3214fb35ccd6
Part 3: Update tests. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/122d31260faa
Part 4: Clobber for ICU update. r=clobber

Comment 22

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/661eb6a31761
https://hg.mozilla.org/mozilla-central/rev/b8ff6d9ecca3
https://hg.mozilla.org/mozilla-central/rev/3214fb35ccd6
https://hg.mozilla.org/mozilla-central/rev/122d31260faa