ICU 57 was released on 2016-03-23. ICU 58 is scheduled for release on 2016-10-01.

http://site.icu-project.org/ http://site.icu-project.org/download/58 It's October 1st, ICU 58 suppose to be released today or sometime soon, bug title needs to be changed to version 58.

Update: ICU 58 was released last Friday (2016-10-21).

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e471dde248e3e3feba8fbd378427e12149a39b Still need to find out if it's possible to disable the "bad implicit conversion constructor" checks for ICU to avoid this additional patch: https://hg.mozilla.org/try/file/9dd091592ccd/intl/icu-patches/explicit_constructor_KeywordsSink.diff

- icu-patches/icu-release-56-1-flagparser-fix.patch and icu-patches/bug-1172609-icu-fix.diff are no longer needed (fixed in ICU57 resp. ICU58) - icu-patches/bug-1228227-bug-1263325-libc++-gcc_hidden.diff needed to be updated to apply against ICU57/58 - ucol_getKeywordValuesForLocale-ulist_resetList.diff is the new patch for https://ssl.icu-project.org/trac/ticket/12827 (directly downloaded as unified patch from https://ssl.icu-project.org/trac/changeset/39484) The layout engine was removed from ICU58, so we no longer need to delete the `${icu_dir}/source/layout` directory in update-icu.sh.

In addition to ignoring any namespace starting with "icu_", it's now also necessary to ignore the "icu" directory to avoid a "bad implicit conversion constructor" error.

Actual update to ICU 58, generated with: ./update-icu.sh https://ssl.icu-project.org/repos/icu/icu/tags/release-58-1/

