Closed Bug 1300164 Opened 3 years ago Closed 3 years ago

Move VISIBILITY_FLAGS to Python configure

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → cmanchester
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?
Attachment #8794438 - Flags: review?(mh+mozilla)
Blocks: 1305823
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 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"
Attachment #8794438 - Flags: review?(mh+mozilla) → review+
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49f79af06dce
Move VISIBILITY_FLAGS to Python configure. r=glandium
https://hg.mozilla.org/mozilla-central/rev/49f79af06dce
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.