--with-system-icu build fails after bug 1315986

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jbeich, Assigned: jfkthame)

Tracking

({regression})

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 fixed, firefox53 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
Ignoring U_LB_COUNT being deprecated in 58.1 bug 1315986 enforces https://ssl.icu-project.org/trac/changeset/38753 baggage. If Unicode 9.0 is really required by Gecko, please, update build/autoconf/icu.m4

$ echo ac_add_options --with-system-icu >>.mozconfig
$ ./mach build
[...]
In file included from objdir/intl/lwbrk/Unified_cpp_intl_lwbrk0.cpp:2:
intl/lwbrk/nsJISx4051LineBreaker.cpp:557:3: error: static_assert failed "Gecko vs ICU
      LineBreak class mismatch"
  static_assert(U_LB_COUNT == mozilla::ArrayLength(sUnicodeLineBreakToClass),
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

$ pkg info -x icu
icu-57.1,1

$ fgrep -r U_LB_COUNT /usr/local/include
/usr/local/include/unicode/uchar.h:    U_LB_COUNT = 40
Assignee

Comment 1

3 years ago
Hmm... I'm not entirely sure which way to go here. We could modify the static_assert to use <= instead of ==, so that we can still build against older ICU versions; that's safe as far as the specific problem from bug 1315986 is concerned.

But we'll shortly be updating other data in Gecko to Unicode 9.0 (in bug 1281448 -- I started on a patch but ran into an unexpected issue that needs diagnosing before it's ready to land). So at that point, Gecko will expect to be supporting Unicode 9, and if we then use an older version of ICU, I'd expect to (at least) see some test failures due to the mismatch in supported versions.

Maybe people using --with-system-icu don't care if the resulting product doesn't fully implement the same Unicode version as "stock" Firefox, and as a result doesn't pass all our tests? That seems unfortunate to me, though. I'm more inclined to say we should update the requirement in icu.m4, so that we can count on a known Unicode version for a given Gecko release.
Reporter

Updated

3 years ago
Blocks: 1299615
Reporter

Updated

3 years ago
Duplicate of this bug: 1317080
Reporter

Comment 4

3 years ago
Can you also fix assert to not break with U_LB_COUNT > 43 i.e., if downstream has more up-to-date ICU than Mozilla in future ?
Assignee

Comment 5

3 years ago
(In reply to Jan Beich from comment #4)
> Can you also fix assert to not break with U_LB_COUNT > 43 i.e., if
> downstream has more up-to-date ICU than Mozilla in future ?

That would be a problem, because the gecko code wouldn't know how to handle new ULineBreak values that might be returned by a newer ICU.
Attachment #8809326 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 6

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5e948205052d195f08bff9ef5d750f15de4c9d
Bug 1316540 - Update version requirement for --with-system-icu to 58.1 for Unicode 9 support. r=waldo
Duplicate of this bug: 1298614

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa5e94820505
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee: nobody → jfkthame
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(jfkthame)
Reporter

Comment 10

3 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> (In reply to Jan Beich from comment #4)
> > Can you also fix assert to not break with U_LB_COUNT > 43 i.e., if
> > downstream has more up-to-date ICU than Mozilla in future ?
> 
> That would be a problem, because the gecko code wouldn't know how to handle
> new ULineBreak values that might be returned by a newer ICU.

Did Gecko know how to before bug 1299615 landed if built --with-system-icu ? Otherwise, bug 1315986 impacts downstream --with-system-icu where ICU >= 58.
In principle we could add #ifs testing ICU version to handle multiple variants, if it were really important.  However, such #ifs are much less maintainable than just bumping ICU version, so I'd rather avoid them.  (Also, we periodically patch ICU with patches upstream has accepted, and at that point system-icu becomes temporarily unusable anyway.)
Reporter

Comment 12

3 years ago
Bumping ICU version fixes mozilla-central. What about release branches? For one, downstream maybe be using ICU 58.1 + --with-system-icu on Firefox 50.
Unless I'm misunderstanding, the backport would only be a single branch, for an uplift that happened a week ago.  That seems little enough time that backporting further seems like the right decision.  If we'd discovered this several weeks later, it might be a different matter.
Reporter

Comment 14

3 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> Unless I'm misunderstanding, the backport would only be a single branch, for
> an uplift that happened a week ago.

Correct, bug 1316540 (this one) has to chase bug 1299615 to Aurora. If comment 5 is true for --with-system-icu *before* bug 1299615 then bug 1315986 has to chase at least bug 1265631 to Release as well. Bug 1299615 Part 6 may have similar impact.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> Also, we periodically patch ICU with patches upstream has accepted,
> and at that point system-icu becomes temporarily unusable anyway

From downstream POV backporting an upstream fix that doesn't break backward compatibility is regular occurence. What would be a problem if Mozilla forks old ICU version (like with Cairo) or exposes newer ICU to a vulnerability because forward compatibility is NPOTB.
Assignee

Comment 15

3 years ago
(In reply to Jan Beich from comment #14)
> If
> comment 5 is true for --with-system-icu *before* bug 1299615 then bug
> 1315986 has to chase at least bug 1265631 to Release as well.

Yes, I believe this is the case. If I'm following correctly, when firefox 49..51 (including bug 1265631) or later is built with ICU 58, you'll run into the crash from bug 1315986.

(Note that there might be other similar issues where using a newer ICU version than gecko was aware of could lead to problems, though I am not currently aware of other examples.)
Flags: needinfo?(jfkthame)
Assignee

Comment 16

3 years ago
Comment on attachment 8809326 [details] [diff] [review]
Update version requirement for --with-system-icu to 58.1 for Unicode 9 support

Approval Request Comment
[Feature/regressing bug #]: 1299615
[User impact if declined]: build failure using --with-system-icu if the system ICU is too old
[Describe test coverage new/current, TreeHerder]: n/a (does not affect mozilla builds)
[Risks and why]: minimal, just update --with-system-icu version requirement to match in-tree ICU version
[String/UUID change made/needed]: n/a
Attachment #8809326 - Flags: approval-mozilla-aurora?
Comment on attachment 8809326 [details] [diff] [review]
Update version requirement for --with-system-icu to 58.1 for Unicode 9 support

take this fix for --with-system-icu builds of aurora52
Attachment #8809326 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.