Closed Bug 1075758 Opened 5 years ago Closed 5 years ago

Update to ICU 55.1

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42

People

(Reporter: ionnv, Assigned: Waldo)

References

()

Details

Attachments

(13 files)

2.21 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1004 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
12.70 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.95 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.68 KB, patch
glandium
: review+
Details | Diff | Splinter Review
6.30 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.80 KB, patch
glandium
: review+
Details | Diff | Splinter Review
7.66 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.15 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.82 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
2.32 KB, patch
ehsan
: review-
Details | Diff | Splinter Review
2.19 KB, patch
Waldo
: review+
ehsan
: checkin+
Details | Diff | Splinter Review
96.67 KB, patch
arai
: review+
Details | Diff | Splinter Review
http://site.icu-project.org/download/54

ICU 54.1 officially released, should update to it if possible.
Depends on: 924839
Blocks: 866372
Summary: Update to ICU 54.1 → Update to ICU 55.1
FYI.  Maybe, pkgdata cannot create ICU data since our commandline parameters is too long.
http://bugs.icu-project.org/trac/ticket/11732
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.
Assignee: nobody → jwalden+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Duplicate of this bug: 992079
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.
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.
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.
<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>
Make the relevant changes to SVN-INFO corresponding to the update script being run while pulling 55.1, for clarity.
Attachment #8621826 - Flags: review?(mh+mozilla)
I agree with your suggestion as to just bumping the buffer size.
Attachment #8621836 - Flags: review?(m_kato)
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.
Attachment #8621846 - Flags: review?(ehsan)
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"
Attachment #8621836 - Flags: review?(m_kato) → review+
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!
Attachment #8621846 - Flags: review?(ehsan) → review-
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.
Attachment #8622563 - Flags: review?(jwalden+bmo) → review+
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
Attachment #8622563 - Flags: checkin+
UnCCing myself, please CC me again if something needs my attention.
Keywords: leave-open
Attachment #8621826 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8621827 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8621828 - Flags: review?(mh+mozilla) → review+
Attachment #8621829 - Flags: review?(mh+mozilla) → review+
Attachment #8621830 - Flags: review?(mh+mozilla) → review+
Attachment #8621832 - Flags: review?(mh+mozilla) → review+
Attachment #8621833 - Flags: review?(mh+mozilla) → review+
Attachment #8621834 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8621823 - Flags: review?(mh+mozilla) → review+
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.
Depends on: 1175347
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.
Attachment #8623444 - Flags: review?(arai.unmht)
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 :)
Attachment #8623444 - Flags: review?(arai.unmht) → review+
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.
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.
Depends on: 1179106
Blocks: 1013091
Oops, this is done.
Keywords: leave-open
Target Milestone: --- → mozilla42
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1176053
Blocks: 1197724
Depends on: 1228227
Duplicate of this bug: 1107956
You need to log in before you can comment on or make changes to this bug.