1300164 - Move VISIBILITY_FLAGS to Python configure

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Chris Manchester (:chmanchester)]

Attachment: Bug 1300164 - Move VISIBILITY_FLAGS to Python configure.

Review commit: https://reviewboard.mozilla.org/r/80910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80910/

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8794438 [details]
Bug 1300164 - Move VISIBILITY_FLAGS to Python configure.

https://reviewboard.mozilla.org/r/80910/#review79904

::: build/moz.configure/toolchain.configure:881
(Diff revision 1)
> +# Check for .hidden assembler directive and visibility attribute.
> +# Converted from code borrowed from glibc configure.in

Oh my, that comment is completely outdated, you can remove it :).

::: build/moz.configure/toolchain.configure:885
(Diff revision 1)
>  
> +# Check for .hidden assembler directive and visibility attribute.
> +# Converted from code borrowed from glibc configure.in
> +@depends(c_compiler, target, check_build_environment)
> +def visibility_flags(c_compiler, target, env):
> +    if c_compiler.type in ('gcc', 'clang') and target.os != 'WINNT':

Nowadays, a test that says "compiler is gcc or clang, and target is not WINNT" can be reduced to "target is not WINNT": We don't support building with other compilers for non-WINNT targets.

::: build/moz.configure/toolchain.configure:886
(Diff revision 1)
> +# Check for .hidden assembler directive and visibility attribute.
> +# Converted from code borrowed from glibc configure.in
> +@depends(c_compiler, target, check_build_environment)
> +def visibility_flags(c_compiler, target, env):
> +    if c_compiler.type in ('gcc', 'clang') and target.os != 'WINNT':
> +        if target.os == 'OSX':

The test should still be against Darwin (with target.kernel), because building for iOS requires the same flags.

::: build/moz.configure/toolchain.configure:892
(Diff revision 1)
> +@depends(target, visibility_flags)
> +def wrap_system_includes(target, visibility_flags):
> +    if visibility_flags and target.os != 'OSX':
> +        return True

there's only really one relevant place where WRAP_SYSTEM_INCLUDES is used: config/Makefile.in (there's another in toolkit/xre/moz.build, but it seems bogus)

We might as well change that Makefile to check if VISIBILITY_FLAGS contains system_wrappers.

::: build/moz.configure/toolchain.configure:899
(Diff revision 1)
> +    if visibility_flags and target.os != 'OSX':
> +        return True
> +
> +set_define('HAVE_VISIBILITY_HIDDEN_ATTRIBUTE',
> +           depends(visibility_flags)(lambda v: bool(v) or None))
> +set_define('HAVE_VISIBILITY_ATTRIBUTE',

I think we should remove this one. It's only used in mfbt/Types.h and xpcom/base/nscore.h and in both cases it should IMHO be replaced with compiler #ifdefs. (HAVE_VISIBILITY_HIDDEN_ATTRIBUTE is used by third-party code, so it's different). Can you file a followup bug to remove it?

Comment 3

[Chris Manchester (:chmanchester)]

Comment on attachment 8794438 [details]
Bug 1300164 - Move VISIBILITY_FLAGS to Python configure.

https://reviewboard.mozilla.org/r/80910/#review79904

> there's only really one relevant place where WRAP_SYSTEM_INCLUDES is used: config/Makefile.in (there's another in toolkit/xre/moz.build, but it seems bogus)
> 
> We might as well change that Makefile to check if VISIBILITY_FLAGS contains system_wrappers.

Unfortunately this Makefile is in a directory that has NO_VISIBILITY_FLAGS set.

Comment 4

[Chris Manchester (:chmanchester)]

Comment on attachment 8794438 [details]
Bug 1300164 - Move VISIBILITY_FLAGS to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80910/diff/1-2/

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8794438 [details]
Bug 1300164 - Move VISIBILITY_FLAGS to Python configure.

https://reviewboard.mozilla.org/r/80910/#review81550

::: build/moz.configure/toolchain.configure:892
(Diff revision 2)
> +                '-include',
> +                '%s/config/gcc_hidden.h' % env.topsrcdir)
> +
> +@depends(target, visibility_flags)
> +def wrap_system_includes(target, visibility_flags):
> +    if visibility_flags and target.os != 'OSX':

target.kernel != "Darwin"

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49f79af06dce
Move VISIBILITY_FLAGS to Python configure. r=glandium

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49f79af06dce