Update our in-tree ICU to 61

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Waldo, Assigned: anba)

Tracking

Trunk
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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
(Reporter)

Comment 2

a year ago
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?
(Assignee)

Comment 3

a year ago
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.
(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
(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:
(Reporter)

Comment 6

a year ago
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.
(Reporter)

Comment 7

a year ago
...or, oh, maybe you meant *your* upstream changes.  Generalized, importable libraries are hard.  :-(
(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…
(Reporter)

Comment 9

a year ago
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.
(Reporter)

Comment 10

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

Comment 12

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

Comment 13

a year ago
This patch is no longer needed now that ICU uses double-conversion internally.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8962858 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 14

a year ago
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/
Attachment #8962862 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 15

a year ago
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
Attachment #8962863 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 16

a year ago
Clobber for ICU update
Attachment #8962864 - Flags: review+
(Assignee)

Updated

a year ago
Blocks: 1449574
(Assignee)

Updated

a year ago
Duplicate of this bug: 1449626
(Reporter)

Updated

a year ago
Attachment #8962858 - Flags: review?(jwalden+bmo) → review+
(Reporter)

Comment 19

a year ago
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
Attachment #8962862 - Flags: review?(jwalden+bmo) → review+
(Reporter)

Updated

a year ago
Attachment #8962863 - Flags: review?(jwalden+bmo) → review+

Comment 21

a year ago
Pushed by dluca@mozilla.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.