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

btw, the next uplift date is October 28: https://wiki.mozilla.org/RapidRelease/Calendar

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

Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/f4bc5478669b

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

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.

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.

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.

[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 on attachment 823700 [details] [diff] [review] Aurora-targeted backout Review of attachment 823700 [details] [diff] [review]: ----------------------------------------------------------------- Sounds good.

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

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

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.

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.

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

(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

This locally-applied patch no longer matters upstream, per patch summary. Remove it.

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

Another patch already upstream, per patch summary.

The BSD truncate symbol-clash is no more in 52.1, with that patch already existing upstream.

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.

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.

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.

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.

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.

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.

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 on attachment 8344297 [details] [diff] [review] bug-899722-4 Yes it seem that part was never pushed upstream..

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.

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

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

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

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

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.

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

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.

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.

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

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.

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.

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!

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.

Don't touch intl/icu/{moz.build,Makefile.in}.

Second try: https://tbpl.mozilla.org/?tree=Try&rev=eda7d22b6277

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)

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.

Eventually, I want to consolidate those update scripts, and it's easier if they don't do too fancy things.

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

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.

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

An icu large update just some days before a merge to aurora ? you guys are scaring me :)

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

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.

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/604818812338 for this xperf failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=33842929&tree=Mozilla-Inbound

Relanded, with xperf stuff fixt: https://hg.mozilla.org/integration/mozilla-inbound/rev/2efbd5388bc7

https://hg.mozilla.org/mozilla-central/rev/2efbd5388bc7 http://mozillamemes.tumblr.com/post/53831599154/these-trains-run-on-time

Comment on attachment 8344301 [details] [diff] [review] genrb-omitcollationrules.diff That was upstreamed in some way, and made its way back to us, right?

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. :-(