Closed
Bug 1001001
Opened 10 years ago
Closed 10 years ago
don't export opus_ symbols from libxul
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: tbsaunde, Assigned: tbsaunde)
Details
Attachments
(1 file, 2 obsolete files)
1.39 KB,
patch
|
rillian
:
review+
tbsaunde
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
opus doesn't seem to provide a define for being built as a static library, but it allows us to override what it would define OPUS_EXPORT to be. So we can just define OPUS_EXPORT to the empty string and then those symbols will be hidden in libxul just like anything else.
Attachment #8411974 -
Flags: review?(tterribe)
Comment 2•10 years ago
|
||
Comment on attachment 8411974 [details] [diff] [review] stop exporting opus_ symbols from libxul Review of attachment 8411974 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libopus/moz.build @@ +20,5 @@ > DEFINES['USE_ALLOCA'] = True > > +# We only need to export symbols if we're built into libgkmedias instead of > +# libxul which only happens on windows. > +if CONFIG['OS_ARCH'] != 'WINNT': Please use CONFIG['GKMEDIAS_SHARED_LIBRARY'] here instead, since that's the relevant condition.
Assignee | ||
Comment 3•10 years ago
|
||
opus doesn't seem to provide a define for being built as a static library, but it allows us to override what it would define OPUS_EXPORT to be. So we can just define OPUS_EXPORT to the empty string and then those symbols will be hidden in libxul just like anything else.
Attachment #8412118 -
Flags: review?(giles)
Comment 4•10 years ago
|
||
Comment on attachment 8412118 [details] [diff] [review] stop exporting opus_ symbols from libxul Review of attachment 8412118 [details] [diff] [review]: ----------------------------------------------------------------- Fine with me. Adding Ted for build peer review.
Attachment #8412118 -
Flags: review?(ted)
Attachment #8412118 -
Flags: review?(giles)
Attachment #8412118 -
Flags: review+
Comment 5•10 years ago
|
||
Comment on attachment 8412118 [details] [diff] [review] stop exporting opus_ symbols from libxul Review of attachment 8412118 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libopus/moz.build @@ +21,5 @@ > > +# We only need to export symbols if we're built into libgkmedias instead of > +# libxul which only happens on windows. > +if CONFIG['GKMEDIAS_SHARED_LIBRARY']: > + DEFINES['OPUS_EXPORT'] = '' You want this to be a literal "-DOPUS_EXPORT=" ? If you just want "-DOPUS_EXPORT" you can set this = True.
Attachment #8412118 -
Flags: review?(ted)
Comment 6•10 years ago
|
||
I believe it needs to be -DOPUS_EXPORT='' on the compiler command line. OPUS_EXPORT is used as a decorator on public function declarations so we need an empty string expansion to override the default values of '__attribute__ ((visibility ("default")))' &c. Just defining the symbol doesn't result in an expansion and generates parse errors. -DOPUS_EXPORT= seems to generate the empty string expansion, at least with gcc and clang, so we don't need the explicit quotes on the command line.
Comment 7•10 years ago
|
||
Comment on attachment 8412118 [details] [diff] [review] stop exporting opus_ symbols from libxul Okay, thanks for the explanation.
Attachment #8412118 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #6) > I believe it needs to be -DOPUS_EXPORT='' on the compiler command line. > OPUS_EXPORT is used as a decorator on public function declarations so we > need an empty string expansion to override the default values of > '__attribute__ ((visibility ("default")))' &c. > > Just defining the symbol doesn't result in an expansion and generates parse > errors. no, it implicitly defines the macro to be 1 which makes sense so you can do #if FOO_ENABLED and #ifdef FOO_ENABLED
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bab9339e6d4
Comment 10•10 years ago
|
||
Comment on attachment 8412118 [details] [diff] [review] stop exporting opus_ symbols from libxul Review of attachment 8412118 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libopus/moz.build @@ +20,5 @@ > DEFINES['USE_ALLOCA'] = True > > +# We only need to export symbols if we're built into libgkmedias instead of > +# libxul which only happens on windows. > +if CONFIG['GKMEDIAS_SHARED_LIBRARY']: The conditional does the opposite of what the comment/commit message describes. # configure.in if test "$OS_ARCH" = "WINNT"; then GKMEDIAS_SHARED_LIBRARY=1 fi AC_SUBST(GKMEDIAS_SHARED_LIBRARY) plus # media/libopus/moz.build if CONFIG['GKMEDIAS_SHARED_LIBRARY']: DEFINES['OPUS_EXPORT'] = '' equals # media/libopus/moz.build if CONFIG['OS_ARCH'] == 'WINNT: DEFINES['OPUS_EXPORT'] = '' Notice the result being different from attachment 8411974 [details] [diff] [review].
Comment 11•10 years ago
|
||
Yep. Oops.
Comment 12•10 years ago
|
||
Inverted the conditional, cleanup up comment and commit message. Carrying forward r=me, ted. Trevor, is this what we want instead? Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=7ad2a64c1d29
Attachment #8411974 -
Attachment is obsolete: true
Attachment #8412118 -
Attachment is obsolete: true
Attachment #8411974 -
Flags: review?(tterribe)
Attachment #8413299 -
Flags: review+
Attachment #8413299 -
Flags: feedback?(trev.saunders)
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8413299 [details] [diff] [review] Don't export opus_ symbols from libxul v3 yeah, I just didn't get around to this on friday, so lgtm, and sorry about being laggy.
Attachment #8413299 -
Flags: feedback?(trev.saunders) → feedback+
Comment 14•10 years ago
|
||
No worries. I certainly didn't expect you to work over the weekend. :) Thanks for confirming. Green on try.
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c7e0011be7
Keywords: checkin-needed
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c1c7e0011be7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•