Last Comment Bug 924839 - Update in-tree ICU to 52.1 (possibly with a few local patches)
: Update in-tree ICU to 52.1 (possibly with a few local patches)
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla29
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
http://site.icu-project.org/download/52
: 932244 (view as bug list)
Depends on: 724531 966113 966347 967230
Blocks: es-intl 1215247 853301 864843 866301 966559 1075758 1215252
  Show dependency treegraph
 
Reported: 2013-10-09 03:12 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2015-10-15 12:07 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
disabled
disabled
?


Attachments
Aurora-targeted backout (1.50 KB, patch)
2013-10-28 17:40 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
nicolas.b.pierron: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Partial patchwork such that update-icu.sh would update everything as desired (27.71 KB, patch)
2013-11-12 17:20 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
irrelevant-mutex-fixup.diff (1.63 KB, patch)
2013-12-07 23:02 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: review+
Details | Diff | Splinter Review
bsd-libname-extra-s.diff (1.88 KB, patch)
2013-12-07 23:04 PST, Jeff Walden [:Waldo] (remove +bmo to email)
landry: review+
Details | Diff | Splinter Review
zero-sigfigs.diff (2.52 KB, patch)
2013-12-07 23:05 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mozillabugs: review+
Details | Diff | Splinter Review
bsd-truncate.diff (5.38 KB, patch)
2013-12-07 23:05 PST, Jeff Walden [:Waldo] (remove +bmo to email)
landry: review+
Details | Diff | Splinter Review
update-icu.diff (1.14 KB, patch)
2013-12-07 23:10 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: review+
Details | Diff | Splinter Review
import-changes.diff.gz (3.40 MB, application/gzip)
2013-12-07 23:13 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details
bug-899722-4 (1.73 KB, patch)
2013-12-07 23:15 PST, Jeff Walden [:Waldo] (remove +bmo to email)
landry: review+
Details | Diff | Splinter Review
bug-724533 (25.67 KB, patch)
2013-12-07 23:21 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
icu-cxxox.diff (7.17 KB, patch)
2013-12-07 23:24 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: review-
Details | Diff | Splinter Review
genrb-omitcollationrules.diff (37.28 KB, patch)
2013-12-07 23:48 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
fix-update.diff (946 bytes, patch)
2013-12-07 23:55 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: review+
Details | Diff | Splinter Review
Now in patch form, after further resort a hex editor (2.09 KB, patch)
2013-12-08 12:36 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mozillabugs: review+
Details | Diff | Splinter Review
bug-724533 (24.68 KB, patch)
2013-12-09 00:21 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: review+
Details | Diff | Splinter Review
upstream-windows-using-icu-fix.diff (1.94 KB, patch)
2013-12-09 00:35 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mozillabugs: review-
Details | Diff | Splinter Review
upstream-windows-using-icu-fix.diff (1.90 KB, patch)
2013-12-09 11:11 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mozillabugs: review+
Details | Diff | Splinter Review
Aurora-targeted patch: disable the Intl API in release (i.e. in beta/final release, but *not* in aurora) (1.36 KB, patch)
2013-12-09 18:51 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Make update-icu.sh behave timezone-agnostically (659 bytes, patch)
2014-01-03 15:49 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
ignore-makebuild.diff (1.08 KB, patch)
2014-01-28 18:55 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: feedback-
Details | Diff | Splinter Review
Okay, I *think* this is what was meant/discussed on IRC (220.56 KB, patch)
2014-01-29 16:18 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: review+
Details | Diff | Splinter Review
genrb-upstreamed (10.96 KB, patch)
2014-01-29 17:22 PST, Jeff Walden [:Waldo] (remove +bmo to email)
mozillabugs: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-09 03:12:56 PDT
We currently embed a ~6mo. old ICU.  It needs to be updated before we can ship it, which means before the next uplift.  Details on how to do so are here (to be moved to a better location, soon):

https://wiki.mozilla.org/User:Waldo/Internationalization_API
Comment 1 User image Chris Peterson [:cpeterson] 2013-10-11 14:10:16 PDT
btw, the next uplift date is October 28:

https://wiki.mozilla.org/RapidRelease/Calendar
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-25 09:49:46 PDT
Disabled Intl and ICU in anticipation of next uplift, will reenable in a few days after uplift (and then get to updating ICU):

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3a2d93fb173
Comment 3 User image :Ms2ger (⌚ UTC+1/+2) 2013-10-25 11:46:35 PDT
Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4bc5478669b
Comment 4 User image Chris Peterson [:cpeterson] 2013-10-25 13:45:20 PDT
Jeff: I think you need to also back out bug 853301 test_interfaces.html changes from cset b6dc96f18391:

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

Test failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=29672573&tree=Mozilla-Inbound

39796 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | Intl should  be defined on the global scope
39853 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | The following interface(s) are not enumerated: Intl - got 1, expected 0
Comment 5 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-26 15:00:08 PDT
Ugh.  Stupid test is stupid, again.

