1075758 - Update to ICU 55.1

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[NVD]

http://site.icu-project.org/download/54

ICU 54.1 officially released, should update to it if possible.

Comment 1

[Makoto Kato [:m_kato]]

FYI.  Maybe, pkgdata cannot create ICU data since our commandline parameters is too long.
http://bugs.icu-project.org/trac/ticket/11732

Comment 2

[Jeff Walden [:Waldo]]

I have patches working locally that do this, at least as far as building JS standalone goes.  I didn't run into issues pointed out in comment 1.  Maybe I'm lucky, or maybe it's not as big a deal as it seems, or maybe it's a QoI concern but not a correctness/build-succeeded issue I would observe so quickly.  Or something.

Patches shortly.

Comment 4

[Jeff Walden [:Waldo]]

I think my patches are nearly complete for this, green possibly across the board, save on Windows:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca531c547543
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f07b48d8ce21

The Windows issue looks like it might be what comment 1 referred to, but I don't have enough in the build log to be absolutely sure about that.  I may post the patches I have for review, then do the last patch for the Windows thing once I've figured it out and patched it.  If I don't, those pushes contain all the same patches as a sneak preview.

Comment 5

[Jeff Walden [:Waldo]]

The build failure does appear to be in pgkdata, but what comment 1 points out is a failure in a method that exists in ICU trunk but doesn't exist in ICU 55.1.  A similar sort of failure in adjacent code seems most likely.

I'll see what I can figure out with a build of those patches on a Windows system, hopefully.

Comment 6

[Jeff Walden [:Waldo]]

Or no, I was consulting un-updated code when I claimed comment 1's issue was only on ICU trunk, not in ICU 55.1.  On my system the overflowing command is ~584 characters in a 512 character buffer.  As comment 1's report suggests, tweaking from a small buffer to a large buffer (512 -> 2048) works a charm on my system.  Tryservering now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c716ee5f002e

I could imagine upstream doing a proper fix and handling arbitrarily-long compiler invocations.  But the hackish fix of just bumping buffer size gives us ~1.5KB space beyond what we need but don't have now, so I think we can leave a 100% rigorous solution to upstream, if they even care.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Adjust SVN URL in update-icu.sh to 55.1, add an early-exit so script is runnable without applying possibly-bad patches

Comment 8

[Jeff Walden [:Waldo]]

<omitting upload of patches that do a full ICU import and remove all the layout/samples/tests/unused data files from ICU after running the update script with that patch applied>

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Update intl/icu/SVN-INFO for the update to 55.1

Make the relevant changes to SVN-INFO corresponding to the update script being run while pulling 55.1, for clarity.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Remove local patch to make ICU build with MSYS/MSVC -- upstream has since acquired such support

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Remove local patch to not override CC/CXX when building with *BSD -- patch landed upstream

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Apply the local patch for bug 915735 (it works without needing any updating)

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Remove omitCollationRules patch, already performed upstream

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Remove a patch properly qualifying a type in umutex.h (already landed upstream)

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Update the local patch to suppress various clang warnings in ICU

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Remove the applied-upstream patch to pass the right argument type to InterlockedCompareExchange in ICU

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Use a larger buffer in pkgdata, corresponding to Mozilla's inordinately-long compiler command

I agree with your suggestion as to just bumping the buffer size.

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Update the clang plugin to ignore implicit constructors in ICU 55

It would be nice to not have to update this every time, but it's at least mildly nice to be aware of this existing, I guess.

Comment 19

[Jeff Walden [:Waldo]]

With all that done, tests pass, things are good, and we even pick up a few bugfixes.  For example, right now:

js> new Intl.NumberFormat("en-US", { style: "currency", currencyDisplay: "name", currency: "USD" }).format(17.17)
typein:1:0 Error: internal error while computing Intl data

With ICU updated:

js> new Intl.NumberFormat("en-US", { style: "currency", currencyDisplay: "name", currency: "USD" }).format(17.17)
"17.17 US dollars"

Comment 20

[(no longer active)]

Comment on attachment 8621846 [details] [diff] [review]
Update the clang plugin to ignore implicit constructors in ICU 55

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

It's pretty sad to have to do this every time we update ICU.  Let me give you a better patch to fix this once and for all!

Comment 21

[(no longer active)]

Attachment: Do not look at the ICU version number when whitelisting the ICU namespace in the clang plugin

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 8622563 [details] [diff] [review]
Do not look at the ICU version number when whitelisting the ICU namespace in the clang plugin

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

