Closed Bug 978784 Opened 6 years ago Closed 6 years ago

Stop exporting ICU symbols from xul.dll

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

Adding the U_*_IMPLEMENTATION flags to our config means that we will end up
exporting these symbols from the ICU headers included in libxul, which bloats
xul.dll without any reason.
Attachment #8384618 - Flags: review?(mh+mozilla)
Assignee: nobody → ehsan
Blocks: 915735
Comment on attachment 8384618 [details] [diff] [review]
Stop exporting ICU symbols from xul.dll

Review of attachment 8384618 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/icu.m4
@@ -123,5 @@
> -    if test -z "$MOZ_SHARED_ICU"; then
> -        AC_DEFINE(U_STATIC_IMPLEMENTATION)
> -    else
> -        AC_DEFINE(U_COMBINED_IMPLEMENTATION)
> -    fi

That part should still be needed.
Attachment #8384618 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #2)
> Comment on attachment 8384618 [details] [diff] [review]
> Stop exporting ICU symbols from xul.dll
> 
> Review of attachment 8384618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/autoconf/icu.m4
> @@ -123,5 @@
> > -    if test -z "$MOZ_SHARED_ICU"; then
> > -        AC_DEFINE(U_STATIC_IMPLEMENTATION)
> > -    else
> > -        AC_DEFINE(U_COMBINED_IMPLEMENTATION)
> > -    fi
> 
> That part should still be needed.

No, it is the part which caused the bug by defining these bogus macros in mozilla-config.h.  This fallback logic <http://mxr.mozilla.org/mozilla-central/source/intl/icu/source/common/unicode/utypes.h#355> means that code which doesn't have any of these preprocessor macros will import the symbols, which is what we want.  Right?
Flags: needinfo?(mh+mozilla)
For U_COMBINED_IMPLEMENTATION you may be right, but for U_STATIC_IMPLEMENTATION i'm very much skeptical. You won't see it because clang and gcc don't care, but msvc doesn't. But we don't (currently) do non-shared icu builds for windows, so you won't see it either.
Flags: needinfo?(mh+mozilla)
Alright, I'll preserve the U_STATIC_IMPLEMENTATION.  Makes sense.
The build succeeded here: <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c75ce018e5db>.  I'm a victim of clobber, relanding: <https://hg.mozilla.org/integration/mozilla-inbound/rev/47afb5f53400>

glandium, want me to file a bug for the clobber issue?
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #8)
> I'm a victim of clobber, relanding:
> <https://hg.mozilla.org/integration/mozilla-inbound/rev/47afb5f53400>

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=de9872bce666
this one *was* a clobber, and failed.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb989c28ffb because not only was that previous clobber busted, and your new clobbers were busted, but whatever it was about https://tbpl.mozilla.org/php/getParsedLog.php?id=35576100&tree=Mozilla-Inbound that caused it to actually build, it wasn't the fact that it was a clobber since it wasn't.
Hmm so clearly there is something happening here which I can't reproduce locally...  But actually, now that I think about it again, for the purposes of this bug dropping the AC_DEFINE should be all that's needed, right?  The other change is actually changing the arguments passed to the ICU configure script, which is currently unneeded.
Flags: needinfo?(mh+mozilla)
I wouldn't expect dropping the icu configure changes to make the icu symbols not being exported.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #12)
> I wouldn't expect dropping the icu configure changes to make the icu symbols
> not being exported.

It does, tested my theory on try: https://tbpl.mozilla.org/?tree=Try&rev=9245d8a6b5b9
https://hg.mozilla.org/mozilla-central/rev/d421723d3a0f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
It makes no sense.
I just realized I misunderstood what this bug was about, and now that i think of it, it does make sense. It would have helped to have a more verbose description of the bug that was meant to be fixed.
(In reply to comment #17)
> I just realized I misunderstood what this bug was about, and now that i think
> of it, it does make sense. It would have helped to have a more verbose
> description of the bug that was meant to be fixed.

Sorry, re-reading comment 0 makes me believe that it wasn't as clear as I thought it was.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.