I'll try to cons up a backout on the MSP->SFO flight, then post the backout patch to the bug.  It's now midnight London time, and it'll be even later in Mountain View when I get there, so no chance of me even considering pushing it myself.  But perhaps someone else can try/push it for me.
Comment 6 User image Ryan VanderMeulen [:RyanVM] 2013-10-26 15:01:47 PDT
FWIW, I would just land it on Aurora post-merge personally. There's a reason why we embargo updates for a few days post-merge. Seems much cleaner that way and trunk history isn't needlessly polluted.
Comment 7 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-26 21:17:03 PDT
Didn't know that was an option!  That sounds good to me.  I'll not rush so much to post a possible patch, then.  :-)  Tomorrow or Monday.
Comment 8 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-28 17:40:10 PDT
Created attachment 823700 [details] [diff] [review]
Aurora-targeted backout

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Intl landing, but ICU not being up-to-date enough to ship yet
User impact if declined: exposing a feature before it's totally ready
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?tree=Try&rev=7946cffd62d2
Risk to taking this patch (and alternatives if risky): just flipping a build flag, low risk
String or IDL/UUID changes made by this patch: N/A
Comment 9 User image Nicolas B. Pierron [:nbp] 2013-10-28 17:44:27 PDT
Comment on attachment 823700 [details] [diff] [review]
Aurora-targeted backout

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

Sounds good.
Comment 10 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-28 21:41:41 PDT
Commit message bits could have been updated further as this landed post-uplift, but oh well.  :-)

https://hg.mozilla.org/releases/mozilla-aurora/rev/d49507e7e118
Comment 11 User image Loic 2013-10-29 05:13:45 PDT
*** Bug 932244 has been marked as a duplicate of this bug. ***
Comment 12 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-11-12 17:20:41 PST
Created attachment 831207 [details] [diff] [review]
Partial patchwork such that update-icu.sh would update everything as desired

This is -- in theory -- an approximately working patch to get you to the point where, if you ran update-icu.sh with 52.1's URL, you'd get something that almost worked.  It doesn't work for me locally using clang (the same old __float128 issue that's plagued everyone trying to use clang -std=c++0x forever with new-enough libstdc++ headers), and it might not work a bunch of other places.  But it is maybe at least plausibly somewhat usable, or close-ish to it.

