1833064 - thunderbird 114 build failure with system ICU

Status: RESOLVED FIXED

(Thunderbird :: Build Config, defect)

Description

[Daniel Kolesa]

Steps to reproduce:

Built Thunderbird 114.0b1, Clang+LLD 16, Rust 1.69, Linux, any architecture (tested ppc64le and x86_64), ThinLTO, musl libc, but none of these things should probably matter much. Tested ICU 72 and 73 with same results.

Actual results:

Link error:

47:28.72 ld.lld: error: undefined hidden symbol: icu_73::VTimeZone::createVTimeZoneByID(icu_73::UnicodeString const&)
47:28.72 >>> referenced by TimezoneDatabase.cpp:79 (./comm/calendar/base/src/TimezoneDatabase.cpp:79)
47:28.72 >>> lto.tmp:(TimezoneDatabase::GetTimezoneDefinition(nsTSubstring<char> const&, nsTSubstring<char>&))
47:28.72 ld.lld: error: undefined hidden symbol: icu_73::VTimeZone::write(icu_73::UnicodeString&, UErrorCode&) const
47:28.72 >>> referenced by TimezoneDatabase.cpp:102 (./comm/calendar/base/src/TimezoneDatabase.cpp:102)
47:28.73 >>> lto.tmp:(TimezoneDatabase::GetTimezoneDefinition(nsTSubstring<char> const&, nsTSubstring<char>&))
47:28.73 >>> did you mean: icu_73::VTimeZone::write(double, icu_73::UnicodeString&, UErrorCode&) const
47:28.73 >>> defined in: /lib/libicui18n.so

Expected results:

The linking should have succeeded. As far as I can tell, the issue is that the C++ API inside vtzone.h is not exported properly (U_I18N_API is U_IMPORT at the time the class is declared). That means the symbols are treated as having hidden visibility, so the linkage fails (as they are actually exported/visible symbols in libicui18n.so.73). I verified this by adding #define U_I18N_API U_EXPORT before the vtzone.h include, which resulted in a successful build. Of course, this is not the right solution, and the issue should be fixed elsewhere. I'm not quite sure where though, as I am not familiar enough with the Mozilla build system to say for sure, while this was a foolproof way for me to verify.

Commenting out the implementation of GetTimezoneDefinition in the affected source file also allowed the build to succeed.

Comment 1

[Magnus Melin [:mkmelin]]

Can you build Firefox? (I'd suspect this bug is in shared code.)

Comment 2

[alice]

the firefox dev edition of 114 beta 1-3 (and stable, esr, ..) builds for me in more or less the exact same configuration (same dep versions, arch, musl, ..), so this doesn't seem to be shared.

afaict the ./comm/calendar folder is not built for the firefox application target, just thunderbird(?)

Comment 3

[Rob Lemley [:rjl]]

I don't see how this would be a bug in shared code. I can confirm this happens when building with --with-system-icu using icu 72.1-2 (Arch).

TimezoneDatabase.cpp is relatively new, and literally the only file that does anything with VTimeZone in comm-central and mozilla-central inclusively.

Since the copy of icu in Gecko is the same as my system icu, I suspect there's something to adjust in the build system somewhere... not sure what right now though. Might not get back to this for a little while unfortunately.

It's possible that Fedora or Debian have patched something in their packages to deal with this?

Comment 4

[Sean Burke [:leftmostcat]]

Fedora doesn't use system ICU. I haven't looked at Debian's packages. Part of the problem here is that we have an in-tree patch to work around an ICU bug. The other part is that the Mozilla build system apparently doesn't pull in vtzone.h as a system header when building with system ICU.

We can deal with the latter part on our end. However, a version of that patch has been sitting with ICU for an extended period of time and without it, calendar functionality will be badly broken. ICU also doesn't make any ABI stability guarantees for their C++ API; Firefox works around this by only using C APIs for time zone-related queries (apparently at a performance cost) if building with system ICU, but we can't avoid the C++ API for this and that would necessitate rebuilds if there are any ABI changes.

All this to say: using system ICU is a bit of a fraught prospect at the moment, sorry.

Comment 5

[Daniel Kolesa]

Distributions generally already rebuild all revdeps on ICU version bumps, so the ABI part shouldn't really be relevant... the patch can probably be put into distribution downstreams for the time being. Is there any other issue?

Comment 6

[Rob Lemley [:rjl]]

The build system still needs to export from vtzone.h properly. A patch should hit this bug to do that momentarily. Big caveat, calendar functionality is totally broken without the patch to icu that Sean referred to.

Comment 7

[Daniel Kolesa]

yes, i already noticed calendar was kinda busted - i can pull the patch downstream if necessary

Comment 8

[Daniel Kolesa]

I rebuilt ICU with the linked patch, and launched thunderbird (compiled with the workaround in the original report) and I can confirm calendar works fine now (I'll pull in a proper patch for thunderbird once it's available to test it)

Comment 9

[Rob Lemley [:rjl]]

Phabricator is being annoying, but this is the build patch:

diff --git a/mail/app-system-headers.mozbuild b/mail/app-system-headers.mozbuild
--- a/mail/app-system-headers.mozbuild
+++ b/mail/app-system-headers.mozbuild
@@ -8,5 +8,11 @@
 
 # system_headers += [
 #    'file1.h',
 #    'file2.h',
 # ]
+
+if CONFIG['MOZ_SYSTEM_ICU']:
+    system_headers += [
+        "unicode/strenum.h",
+        "unicode/vtzone.h",
+    ]

Comment 10

[Daniel Kolesa]

works great, thanks!

Comment 11

[Rob Lemley [:rjl]]

Attachment: Bug 1833064 - Add icu headers needed for TimezoneDatabase.cpp to system headers. r=leftmostcat

Adding these two headers to the system headers list fixes a compile error when
building with --with-system-icu. Calendar functionality will be broken however
due to bug 1790071 (the upstream bug remains unfixed).

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/395313bae793
Add icu headers needed for TimezoneDatabase.cpp to system headers. r=leftmostcat

Comment 13

[Magnus Melin [:mkmelin]]

(In reply to Rob Lemley [:rjl] from comment #11)

Adding these two headers to the system headers list fixes a compile error when
building with --with-system-icu. Calendar functionality will be broken however
due to bug 1790071 (the upstream bug remains unfixed).

So how do we deal with that? That's that's the kind of thing that will cause loads of problems and obscure bugs. Do we need to make --with-system-icu a no-op until upstream is actually fixed?

Comment 14

[Daniel Kolesa]

why? it's a non-default option and the upstream binaries aren't built with it; distros tend to build with it, but they can pick up the icu patch downstream as long as upstream hasn't addressed it (distros tend to carry other patches for various things regardless)

making it a noop on thunderbird side would make yet another patch to deal with, so IMO best is to not do anything

Comment 15

[Daniel Kolesa]

the issue when icu is unpatched is simply that calendar does not work, it's immediately visible, so dunno what "loads of problems" or "obscure bugs" you mean

Comment 16

[Rob Lemley [:rjl]]

(In reply to Magnus Melin [:mkmelin] from comment #13)

So how do we deal with that? That's that's the kind of thing that will cause loads of problems and obscure bugs. Do we need to make --with-system-icu a no-op until upstream is actually fixed?

We couldn't make it a no-op anyway. That is all inherited build code from Firefox.

Calendar is completely broken without the ICU patch. It's very obvious.

As Daniel says, it's a non-default build option. It really only affects Linux distributions such as Debian that build against the system icu.

Personally, I think we should have some kind of alternative implementation to enable when building with system-icu. I have no idea what that might look like though.