::: build/clang-plugin/clang-plugin.cpp
@@ +143,5 @@
> +  return name == "std" ||               // standard C++ lib
> +         name == "__gnu_cxx" ||         // gnu C++ lib
> +         name == "boost" ||             // boost
> +         name == "webrtc" ||            // upstream webrtc
> +         name.substr(0, 4) == "icu_" || // icu

Stab aligned comments/operators/anything stab stab stab.

Comment 23

[(no longer active)]

Comment on attachment 8622563 [details] [diff] [review]
Do not look at the ICU version number when whitelisting the ICU namespace in the clang plugin

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8487036f57

Comment 24

[(no longer active)]

UnCCing myself, please CC me again if something needs my attention.

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8621827 [details] [diff] [review]
Remove local patch to make ICU build with MSYS/MSVC -- upstream has since acquired such support

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

That really doesn't need to be a separate patch.

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8621828 [details] [diff] [review]
Remove local patch to not override CC/CXX when building with *BSD -- patch landed upstream

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

Likewise.

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8621823 [details] [diff] [review]
Adjust SVN URL in update-icu.sh to 55.1, add an early-exit so script is runnable without applying possibly-bad patches

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

Please fold everything.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d8487036f57

Comment 29

[Jeff Walden [:Waldo]]

Yeah, the plan was (I had sort of thought obviously) to fold everything together before landing.  But one undistinguished mess is basically unreviewable, so I split things up for that.

I could have sworn I did a try-push of all this, but an unrelated push just recently revealed some test failures with the update.  I need to investigate that a little more before I can land everything.

Comment 30

[Jeff Walden [:Waldo]]

Attachment: Update String.prototype.normalize tests for normalization changes in ICU 55

I didn't go through this with a fine-toothed comb, but https://codepoints.net/U+A69C at least says that that one hex number was introduced in Unicode 7.0, comporting with the 6.3->7.0 change in the test file, so that check for change-sanity works out.

Comment 31

[Tooru Fujisawa [:arai]]

Comment on attachment 8623444 [details] [diff] [review]
Update String.prototype.normalize tests for normalization changes in ICU 55

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

Thank you for updating the test!
No converting issue found there :)

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c8d00afb56
https://hg.mozilla.org/integration/mozilla-inbound/rev/72d5e6070f98
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0938af93eb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/69d7bda985a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c76364c9a1d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/addddfefa2e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e575191a567b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce5c66c1283
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcc5ea79885
https://hg.mozilla.org/integration/mozilla-inbound/rev/80841ebc9aca
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8ff9ab0b32d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6a57dc9a5a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/319ea1367089

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f30e1413ebd1

Comment 34

[Jeff Walden [:Waldo]]

Sigh, forgot to fold those together before landing.  :-(  Oh, and I seem to have hit the updating-ICU-needs-a-CLOBBER bug, that I believe is already on file.  So it goes.

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f9c8d00afb56
https://hg.mozilla.org/mozilla-central/rev/72d5e6070f98
https://hg.mozilla.org/mozilla-central/rev/a0938af93eb2
https://hg.mozilla.org/mozilla-central/rev/69d7bda985a7
https://hg.mozilla.org/mozilla-central/rev/c76364c9a1d0
https://hg.mozilla.org/mozilla-central/rev/addddfefa2e8
https://hg.mozilla.org/mozilla-central/rev/e575191a567b
https://hg.mozilla.org/mozilla-central/rev/2ce5c66c1283
https://hg.mozilla.org/mozilla-central/rev/cbcc5ea79885
https://hg.mozilla.org/mozilla-central/rev/80841ebc9aca
https://hg.mozilla.org/mozilla-central/rev/b8ff9ab0b32d
https://hg.mozilla.org/mozilla-central/rev/a6a57dc9a5a7
https://hg.mozilla.org/mozilla-central/rev/319ea1367089
https://hg.mozilla.org/mozilla-central/rev/f30e1413ebd1

Comment 36

[Gregory Szorc [:gps]]

FTR, this series of pushes added ~27MB to the total size of mozilla-central. Anything we can do to cut down on bloat for future ICU upgrades would be appreciated. But hopefully we'll have shallow clone by then and this won't matter so much.

Comment 37

[Jeff Walden [:Waldo]]

Oops, this is done.