1001001 - don't export opus_ symbols from libxul

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: stop exporting opus_ symbols from libxul

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.

Comment 2

[Ralph Giles (:rillian)]

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.

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: stop exporting opus_ symbols from libxul

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.

Comment 4

[Ralph Giles (:rillian)]

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.

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Ralph Giles (:rillian)]

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

[(not currently active) Ted Mielczarek]

Comment on attachment 8412118 [details] [diff] [review]
stop exporting opus_ symbols from libxul

Okay, thanks for the explanation.

Comment 8

[Trevor Saunders (:tbsaunde)]

(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

Comment 9

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bab9339e6d4

Comment 10

[Jan Beich]

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

[Ralph Giles (:rillian)]

Yep. Oops.

Comment 12

[Ralph Giles (:rillian)]

Attachment: Don't export opus_ symbols from libxul v3

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

Comment 13

[Trevor Saunders (:tbsaunde)]

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.

Comment 14

[Ralph Giles (:rillian)]

No worries. I certainly didn't expect you to work over the weekend. :)

Thanks for confirming. Green on try.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c7e0011be7

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1c7e0011be7