Closed
Bug 1300164
Opened 5 years ago
Closed 5 years ago
Move VISIBILITY_FLAGS to Python configure
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
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 | ||
Updated•5 years ago
|
Assignee: nobody → cmanchester
Comment hidden (mozreview-request) |
Comment 2•5 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 3•5 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 5•5 years ago
|
||
mozreview-review |
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
Comment 7•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/49f79af06dce
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•3 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•