Closed
Bug 935881
Opened 11 years ago
Closed 11 years ago
Add a FINAL_LIBRARY variable to use in moz.build
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla28
People
(Reporter: glandium, Assigned: glandium)
References
Details
(Whiteboard: [qa-])
Attachments
(6 files, 12 obsolete files)
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
The purpose of the variable is to inverse declarations for most static libraries, by defining in the moz.build that defines them where they end up linked. That removes the burden to find the right place to add a SHARED_LIBRARY_LIBS, place the right ifdefs (and keep them in sync with moz.build DIRS changes), etc.
I don't think this is how we want things to be defined in the end game, but it's a start, and i don't want to be blocked by bikeshedding and scope-creep (going for full fledged library definitions is going to be much more work, and that would mean consequent build time improvements would have to wait, and /that/ would be a shame).
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #828531 -
Flags: review?(gps)
Assignee | ||
Comment 2•11 years ago
|
||
This will be used to declare in what shared library or intermediate static
library objects are going to be linked into.
Attachment #828532 -
Flags: review?(gps)
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
Comment on attachment 828533 [details] [diff] [review]
PoC for i18n, necko, uconv and xpcom_core
This is what the change looks like for i18n, necko, uconv and xpcom_core. I'll do the rest. Further down the road we can remove some of those useless intermediates. Even further down the road we can remove the leaf fake libraries.
This can also probably be simplified by the use of export(), now that we have that.
Attachment #828533 -
Flags: feedback?(gps)
Assignee | ||
Comment 5•11 years ago
|
||
We're not currently using it anywhere, and this will made useless by FINAL_LIBRARY (except for a few things, but even then, it will have to be something else than a passthrough)
Attachment #828535 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #828535 -
Attachment is obsolete: true
Attachment #828535 -
Flags: review?(gps)
Assignee | ||
Comment 7•11 years ago
|
||
Revised to use lowercase for LIBRARY_NAME matching, so that XUL vs xul can be handled. This is kind of awful, but the alternative (making LIBRARY_NAME 'xul' on mac) is not less awful.
Attachment #828583 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #828532 -
Attachment is obsolete: true
Attachment #828532 -
Flags: review?(gps)
Assignee | ||
Comment 8•11 years ago
|
||
Sigh, this is a version that still does lower(), but not on _libs_by_objdir because otherwise the build breaks.
I'm not sure if lower() is not worse than the alternative, which, fwiw looks like this:
https://hg.mozilla.org/try/rev/bda92d9c041e
Attachment #828612 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #828583 -
Attachment is obsolete: true
Attachment #828583 -
Flags: review?(gps)
Assignee | ||
Comment 9•11 years ago
|
||
This is work in progress. This is what it looks like with almost everything using FINAL_LIBRARY. It builds on Linux, and I fixed the first round of errors I got on other architectures. Hopefully try will succeed or there'll be another round.
Assignee | ||
Comment 10•11 years ago
|
||
Note that as a side effect this removes almost all EXPORT_LIBRARY references (only about 10 left)
Comment 11•11 years ago
|
||
Comment on attachment 828612 [details] [diff] [review]
Add a FINAL_LIBRARY variable to use in moz.build
Review of attachment 828612 [details] [diff] [review]:
-----------------------------------------------------------------
::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +356,5 @@
> backend_file.xpt_name = '%s.xpt' % obj.module
> elif isinstance(obj, VariablePassthru):
> + if 'LIBRARY_NAME' in obj.variables:
> + name = obj.variables['LIBRARY_NAME']
> + self._libs[name.lower()] = obj.objdir
What's the significance of last write wins here? Can't we just derive these entries from _libs_by_objdir?
@@ +357,5 @@
> elif isinstance(obj, VariablePassthru):
> + if 'LIBRARY_NAME' in obj.variables:
> + name = obj.variables['LIBRARY_NAME']
> + self._libs[name.lower()] = obj.objdir
> + self._libs_by_objdir[obj.objdir] = name
I don't care for extracting LIBRARY_NAME out of VariablePassthru like this. Could you convert it to a full-blown class, please?
@@ +667,5 @@
> + backend_file = self._backend_files[objdir]
> + if basename not in self._libs:
> + raise Exception('No LIBRARY_NAME in the tree matches '
> + 'FINAL_LIBRARY from %s (%s)' %
> + (backend_file.srcdir, basename))
This feels like something we should do in emitter since the logic is conceptually shared across backends.
Attachment #828612 -
Flags: review?(gps) → feedback+
Updated•11 years ago
|
Attachment #828573 -
Flags: review?(gps) → review+
Updated•11 years ago
|
Attachment #828531 -
Flags: review?(gps) → review+
Comment 12•11 years ago
|
||
Comment on attachment 828533 [details] [diff] [review]
PoC for i18n, necko, uconv and xpcom_core
Review of attachment 828533 [details] [diff] [review]:
-----------------------------------------------------------------
This is beautiful.
Attachment #828533 -
Flags: feedback?(gps) → feedback+
Comment 13•11 years ago
|
||
Comment on attachment 828634 [details] [diff] [review]
WiP
Review of attachment 828634 [details] [diff] [review]:
-----------------------------------------------------------------
I will continue giving +1's on this bug until it lands :)
Attachment #828634 -
Flags: feedback+
Assignee | ||
Updated•11 years ago
|
Attachment #828533 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #828531 -
Attachment is obsolete: true
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #830448 -
Flags: review?(gps)
Assignee | ||
Comment 15•11 years ago
|
||
This will be used to declare in what shared library or intermediate static
library objects are going to be linked into.
Attachment #830449 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #828612 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
This currently fails in multiple ways.
- on windows debug builds, it fails to link, because of what i think is a serious bug we already have, it's just hidden. Will file it.
- on linux pgo builds, it times-out when running the profileserver, which i can't reproduce locally :(
Assignee | ||
Updated•11 years ago
|
Attachment #828634 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #831165 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #830453 -
Attachment is obsolete: true
Assignee | ||
Comment 18•11 years ago
|
||
Refreshed against bug 937526
Attachment #831169 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #830448 -
Attachment is obsolete: true
Attachment #830448 -
Flags: review?(gps)
Assignee | ||
Comment 19•11 years ago
|
||
Refreshed against bug 935980 and bug 937526.
Attachment #831173 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #831165 -
Attachment is obsolete: true
Attachment #831165 -
Flags: review?(gps)
Assignee | ||
Comment 20•11 years ago
|
||
FWIW, this is what i got out of try talos for PGO performance:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=87b26c259eb9&newRev=bfce10eb4682&submit=true
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=33f5a234edc3&newRev=0a6d92d0eb7f&submit=true
Apparently, there's a big win on dromaeo css, and no significant change on other results (although at the time i'm writing this there's not enough data for mac)
Interestingly, linker max vsize got down 1.5MB, too, which is not a lot, but it's also better than what tbsaunde had got in a previous attempt with something different with the same kind of effect of getting rid of most SHARED_LIBRARY_LIBS, where memory usage went up significantly.
Assignee | ||
Comment 21•11 years ago
|
||
Actually rebased against bug 937526.
Attachment #831233 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #831173 -
Attachment is obsolete: true
Attachment #831173 -
Flags: review?(gps)
Assignee | ||
Comment 22•11 years ago
|
||
Rebased again. Sorry for the noise. This should be the last.
Attachment #831388 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #831233 -
Attachment is obsolete: true
Attachment #831233 -
Flags: review?(gps)
Comment 23•11 years ago
|
||
Comment on attachment 830449 [details] [diff] [review]
Add a FINAL_LIBRARY variable to use in moz.build
Review of attachment 830449 [details] [diff] [review]:
-----------------------------------------------------------------
::: python/mozbuild/mozbuild/frontend/data.py
@@ +327,5 @@
> +class LibraryDefinition(SandboxDerived):
> + """Partial definition for a library"""
> + __slots__ = (
> + 'basename',
> + 'static_libraries',
Please document static_libraries.
@@ +337,5 @@
> + self.basename = basename
> + self.static_libraries = []
> +
> + def link_static_lib(self, sandbox, basename):
> + self.static_libraries.append((sandbox['RELATIVEDIR'], basename))
I'm not sure why you need a full method for a one-liner only called once.
::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +296,5 @@
> + raise SandboxValidationError('FINAL_LIBRARY requires LIBRARY_NAME')
> + if not sandbox.get('FORCE_STATIC_LIB') and \
> + not sandbox.get('LIBXUL_LIBRARY'):
> + raise SandboxValidationError('FINAL_LIBRARY requires FORCE_STATIC_LIB or LIBXUL_LIBRARY')
> + self._final_libs.append((sandbox, libname, final_lib))
Must you hold a reference to the sandbox? That will prevent gc. But we probably don't care given our scale.
Attachment #830449 -
Flags: review?(gps) → review+
Updated•11 years ago
|
Attachment #831169 -
Flags: review?(gps) → review+
Comment 24•11 years ago
|
||
Comment on attachment 831388 [details] [diff] [review]
Use FINAL_LIBRARY for all (fake) libraries that end up linked in a single other library.
Review of attachment 831388 [details] [diff] [review]:
-----------------------------------------------------------------
Some of my favorite patches are ones that remove move lines than they add while accomplishing the same thing. This patch ranks high in that index!
Attachment #831388 -
Flags: review?(gps) → review+
Assignee | ||
Comment 25•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #23)
> I'm not sure why you need a full method for a one-liner only called once.
You tell me :) It just seemed to be the pattern.
> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +296,5 @@
> > + raise SandboxValidationError('FINAL_LIBRARY requires LIBRARY_NAME')
> > + if not sandbox.get('FORCE_STATIC_LIB') and \
> > + not sandbox.get('LIBXUL_LIBRARY'):
> > + raise SandboxValidationError('FINAL_LIBRARY requires FORCE_STATIC_LIB or LIBXUL_LIBRARY')
> > + self._final_libs.append((sandbox, libname, final_lib))
>
> Must you hold a reference to the sandbox?
Yes, because the sandbox is used at consume_finished time. And there will be more of those kind of things in the future, fwiw.
Assignee | ||
Comment 26•11 years ago
|
||
As it turns out, I'm not going to use the sandbox as it was stored in that patch, so I'm just switching to using the relative directory directly. Future patches for upcoming bugs *will* store the sandboxes, but somewhere else.
Attachment #832032 -
Flags: review?(gps)
Comment 27•11 years ago
|
||
Comment on attachment 831388 [details] [diff] [review]
Use FINAL_LIBRARY for all (fake) libraries that end up linked in a single other library.
Review of attachment 831388 [details] [diff] [review]:
-----------------------------------------------------------------
::: intl/build/Makefile.in
@@ -17,5 @@
> -I$(srcdir)/../lwbrk/src \
> -I$(srcdir)/../unicharutil/src \
> -I$(srcdir)/../strres/src \
> -I$(srcdir)/../locale/src \
> -I$(srcdir)/../locale/src/$(LOCALE_DIR) \
LOCALE_DIR is still used here
::: toolkit/library/Makefile.in
@@ +20,4 @@
> ifdef MOZ_CONTENT_SANDBOX
> ifeq ($(OS_ARCH),WINNT)
> LOCAL_INCLUDES += -I$(srcdir)/../sandboxbroker
> SHARED_LIBRARY_LIBS += ../../security/sandbox/win/src/sandboxbroker/$(LIB_PREFIX)sandboxbroker.$(LIB_SUFFIX)
How about this one?
@@ +23,5 @@
> SHARED_LIBRARY_LIBS += ../../security/sandbox/win/src/sandboxbroker/$(LIB_PREFIX)sandboxbroker.$(LIB_SUFFIX)
> endif
> endif
>
> +# COMPONENT_LIBS is mosly useless since bug 935881, but is kept for
mostly
@@ +73,1 @@
> # component libraries
No longer, I'd say
Assignee | ||
Comment 28•11 years ago
|
||
(In reply to :Ms2ger from comment #27)
> Comment on attachment 831388 [details] [diff] [review]
> Use FINAL_LIBRARY for all (fake) libraries that end up linked in a single
> other library.
>
> Review of attachment 831388 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: intl/build/Makefile.in
> @@ -17,5 @@
> > -I$(srcdir)/../lwbrk/src \
> > -I$(srcdir)/../unicharutil/src \
> > -I$(srcdir)/../strres/src \
> > -I$(srcdir)/../locale/src \
> > -I$(srcdir)/../locale/src/$(LOCALE_DIR) \
>
> LOCALE_DIR is still used here
Well, I guess I can remove that line. The headers in those directories are actually not included from anywhere else than those directories. So this -I was useless.
> ::: toolkit/library/Makefile.in
> @@ +20,4 @@
> > ifdef MOZ_CONTENT_SANDBOX
> > ifeq ($(OS_ARCH),WINNT)
> > LOCAL_INCLUDES += -I$(srcdir)/../sandboxbroker
> > SHARED_LIBRARY_LIBS += ../../security/sandbox/win/src/sandboxbroker/$(LIB_PREFIX)sandboxbroker.$(LIB_SUFFIX)
>
> How about this one?
It's linked in several places. This needs separate handling, which is next.
> @@ +23,5 @@
> > SHARED_LIBRARY_LIBS += ../../security/sandbox/win/src/sandboxbroker/$(LIB_PREFIX)sandboxbroker.$(LIB_SUFFIX)
> > endif
> > endif
> >
> > +# COMPONENT_LIBS is mosly useless since bug 935881, but is kept for
>
> mostly
>
> @@ +73,1 @@
> > # component libraries
>
> No longer, I'd say
COMPONENT_LIBS += $(MOZ_APP_COMPONENT_LIBS)
Updated•11 years ago
|
Attachment #832032 -
Flags: review?(gps) → review+
Assignee | ||
Comment 29•11 years ago
|
||
Attachment #8333596 -
Flags: review?(gps)
Updated•11 years ago
|
Attachment #8333596 -
Flags: review?(gps) → review+
Assignee | ||
Comment 30•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3d0fce244e6b
https://hg.mozilla.org/mozilla-central/rev/a296036e608f
https://hg.mozilla.org/mozilla-central/rev/7161a2d28363
https://hg.mozilla.org/mozilla-central/rev/8a50d16b1645
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Whiteboard: [qa-]
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•