ICU joined the Unicode consortium (http://blog.unicode.org/2016/05/icu-joins-unicode-consortium.html) and had its license changed.

Update ICU data files to tzdata2016h (again), generated with: ./update-tzdata.sh -e $ICU_BUILD_DIR/bin/icupkg 2016h

Handle new UDateFormatField enumeration members.

Update expected results in test case to match ICU 58 resp. CLDR 30.

Comment on attachment 8805285 [details] [diff] [review] bug1299615-part1.patch Review of attachment 8805285 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/icu-patches/ucol_getKeywordValuesForLocale-ulist_resetList.diff @@ +1,1 @@ > +https://ssl.icu-project.org/trac/ticket/12827 Worth also citing https://ssl.icu-project.org/trac/changeset/39484 here as well (with a parenthetical that the test-related bits are excluded because we patch ICU *after* removing directories we don't care about)? Seems like it to me.

Comment on attachment 8805286 [details] [diff] [review] bug1299615-part2.patch Review of attachment 8805286 [details] [diff] [review]: ----------------------------------------------------------------- Plausible, but I imagine this needs a clang-plugin person's once-over.

(In reply to André Bargull from comment #6) diff --git a/intl/icu/SVN-INFO b/intl/icu/SVN-INFO --- a/intl/icu/SVN-INFO +++ b/intl/icu/SVN-INFO @@ -1,9 +1,10 @@ -Path: release-56-1 -URL: http://source.icu-project.org/repos/icu/icu/tags/release-56-1 -Repository Root: http://source.icu-project.org/repos/icu -Repository UUID: 251d0590-4201-4cf1-90de-194747b24ca1 -Node Kind: directory -Last Changed Author: mow -Last Changed Rev: 38044 -Last Changed Date: 2015-10-07 22:20:20 +0000 (Wed, 07 Oct 2015) +Pfad: release-58-1 +URL: https://ssl.icu-project.org/repos/icu/icu/tags/release-58-1 +Relative URL: ^/icu/tags/release-58-1 +Basis des Projektarchivs: https://ssl.icu-project.org/repos/icu +UUID des Projektarchivs: 251d0590-4201-4cf1-90de-194747b24ca1 +Knotentyp: Verzeichnis +Letzter Autor: srl +Letzte geänderte Rev: 39472 +Letztes Änderungsdatum: 2016-10-19 20:35:30 +0000 (Mi, 19. Okt 2016) I mean, okay, but. Could you export LANG=en_US.UTF8 or something in intl/update-icu.sh so this remains English and all, said language being what the rest of the tree is? I skimmed bits of the rest of this, but practically I think I have to just take this as it is. rs=me on part 3.

Update part 1 to include URL to ICU changeset and also change default language for intl/update-icu.sh to English. Carrying r+ from Waldo.

Update commit message for part 2 to include both reviewers. Carrying r+.

Update part 3 to use English for the SVN-info file, and update review marker in commit message from "r" to "rs". Carrying r+.

Update to ICU 58 seems to require a clobber to avoid failures in artifact builds.

Comment on attachment 8807307 [details] [diff] [review] bug1299615-part8.patch Review of attachment 8807307 [details] [diff] [review]: ----------------------------------------------------------------- Not sure CLOBBER-touches require review, exactly. But do file a Core::Build Config bug on this necessity.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18) > Not sure CLOBBER-touches require review, exactly. But do file a Core::Build > Config bug on this necessity. Filed bug 1315397.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/69147f2ddca8 Part 1: Update ICU patch files to apply cleanly on ICU58. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/8a609e18410d Part 2: Skip ICU source directory in Clang build plugin when searching for implicit conversion constructors. r=Waldo, r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/eb823a4a31f0 Part 3: Update in-tree ICU to release 58.1. rs=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/6a7c7000b047 Part 4: Change license note for ICU to point to Unicode license. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/541e579e59b3 Part 5: Update tzdata in ICU data files to 2016h. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8727f367c5 Part 6: Update ICU stub implementations for ICU 58. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/a3cc99e3ff24 Part 7: Adjust expected test result after update to ICU 58. r=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/d22677d3a37f Part 8: Clobber for ICU 58. r=Waldo

https://hg.mozilla.org/mozilla-central/rev/69147f2ddca8 https://hg.mozilla.org/mozilla-central/rev/8a609e18410d https://hg.mozilla.org/mozilla-central/rev/eb823a4a31f0 https://hg.mozilla.org/mozilla-central/rev/6a7c7000b047 https://hg.mozilla.org/mozilla-central/rev/541e579e59b3 https://hg.mozilla.org/mozilla-central/rev/9c8727f367c5 https://hg.mozilla.org/mozilla-central/rev/a3cc99e3ff24 https://hg.mozilla.org/mozilla-central/rev/d22677d3a37f

FYI, coverity found an important number of new issues in this library with today's run. I cannot tell how many of them are impacting us but I expect some regressions from this change. As 52 is an ESR, can we backout all the changes and reland next week after the merge? So that changes will be in 53.

Based on bug 1020565 comment 2, I always assumed "a bajillion Coverity warnings" are to be expected. But I'm hardly an expert on this topic, so I can't give a definite answer on how to treat the Coverity results for ICU. Does the Coverity scan take into account that we're only using a small part of the ICU API? Or does it scan the complete library?

Static analyzer tools don't have the capability to know what is used and what is not. They are analyzing everything compiled. Can we compile less stuff then ? :) I think we should investigating more in fixing issues upstream but my experience wasn't great: http://bugs.icu-project.org/trac/ticket/10881 (took forever, I wasn't credited at all for the change, etc)

I will look other this issue, my guess is that this update also consisted of a refactoring, and coverity wasn't able to match the existing bugs in the new source code, so it removed the old ones and inserted the already existing ones as "new". It's just a supposition but i will investigate this.

(In reply to Andi-Bogdan Postelnicu from comment #25) > It's just a supposition but i will investigate this. Thank you! :-)

(In reply to Sylvestre Ledru [:sylvestre] from comment #24) > I think we should investigating more in fixing issues upstream but my > experience wasn't great: I've been recently more active in filling ICU bugs because of our increased usage of their API for our Intl API, and my experience is better. I'd like to think that their processes improved over last three years, so if you'll have a motivation to give it a try, I hope you'll get better results this time :)

OK, thanks, I will give a try with one or two trivial bugs.

So my first guess was right(In reply to André Bargull from comment #26) > (In reply to Andi-Bogdan Postelnicu from comment #25) > > It's just a supposition but i will investigate this. > > Thank you! :-) I investigated this issue and my first guess was right, the files had been modified by adding a header like: >>// Copyright (C) 2016 and later: Unicode, Inc. and others. >>// License & terms of use: http://www.unicode.org/copyright.html Plus other minor differences that led Coverity unable to match existing bugs against the new code. This is strange since the chnages were not that big. An example to enforce this is thew new bug 1394243 that replaces eliminated bug 1346407. This having said i think we can eliminate for now the possibility of having introduced in the new ICU library more bugs. @Sylvestre: I think this also applies to the last week build where almost the same situation happened for SKIA.

This implicitly causes a Unicode version bump for the ICU-backed parts of Gecko, as ICU 58 supports Unicode 9.0, whereas previously we were on Unicode 8. As such, I think we really should have co-ordinated this with bug 1281448, which is about updating the Unicode version implemented elsewhere in Gecko. We shouldn't have a patchwork of mixed versions...

(In reply to Andi-Bogdan Postelnicu from comment #29) > This having said i think we can eliminate for now the possibility of having > introduced in the new ICU library more bugs. \o/

(In reply to Jonathan Kew (:jfkthame) from comment #30) > As such, I think we really should have co-ordinated this with bug 1281448, > which is about updating the Unicode version implemented elsewhere in Gecko. > We shouldn't have a patchwork of mixed versions... Sorry, I didn't know about bug 1281448 and that updating ICU or rather the Unicode version used in ICU affects other parts of Firefox. :-( Maybe the various update scripts should contain a warning message that other parts of Firefox also need to be updated when the Unicode version is bumped (js/src/vm/make_unicode.py, intl/update-icu.sh and Gecko's update script)?

Let's try here: http://bugs.icu-project.org/trac/ticket/12852

(In reply to Sylvestre Ledru [:sylvestre] from comment #24) > Static analyzer tools don't have the capability to know what is used and > what is not. > They are analyzing everything compiled. Can we compile less stuff then ? :) > > I think we should investigating more in fixing issues upstream but my > experience wasn't great: > http://bugs.icu-project.org/trac/ticket/10881 (took forever, I wasn't > credited at all for the change, etc) Sorry you had a bad experience. And thanks for the patches of course. For what it's worth, ICU4C runs an automated coverity scan weekly. http://bugs.icu-project.org/trac/ticket/12852 looks like a good bug and hopefully will get in soon.