Closed
Bug 978784
Opened 10 years ago
Closed 10 years ago
Stop exporting ICU symbols from xul.dll
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla30
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file)
1.93 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8384618 -
Flags: review?(mh+mozilla)
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
(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)
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Alright, I'll preserve the U_STATIC_IMPLEMENTATION. Makes sense.
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c75ce018e5db
Comment 7•10 years ago
|
||
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/11ab722b0bcb for https://tbpl.mozilla.org/php/getParsedLog.php?id=35571524&tree=Mozilla-Inbound
Assignee | ||
Comment 8•10 years ago
|
||
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?
Comment 9•10 years ago
|
||
(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.
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
I wouldn't expect dropping the icu configure changes to make the icu symbols not being exported.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 13•10 years ago
|
||
(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
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d421723d3a0f
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d421723d3a0f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 16•10 years ago
|
||
It makes no sense.
Comment 17•10 years ago
|
||
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.
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•