(No results yet regarding the issue Norbert ran into that prevented updating ICU earlier, for what it's worth.)
Comment 13 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-11-13 11:40:34 PST
Hacking intl/icu/source/tools/toolutil/toolutil.cpp to put the #include "unicode/utypes.h" at the top, and adding -D__STRICT_ANSI__ when adding -std=c++0x to the commandline, makes the build continue past the __float128 sadness.  Both changes will have to be patches atop ICU's source, alas, but I don't see a way around that.  Once we've got things landed here, then I'll look at how to upstream that somehow.

After __float128 I hit <http://bugs.icu-project.org/trac/ticket/10043>.  Given that's in my awesomebar, it seems very likely it's the error that was hit that prevented updating ICU just after it was first landed.  :-)  There's been some activity in that ticket since 52.1 was released, and comments that discuss the ultimate issue, so perhaps I can get a patch going there.  Investigating that now.
Comment 14 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-06 22:02:04 PST
I have a patch stack, locally, that I *think* fixes http://bugs.icu-project.org/trac/ticket/10043 and successfully compiles the JS engine.  But on starting it:

[jwalden@find-waldo-now dbg]$ ./js
./js: error while loading shared libraries: libicui18n.so.52: cannot open shared object file: No such file or directory

I have a strong suspicion that this was caused by my patches not being aware of bug 912371, or possibly bug 915735, as I think (if memory serves) my patchwork here touched somewhat at the edges some of the js/src/{configure.in,Makefile.in} bits there.  Hopefully that won't be too bad to sort through.

It's gotten late enough now, tho, that I probably should punt that to tomorrow morning, to ensure I can best focus on that examination.  Still hoping to get this done enough to land before uplift, but we'll see.
Comment 15 User image Till Schneidereit [till] 2013-12-07 02:39:25 PST
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> I have a strong suspicion that this was caused by my patches not being aware
> of bug 912371, or possibly bug 915735, as I think (if memory serves) my
> patchwork here touched somewhat at the edges some of the
> js/src/{configure.in,Makefile.in} bits there.

In case you haven't figured that out by now: it's almost certainly caused by bug 947299 and hence unrelated to your changes.
Comment 16 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:01:32 PST
(In reply to Till Schneidereit [:till] from comment #15)
> In case you haven't figured that out by now: it's almost certainly caused by
> bug 947299 and hence unrelated to your changes.

Oddment, I could have sworn I popped all my changes and got a tree/build that *did* work, implicating exactly my changes.  But in the light of day today, with a tree updated through those changes, it wasn't showing.  Meh.

I have a patch stack, will upload posthaste for review.  There is still hope for uplift!

https://tbpl.mozilla.org/?tree=Try&rev=16eb2c458cd5
Comment 17 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:02:43 PST
Created attachment 8344290 [details] [diff] [review]
irrelevant-mutex-fixup.diff

This locally-applied patch no longer matters upstream, per patch summary.  Remove it.
Comment 18 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:04:29 PST
Created attachment 8344291 [details] [diff] [review]
bsd-libname-extra-s.diff

Ditto for this BSD libname patch, again see summary.  (Whoever gets to this review first, just take it -- trying to spread the burden if I can, where I can.  Not that it likely matters for these super-small changes, but oh well.)
Comment 19 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:05:09 PST
Created attachment 8344292 [details] [diff] [review]
zero-sigfigs.diff

Another patch already upstream, per patch summary.
Comment 20 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:05:57 PST
Created attachment 8344293 [details] [diff] [review]
bsd-truncate.diff

The BSD truncate symbol-clash is no more in 52.1, with that patch already existing upstream.
Comment 21 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:10:27 PST
Created attachment 8344294 [details] [diff] [review]
update-icu.diff

Temporarily change update-icu.sh to act to download all the changes, copy them into the tree, then hg addremove, then quit.  (Those changes will be in their own attachment here, provided I can actually upload a ~3.6MB attachment.)  This provided a solid base of updated ICU code, on which I could rebase all our locally-applied patches, and add new patches for new issues that manifested.
Comment 22 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:13:12 PST
Created attachment 8344296 [details]
import-changes.diff.gz

This is a gzipped patch containing all the changes made by running

  ./update-icu.sh http://source.icu-project.org/repos/icu/icu/tags/release-52-1/

in intl/ with all prior patches here applied.  (I don't expect it can reasonably be reviewed, but I'm uploading it for completeness.)

After this change and the previous ones, intl/icu is entirely pristine ICU source.  What remains is to rebase our local patches, add a couple new ones, then reach a point where running the above command makes no changes whatsoever to our tree.
Comment 23 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:15:48 PST
Created attachment 8344297 [details] [diff] [review]
bug-899722-4

This updates and rebases the bug-899722-4 local patch (to not override CC/CXX on BSDs) to work against ICU 52.1 code.  Other patches in that bug have landed upstream, but this one still appears necessary.
Comment 24 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:21:06 PST
Created attachment 8344299 [details] [diff] [review]
bug-724533

Update the MSYS/MSVC ICU build work to apply to 52.1.  No complicated changes to work through here, particularly, but this was definitely a bear to port forward.
Comment 25 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:24:15 PST
Created attachment 8344300 [details] [diff] [review]
icu-cxxox.diff

And now, the two new local patches we need.

Right now, ICU's build system, in concert with libstdc++ C++11 headers, runs afoul of clang not supporting __float128.  To avoid this problem, we have to compile with -D__STRICT_ANSI__ or however it's called.  It's fairly trivial to insert that into ICU's build system in the appropriate places to make things compile with clang.

This patch can and should be upstreamed, and is probably 95% of the way to being upstream-acceptable, but I haven't done so because of focus on work here just yet.  I'll do that once we have all this landed.
Comment 26 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:48:00 PST
Created attachment 8344301 [details] [diff] [review]
genrb-omitcollationrules.diff

This is that tricky bit for http://bugs.icu-project.org/trac/ticket/10043 that Norbert was running into when he was trying to update ICU back seven or eight months ago.

The ICU build compiles a C++ tool called genrb.  genrb is used to convert text files describing locale data into binary files that describe that data more compactly (but less readably).  These binary files are generated with respect to two options: -k to parse the incoming data strictly, and -R to omit collation tailoring rules from the generated output.  (Collation tailoring data takes up a lot of space would be unused, so we don't want to ship it.)  Upstream, it turns out, *broke* the -R option between 50 and 52.1.

The problem, as the ICU ticket notes, is that the genrb executable stores the -R flag's presence in a static global boolean, initialized before any parsing happens.  That boolean is consulted in several different places in the parser, and it's used to short-circuit parsing in those cases.

There's a certain syntax in the text-to-be-parsed that lets locale data refer to/import another locale's data, or a portion of it.  Naturally, the same code is used to parse that locale's data, as is used to parse the top-level locale's data.  The problem is that the output data sometimes needs to be computed with respect to the collation rules in that imported data.  But if -R is passed in, then parsing that sub-import will (because of the static global) *also* omit collation rules.  What's desired, then, is a way to omit collation rules only for the top parse, but not for any parses of referred-to imports.

The trick, then, is to change the parse() function, and all the functions underneath it, to take a |UBool omitCollationRules| argument.  The top-level call will pass in a value determined by the commandline arguments.  Functions underneath it will pass along the value provided to them.  And in the referred-to parse() call, we pass along an explicit |FALSE| to have collation rules included.  The results of this parse aren't directly present in the generated output, so it's not a problem to generate/track collation rules here.

I'm really not sure who should be reviewing this.  Please punt if you have better ideas!

This is the second extra local patch.  Because it touches a bunch of function signatures, and passes around bools a bit more, I am less certain that it's close to what upstream necessarily would want.  But it appears to be correct, by this technique:

  * start with a clean tree, build it
  * ls -s path/to/star-dot-res-files/ > /tmp/clean.txt
  * apply all patches here
  * hg revert -rqparent intl/icu/source/data
    (which reverts all data files to the ICU 50 data)
  * build
  * ls -s path/to/star-dot-res-files/ > /tmp/retro.txt
  * diff -U 2 /tmp/clean.txt /tmp/retro.txt

then I get a super-minimal diff between the two, indicating the same basic data was generated.  (The handful of differences I see, I think are because of new data files in ICU 52.1, or data files in ICU 50 that aren't in 52.1.)  That indicates to me that the same data has been generated.  I also did some spot-checks of files, using the underlying genrb command as invoked by make.  The file ko.txt contains a whole bunch of collation stuff in it, and the genrb output I get with and without -R in a clean tree, matches in size the output I get in a tree with the full patch sequence here applied to it.

Thus I'm reasonably confident this is correct.  But, given it's passing along previously-global values a bit, there's a change it's not the exact way upstream would want.  (The http://bugs.icu-project.org/trac/ticket/10043#comment:8 comment suggests that maybe caching gOmitCollationRules at the start of the referenced method, setting it to false, then restoring it at the end of the method, might work too.  This feels maybe more hackish to me.  Maybe.)  I think this does what we want, but it may not be exactly what will be taken upstream.  But that seems like something we can iron out after this lands.
Comment 27 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-07 23:55:08 PST
Created attachment 8344302 [details] [diff] [review]
fix-update.diff

This removes the hg addremove/early exit in update-icu.sh that were temporarily added in update-icu.diff earlier.

With this patch in place, running

  ./update-icu.sh http://source.icu-project.org/repos/icu/icu/tags/release-52-1/

in intl/ makes no changes to the tree, meaning that we again have a perfectly replayable ICU import.

And with that, we are *almost* done here.  I see the try-push in https://tbpl.mozilla.org/?tree=Try&rev=16eb2c458cd5 has progressed to a point that indicates that Windows has trouble with this patch stack.  I don't know what's up there:

/usr/local/bin/make[5]: Making `all' in `stubdata'
make[6]: Entering directory `/c/builds/moz2_slave/try-w32-d-00000000000000000000/build/obj-firefox/js/src/intl/icu/target/stubdata'
generating dependency information for c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/intl/icu/source/stubdata/stubdata.c
make[6]: Leaving directory `/c/builds/moz2_slave/try-w32-d-00000000000000000000/build/obj-firefox/js/src/intl/icu/target/stubdata'
make.py[7]: Entering directory 'c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\js\src\ctypes\libffi'
make[6]: Entering directory `/c/builds/moz2_slave/try-w32-d-00000000000000000000/build/obj-firefox/js/src/intl/icu/target/stubdata'
cl -DU_USING_ICU_NAMESPACE=0 -DU_NO_DEFAULT_INCLUDE_UTF_HEADERS=1 -DUCONFIG_NO_LEGACY_CONVERSION -DUCONFIG_NO_TRANSLITERATION -DUCONFIG_NO_REGULAR_EXPRESSIONS -DUCONFIG_NO_BREAK_ITERATION    -DU_DEBUG=1 -DHAVE_DLOPEN=0 -DU_HAVE_ATOMIC=0 -DU_HAVE_MMAP=0 -DU_HAVE_INTTYPES_H=0 -DU_HAVE_DIRENT_H=0 -DU_HAVE_POPEN=0 -DU_HAVE_TZNAME=0 -DU_HAVE_WCSCPY=0  -DU_RELEASE=1 -D_DEBUG=1 -D_CRT_SECURE_NO_DEPRECATE -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/intl/icu/source/common  -DU_ATTRIBUTE_DEPRECATED= -DWIN32 -DCYGWINMSVC -TC -nologo -W3 -Gy -Fd -wd4244 -we4553 -Zi -O2 -MDd -W4 -W4   -GF -nologo -c   -Fostubdata.o 
make[6]: Leaving directory `/c/builds/moz2_slave/try-w32-d-00000000000000000000/build/obj-firefox/js/src/intl/icu/target/stubdata'
cl : Command line warning D9025 : overriding '/W3' with '/W4'
cl : Command line error D8003 : missing source filename
make[6]: *** [stubdata.o] Error 2
make[5]: Leaving directory `/c/builds/moz2_slave/try-w32-d-00000000000000000000/build/obj-firefox/js/src/intl/icu/target'
c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\js\src\Makefile:238:0: command 'C:/mozilla-build/msys/local/bin/make.exe -j1 -C intl/icu/target STATIC_O=obj GENRBOPTS='-k -R'' failed, return code 2
make[5]: *** [all-recursive] Error 2

The bit about "/W4" does seem to implicate changes here, tho, based on my memory of there being a -W4 in one of the patches here (probably bug-724533, attachment 8344299 [details] [diff] [review]).  Ideally someone looking here can figure it out before I get back to this.  :-)  But if not, I'll do my best to investigate and figure it out tomorrow.  I expect the delta to fix that will be almost nothing.

And with that, the full patch stack to update ICU is uploaded, except for that remaining niggly bit to make Windows work.  Almost!
Comment 28 User image Landry Breuil (:gaston) 2013-12-08 00:25:06 PST
Comment on attachment 8344297 [details] [diff] [review]
bug-899722-4

Yes it seem that part was never pushed upstream..
Comment 29 User image Norbert Lindenberg 2013-12-08 11:35:35 PST
The try run in comment 16 shows two jsreftest failures that appear to be caused by locale data changes in ICU 52:

Intl/DateTimeFormat/format.js
TypeError: Assertion failed: got "๑๒ ธันวาคม ๒๕๕๕ ๐๓:๐๐:๐๐", expected "๑๒ ธันวาคม ๒๕๕๕, ๐๓:๐๐:๐๐ item 1

Intl/NumberFormat/format.js
TypeError: Assertion failed: got "-$1.00", expected "($1.00) item 1

I've verified that the locale data in CLDR 24 has changes compared to CLDR 23 that remove the comma between date and time in Thai, and change the default currency format for negative values in English from the parenthesized form to the one using the minus sign (the parenthesized form is available separately as an "accounting" variant).

Since the tests serve to verify that Firefox calls ICU correctly and gets the results that ICU should return for the given parameters, and not to freeze localized behavior, the two tests should be updated to expect the new output.
Comment 30 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-08 11:58:18 PST
Yeah, I noticed those looking over results this morning.  I tried to get a patch together for that to flag you for comment on it, and partially succeeded, except that I couldn't for the life of me figure out how to change the test to expect the new output.  My bidi Jordanian typing skills are totally not up to snuff.  (I managed to update the expected results for the first failure below only using a hex editor.  :-\  I still haven't updated the second bit below yet -- probably will have to resort to a hex editor there as well.)

It turns out the NumberFormat.js issue flagged there isn't the only issue in that test -- looks like that failure hides subsequent failures in this bit of that test:

  // Locale ar-JO; currency JOD.
  // The Jordanian Dinar divides into 1000 fils. Jordan uses (real) Arabic digits.
  format = new Intl.NumberFormat("ar-jo", {style: "currency", currency: "JOD"});
  assertEq(format.format(0), ...);
  assertEq(format.format(-1), ...);
  assertEq(format.format(123456789.123456789), ...);

The specific extra failures are:

Intl/NumberFormat/format.js:35:0 Error: Assertion failed: got "\u200F-\u062F.\u0623.\u200F\xA0\u0661\u066B\u0660\u0660\u0660", expected "\u062F.\u0623.\u200F\xA0\u0661\u066B\u0660\u0660\u0660-"
Intl/NumberFormat/format.js:36:0 Error: Assertion failed: got "\u062F.\u0623.\u200F\xA0\u0661\u0662\u0663\u066C\u0664\u0665\u0666\u066C\u0667\u0668\u0669\u066B\u0661\u0662\u0663", expected "\u062F.\u0623.\u200F\xA0\u0661\u0662\u0663\u0664\u0665\u0666\u0667\u0668\u0669\u066B\u0661\u0662\u0663"

Do those new outputs look correct?

(Of course I could update the expected outputs to be in the Unicode-escaped formats.  But given that's approximately as opaque as the other way to me, and the other way is probably less opaque to someone who understands Jordanian number formats, I really don't want to do that.)
Comment 31 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-08 12:36:45 PST
Created attachment 8344383 [details] [diff] [review]
Now in patch form, after further resort a hex editor
Comment 32 User image Norbert Lindenberg 2013-12-08 14:04:33 PST
Comment on attachment 8344383 [details] [diff] [review]
Now in patch form, after further resort a hex editor

I've verified that the additional changes for Arabic also come from CLDR 24: It was changed to place the minus sign for negative values at the logical start rather than the logical end of numeric strings, prefixing it with the right-to-left mark for correct layout, and added grouping separators to decimal numbers.

Editing the Arabic strings is indeed tricky, especially now that CLDR is using invisible layout control characters (RLM). But you got it right.

In the checkin comment, I'd refer to CLDR rather than ICU, since the changes come from CLDR: "Updated jstests that depend on CLDR locale data to match CLDR 24."
Comment 33 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-08 23:22:20 PST
Possible/likely fix for the Windows bustage, which appears to have been a mistake during patch-porting on my part:

https://tbpl.mozilla.org/?tree=Try&rev=3609b1a9851f
Comment 34 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-09 00:21:07 PST
Created attachment 8344519 [details] [diff] [review]
bug-724533

The previous patch's problem was that it didn't change $(SOURCE_FILE) to $< in all the ICU build rules in mh-msys-msvc.  With those changed (they were changed before, my previous patch just lost those changes), in the try-push above we get into the meat of compiling ICU on Windows.  (It doesn't finish, but that too looks like a trivial issue.)
Comment 35 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-09 00:35:07 PST
Created attachment 8344521 [details] [diff] [review]
upstream-windows-using-icu-fix.diff

ICU 52.1 released with Windows-only bustage if you #define U_USING_NAMESPACE 0.  Upstream's http://bugs.icu-project.org/trac/ticket/10486 is applied here (not that it wasn't fairly obvious at a glance what the issue was, but good to get the confirmation) and will probably make the Windows build succeed.

Try push with the last few changes made:

https://tbpl.mozilla.org/?tree=Try&rev=aa5c157771ab

Interestingly, running the update-icu.sh command no longer produces a tree identical to the tree with all these patches applied.  intl/icu/SVN-INFO now has s/Revision: 34717/Revision: 34719/, which I assume is because ICU's repository had changes made to it in unrelated paths.  (The Last Changed * items in SVN-INFO suggest that the tags/release-52-1 path hasn't changed, right?  That's all we care about here.)  In a future bug we probably should grep out the Revision: line when writing to SVN-INFO to avoid SVN-INFO changes here every time ICU trunk changes.
Comment 36 User image Norbert Lindenberg 2013-12-09 10:51:43 PST
Comment on attachment 8344521 [details] [diff] [review]
upstream-windows-using-icu-fix.diff

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

::: intl/icu/source/common/umutex.h
@@ +320,4 @@
>  
>  
>  typedef struct UMutex {
> +    U_NAMESPACE_QUALIFIER UInitOnce         fInitOnce;

This fix was proposed by the bug reporter of
http://bugs.icu-project.org/trac/ticket/10486
but the ICU team actually applied a different fix
http://bugs.icu-project.org/trac/changeset/34586

Can we take the fix made in ICU?
Comment 37 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-09 11:11:36 PST
Created attachment 8344786 [details] [diff] [review]
upstream-windows-using-icu-fix.diff

I could have sworn I did exactly what upstream had done.  But maybe I'm just imagining I went any further in investigation than looking at the revision history for that file on trunk, observing the ticket, then assuming the patch there was what was actually applied.  :-\  Oh well.
Comment 38 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-09 18:51:20 PST
Created attachment 8345073 [details] [diff] [review]
Aurora-targeted patch: disable the Intl API in release (i.e. in beta/final release, but *not* in aurora)

Until the previous patches are reviewed and landed on trunk, and a discussion about backporting to aurora has happened, here's the current plan for what to do about Intl not on mozilla-central: enable Intl and all that only in non-release builds.  This would mean Intl would be built and exposed on aurora with ICU 50 behind it, but when it graduates to beta (and then to release after that) Intl will be disabled completely.

This patch implements this plan.  By itself it passes that one dom/tests/mochitest/general/test_interfaces.html test, and Intl is exposed in the web console when tested manually.  I also manually edited js/src/config/milestone.txt, config/milestone.txt, and browser/config/version.txt to be "28.0.0", and in that configuration Intl wasn't present in the web console when tested manually.  So I think this is good to land now, and it should work as expected when uplift to beta happens, and it should work as expected when the final release rolls around.
Comment 39 User image Mike Hommey [:glandium] 2013-12-16 16:25:31 PST
Comment on attachment 8344300 [details] [diff] [review]
icu-cxxox.diff

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

::: intl/icu-patches/icu-cxxox.diff
@@ +43,5 @@
> +         if [[ $cxx11_okay = yes ]]; then
> +-            AC_MSG_NOTICE([Adding CXXFLAGS option --std=c++0x])
> +-            UCONFIG_CXXFLAGS="${UCONFIG_CXXFLAGS} --std=c++0x"
> ++            AC_MSG_NOTICE([Adding CXXFLAGS option --std=c++0x -D__STRICT_ANSI__=1])
> ++            UCONFIG_CXXFLAGS="${UCONFIG_CXXFLAGS} --std=c++0x -D__STRICT_ANSI__=1"

It seems to me -D__STRICT_ANSI__=1 should be added to CXXFLAGS when we invoke ICU's configure, rather than changing the configure script. Except if you expect this patch to land upstream. Consider this a r+ if the latter.
Comment 40 User image Mike Hommey [:glandium] 2013-12-16 16:47:36 PST
Comment on attachment 8344301 [details] [diff] [review]
genrb-omitcollationrules.diff

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

Can you find someone upstream to review this? even if it's not something they'd take in their codebase.
Comment 41 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-17 16:09:37 PST
Comment on attachment 8345073 [details] [diff] [review]
Aurora-targeted patch: disable the Intl API in release (i.e. in beta/final release, but *not* in aurora)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: exposure of Intl functionality using an older ICU than latest, with any issues that may have been fixed in newer ICU
Testing completed (on m-c, etc.): shipping without ICU is status quo now, this just continues that
Risk to taking this patch (and alternatives if risky): nothing more than our own tests/browser frontend using Intl, but any such instances should be easily fixt (and we haven't announced this as a dependable thing yet, so it's unlikely there are any such things)
String or IDL/UUID changes made by this patch: N/A
Comment 42 User image Ryan VanderMeulen [:RyanVM] 2013-12-19 06:55:11 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1aa4c58f887
Comment 43 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-03 15:20:43 PST
Returning back to this in the new year to get 'er done...

Something between when I wrote these patches and now *appears* to indicate that the icu-cxx0x.diff patch is no longer needed for me to build with clang.  I'm not sure why.  I *had* planned to upstream that, but seeing as it appears to not be needed now, I'm not going to make the effort (which I believe is still necessary -- I think it's a Mozilla build system change that happened to unintentionally fix it) until this bug's closed out.

Which I think leaves only the genrb issue to get through upstream's process (or at least approval, even if they want a different patch for some reason) in order to land all this stuff.
Comment 44 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-03 15:49:12 PST
Created attachment 8355691 [details] [diff] [review]
Make update-icu.sh behave timezone-agnostically

Interesting.  Turns out svn behaves differently in different time zones wrt checkouts and such, so running it in Mountain View, versus running it in Chicago, produces different diffs.  This patch adds an |export TZ=UTC| to that script to get consistent behavior -- a trivial change, just mentioning it here as something I'll be including in the ultimate push here.
Comment 45 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-03 17:25:04 PST
I posted the genrb-related patch in ICU's tracking system -- we'll see what the response is regarding getting it reviewed and landed (probably on trunk, but this is small enough I don't think we need to worry about maintaining our own copy of it locally for the moment).

With this in steady state waiting on the ICU process, I'll probably take a look into what else in Internationalization land needs updating since its last updates from April or so.  https://wiki.mozilla.org/User:Waldo/Internationalization_API has documentation of this, that needs to be moved to a more permanent location at some point, but it'll work as a location until we have Intl in a state where we don't intend to turn it off again, but let it ride the trains into release.  Separate bugs for that work, of course (some of which I believe have already been filed, actually, just haven't been a priority yet given this to do first).
Comment 46 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-22 12:05:41 PST
The ICU patch has been reviewed and landed upstream.  My laptop's on the fritz right now and needs backup and fixing, so I can't push stuff immediately.  But once I get the backup done, and once I can transfer that backup stuff over to a machine from which I can push (probably tomorrow or Friday), I think I can push everything here to update ICU.  At that point there'll be one or two niggling bits of Intl-related updating to do, then we should be good to ship as far as I can tell.
Comment 47 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-22 12:06:09 PST
http://bugs.icu-project.org/trac/changeset/34874 for the upstream landing, because it's not mentioned in the upstream bug and is moderately hard to find trawling through Trac.
Comment 48 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-28 15:55:00 PST
The upstream patch for the ICU ticket is different from what I'd written, and shorter.  I'm looking into it to understand it, then once I do that (and perhaps have someone rubberstamp the upstream import, although it's potentially a pro forma gesture) I think this is pretty much good to land.  This will happen before the end of this week, for sure!
Comment 49 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-28 16:18:53 PST
Try run in progress:

https://tbpl.mozilla.org/?tree=Try&rev=e57bf6ee8858

The final push will be the relevant portions of that, probably all combined into a single patch.  Still need to examine the genrb-upstreamed bit of that somewhat, otherwise there is stampage on everything.
Comment 50 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-28 18:55:24 PST
Created attachment 8367088 [details] [diff] [review]
ignore-makebuild.diff

Don't touch intl/icu/{moz.build,Makefile.in}.
Comment 51 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-28 18:56:11 PST
Second try: https://tbpl.mozilla.org/?tree=Try&rev=eda7d22b6277
Comment 52 User image Mike Hommey [:glandium] 2014-01-28 19:13:27 PST
Comment on attachment 8367088 [details] [diff] [review]
ignore-makebuild.diff

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

::: intl/update-icu.sh
@@ +42,5 @@
> +# independent of this script.
> +hg revert ${icu_dir}/Makefile.in
> +if hg diff ${icu_dir}/moz.build | grep --quiet --regexp=''; then
> +  hg revert ${icu_dir}/moz.build
> +fi

I'd rather avoid relying more on hg than hg addremove for such update scripts.
Instead, you can remove everything from $icu_dir except Makefile.in and moz.build, and svn export --force, which will keep those files.

Or, we could just svn export $1/source. It doesn't seem we need the other directories/files (except maybe license.html, although we do have the license info elsewhere)
Comment 53 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-29 01:58:46 PST
I can probably make those changes.  (I don't know the svn UI all that well to even have known these things existed too much.  Time to learn a little about some corners, looks like.)  But isn't this level of update-script internal aesthetics kind of more bikeshedding than anything else?  I confess to not understanding at all why your scheme is objectively better than the scheme in the patch.
Comment 54 User image Mike Hommey [:glandium] 2014-01-29 14:28:20 PST
Eventually, I want to consolidate those update scripts, and it's easier if they don't do too fancy things.
Comment 55 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-29 16:18:04 PST
Created attachment 8367697 [details] [diff] [review]
Okay, I *think* this is what was meant/discussed on IRC
Comment 56 User image Mike Hommey [:glandium] 2014-01-29 16:44:28 PST
Comment on attachment 8367697 [details] [diff] [review]
Okay, I *think* this is what was meant/discussed on IRC

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

As a followup, you can move update-icu.sh and the patches under intl/icu.

::: intl/update-icu.sh
@@ +16,5 @@
>  icu_dir=`dirname $0`/icu
> +
> +# Remove intl/icu/source, then replace it with a clean export.
> +rm -rf ${icu_dir}/source
> +svn export --force $1/source/ ${icu_dir}/source

You don't need --force here :)
Comment 57 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-29 17:11:20 PST
Two little changes to the update script for better usability by anyone anywhere -- print UTC-style times, don't write ICU's upstream *overall* revision in intl/icu/SVN-INFO because it's not meaningful (only the revision of the subtree we're using, which is stable because stable branch, matters):

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b0697637ef5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4be2e32a9be

I looked at the ICU upstream genrb change enough to understand it, it's doing things rather more sanely than my attempt.  :-)  I guess the import-of-upstream's-patch patch should be posted for a stamp, then with that done this can all land.
Comment 58 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-29 17:22:20 PST
Created attachment 8367735 [details] [diff] [review]
genrb-upstreamed

Pure upstream import.  Summary follows for interest/demonstration of understanding.  (Arguably maybe upstreamed patches should just be takable without review, but that sits kind of weird with me.)

Some of genrb's parses of files (notably the top one, depending) should have collation information in the computed data.  Some of them shouldn't (nested parses triggered by an include-by-reference construct, not used in their entirety by the top-level parse call).  There's a ParseState struct, populated by the relevant call to parse(), that's used internally to pass around this sort of determined-by-the-call-to-parse behavior.  The upstream patch adds a field to ParseState to track should-include-collation-info.  Then it populates it by changing parse()'s signature to take a UBool indicating whether collation rules are desired.  The rest is making sure the top-level parse() is determined by the commandline argument, and making the nested one pass along FALSE to include collation info.  Simple enough, and clearly better than changing ever parsing function to take an extra argument concerning collation rule stuff, that almost always gets forwarded directly from the caller's identical argument.
Comment 59 User image Norbert Lindenberg 2014-01-29 19:25:57 PST
Comment on attachment 8367735 [details] [diff] [review]
genrb-upstreamed

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

I checked that the change in ICU changeset 34874 was correctly ported. There was another changeset for this bug, 34622, which improves the reporting of the kind of errors that this changeset prevents - I think we can live without that.
Comment 60 User image Landry Breuil (:gaston) 2014-01-30 00:24:23 PST
An icu large update just some days before a merge to aurora ? you guys are scaring me :)
Comment 62 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-30 15:22:42 PST
Final rollup patch, individual reviewers called out in the extended commit message, individual components mentioned as living here for anyone wanting to review them:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e956de758ce

ICU's configure stuff isn't quite notified correctly when the update happens -- thus a CLOBBER was required to make this work.  I filed bug 966038 to fix this build system bug.
Comment 64 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-01-31 15:39:13 PST
Relanded, with xperf stuff fixt:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2efbd5388bc7
Comment 66 User image Ryan VanderMeulen [:RyanVM] 2014-02-04 11:34:47 PST
https://hg.mozilla.org/mozilla-central/rev/d0f1ccbc7cfc
Comment 67 User image Mike Hommey [:glandium] 2014-09-22 17:05:11 PDT
Comment on attachment 8344301 [details] [diff] [review]
genrb-omitcollationrules.diff

That was upstreamed in some way, and made its way back to us, right?
Comment 68 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-09-22 17:11:47 PDT
It was upstreamed, they wrote a better fix that's what we imported locally.  That better fix hasn't made its way back yet because I haven't yet taken the time to update ICU.  :-(

Note You need to log in before you can comment on or make changes to this bug.