NVD Reporter
	Description • 10 years ago

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

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

	Makoto Kato [:m_kato]
	Comment 1 • 9 years ago

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

	Jeff Walden [:Waldo] Assignee
	Comment 2 • 9 years ago

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.

	Jeff Walden [:Waldo] Assignee
	Comment 4 • 9 years ago

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.

	Jeff Walden [:Waldo] Assignee
	Comment 5 • 9 years ago

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.

	Jeff Walden [:Waldo] Assignee
	Comment 6 • 9 years ago

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.

	Jeff Walden [:Waldo] Assignee
	Comment 8 • 9 years ago

<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>

	Jeff Walden [:Waldo] Assignee
	Comment 9 • 9 years ago

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

	Jeff Walden [:Waldo] Assignee
	Comment 17 • 9 years ago

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

	Jeff Walden [:Waldo] Assignee
	Comment 18 • 9 years ago

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.

	Jeff Walden [:Waldo] Assignee
	Comment 19 • 9 years ago

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"

	(no longer active)
	Comment 20 • 9 years ago

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!

	Jeff Walden [:Waldo] Assignee
	Comment 22 • 9 years ago

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.

	(no longer active)
	Comment 23 • 9 years ago

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

	(no longer active)
	Comment 24 • 9 years ago

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

	Mike Hommey [:glandium]
	Comment 25 • 9 years ago

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.

	Mike Hommey [:glandium]
	Comment 26 • 9 years ago

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.

	Mike Hommey [:glandium]
	Comment 27 • 9 years ago

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.

	Carsten Book [:Tomcat]
	Comment 28 • 9 years ago

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

	Jeff Walden [:Waldo] Assignee
	Comment 29 • 9 years ago

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.

	Jeff Walden [:Waldo] Assignee
	Comment 30 • 9 years ago

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.

	Tooru Fujisawa [:arai]
	Comment 31 • 9 years ago

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 :)

	Pulsebot
	Comment 32 • 9 years ago

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

	Pulsebot
	Comment 33 • 9 years ago

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

	Jeff Walden [:Waldo] Assignee
	Comment 34 • 9 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 35 • 9 years ago

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

	Gregory Szorc [:gps]
	Comment 36 • 9 years ago

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.

	Jeff Walden [:Waldo] Assignee
	Comment 37 • 9 years ago

Oops, this is done.