Update our in-tree ICU to 58

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(8 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

3 years ago
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.
(Assignee)

Comment 2

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

Updated

3 years ago
Summary: Update our in-tree ICU to 57 → Update our in-tree ICU to 58
(Assignee)

Comment 3

3 years ago
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
(Assignee)

Comment 4

3 years ago
Posted patch bug1299615-part1.patch (obsolete) — Splinter Review
- 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.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8805285 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

3 years ago
Posted patch bug1299615-part2.patch (obsolete) — Splinter Review
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.
Attachment #8805286 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

3 years ago
Posted file bug1299615-part3.zip (obsolete) —
Actual update to ICU 58, generated with:
  ./update-icu.sh https://ssl.icu-project.org/repos/icu/icu/tags/release-58-1/
Attachment #8805287 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

3 years ago
ICU joined the Unicode consortium (http://blog.unicode.org/2016/05/icu-joins-unicode-consortium.html) and had its license changed.
Attachment #8805288 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

3 years ago
Update ICU data files to tzdata2016h (again), generated with:
  ./update-tzdata.sh -e $ICU_BUILD_DIR/bin/icupkg 2016h
Attachment #8805289 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

3 years ago
Handle new UDateFormatField enumeration members.
Attachment #8805290 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 10

3 years ago
Update expected results in test case to match ICU 58 resp. CLDR 30.
Attachment #8805291 - Flags: review?(jwalden+bmo)
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.
Attachment #8805285 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8805286 - Flags: review?(michael)
Attachment #8805286 - Flags: review?(jwalden+bmo)
Attachment #8805286 - Flags: review+
(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.
Attachment #8805288 - Flags: review?(jwalden+bmo) → review+
Attachment #8805289 - Flags: review?(jwalden+bmo) → review+
Attachment #8805290 - Flags: review?(jwalden+bmo) → review+
Attachment #8805291 - Flags: review?(jwalden+bmo) → review+
Attachment #8805287 - Flags: review?(jwalden+bmo) → review+
Attachment #8805286 - Flags: review?(michael) → review+
(Assignee)

Comment 14

3 years ago
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.
Attachment #8805285 - Attachment is obsolete: true
Attachment #8807303 - Flags: review+
(Assignee)

Comment 15

3 years ago
Update commit message for part 2 to include both reviewers. Carrying r+.
Attachment #8805286 - Attachment is obsolete: true
Attachment #8807304 - Flags: review+
(Assignee)

Comment 16

3 years ago
Update part 3 to use English for the SVN-info file, and update review marker in commit message from "r" to "rs". Carrying r+.
Attachment #8805287 - Attachment is obsolete: true
Attachment #8807305 - Flags: review+
(Assignee)

Comment 17

3 years ago
Update to ICU 58 seems to require a clobber to avoid failures in artifact builds.
Attachment #8807307 - Flags: review?(jwalden+bmo)
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.
Attachment #8807307 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 19

3 years ago
(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.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 20

3 years ago
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
Keywords: checkin-needed
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.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(andrebargull)
(Assignee)

Comment 23

3 years ago
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.
(Assignee)

Comment 26

3 years ago
(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.
Depends on: 1315986
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...
(Assignee)

Comment 31

3 years ago
(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/
Flags: needinfo?(andrebargull)
(Assignee)

Comment 32

3 years ago
(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)?

Updated

3 years ago
Depends on: 1316540

Comment 34

3 years ago
(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.
Flags: needinfo?(jwalden+bmo)
Duplicate of this bug: 1334462
You need to log in before you can comment on or make changes to this bug.