Closed
Bug 1036894
Opened 10 years ago
Closed 10 years ago
Move in-tree library linkage information to moz.build
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla34
People
(Reporter: glandium, Assigned: glandium)
References
(Blocks 2 open bugs)
Details
Attachments
(11 files, 16 obsolete files)
10.01 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
6.00 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
54.08 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
15.80 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
33.55 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
8.36 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
129.49 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
38.45 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
54.54 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
8.60 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
631 bytes,
patch
|
ewong
:
review+
|
Details | Diff | Splinter Review |
The goal is double:
- allow to have in-tree inter-dependencies known to the build backend so that it can do smarter things
- hopefully less confusing configuration for developers
Assignee | ||
Comment 1•10 years ago
|
||
At the same time, make the Library data more useful in the build frontend.
Attachment #8453711 -
Flags: review?(gps)
Assignee | ||
Comment 2•10 years ago
|
||
At the same time, make the Library data more useful in the build frontend.
Attachment #8453712 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8453711 -
Attachment is obsolete: true
Attachment #8453711 -
Flags: review?(gps)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8453713 -
Flags: review?(gps)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8453714 -
Flags: review?(gps)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8453717 -
Flags: review?(gps)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8453721 -
Flags: review?(gps)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8453723 -
Flags: review?(gps)
Assignee | ||
Comment 8•10 years ago
|
||
This, and part 6, are what I'm after, here, as far as moz.build configuration goes.
The patch queue is not entirely tested at the moment, and incomplete, but I wanted at least a first review pass.
I haven't decided whether to keep allowing LIBRARY_NAME overlap. Now that there's STATIC_LIBRARY_NAME and SHARED_LIBRARY_NAME, we could impose all LIBRARY_NAMEs to be different. On the other hand, we could allow disambiguation by passing relative or topobjdir-absolute paths (in the form some/path/library_name, not some/path/libfoo.a).
I'm pondering what to do with the various other kinds of linkage, and I'm very tempted to allow all sorts of value in LIBS/HOST_LIBS:
- a code name corresponding to a LIBRARY_NAME (which is what this patch queue is adding)
- a code name with a relative or topobjdir-absolute path as mentioned above
- a full path to a SDK library (think android NDK, direct X import libs...)
- "-Lpath -llib" straight out of pkgconfig
The typical use for these would be to write something like:
LIBS += [
CONFIG['MOZ_ZLIB_LIBS'],
]
etc.
and not have to care about doing checks whether we're using native zlib or not, and switch variable in that case.
I'm not sure how to handle windows system libraries, though (which need to either be -lfoo or foo.lib depending whether we build with msvc or mingw). Maybe kill OS_LIBS and have a windows-specific WIN_LIBS?
Attachment #8453729 -
Flags: review?(gps)
Comment 9•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #8)
> I'm pondering what to do with the various other kinds of linkage, and I'm
> very tempted to allow all sorts of value in LIBS/HOST_LIBS:
This sounds nice, but my only worry is that it will be difficult to disambiguate--both by developers reading moz.build files and by the build system trying to validate entries.
Is there any prior art here in other build description languages (pants/buck/gn)?
Assignee | ||
Updated•10 years ago
|
Attachment #8453712 -
Attachment is obsolete: true
Attachment #8453712 -
Flags: review?(gps)
Assignee | ||
Comment 11•10 years ago
|
||
Refreshed against new part 1.
Attachment #8453743 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8453717 -
Attachment is obsolete: true
Attachment #8453717 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8453721 -
Attachment is obsolete: true
Attachment #8453721 -
Flags: review?(gps)
Assignee | ||
Comment 13•10 years ago
|
||
Needed to get rid of two instances of SHARED_LIBRARY_NAME in makefiles.
Attachment #8453772 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8453741 -
Attachment is obsolete: true
Attachment #8453741 -
Flags: review?(gps)
Assignee | ||
Comment 14•10 years ago
|
||
Order is important for breakpad HOST_LIBS.
Attachment #8453775 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8453729 -
Attachment is obsolete: true
Attachment #8453729 -
Flags: review?(gps)
Comment 15•10 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > I'm pondering what to do with the various other kinds of linkage, and I'm
> > very tempted to allow all sorts of value in LIBS/HOST_LIBS:
>
> This sounds nice, but my only worry is that it will be difficult to
> disambiguate--both by developers reading moz.build files and by the build
> system trying to validate entries.
>
> Is there any prior art here in other build description languages
> (pants/buck/gn)?
I haven't looked at the patches yet, but I thought we decided that style should take a back seat to a) conversions b) build efficiency. i.e. if we can make things look clean for little extra effort, great. If not, we won't wait on it. Given the relative stagnation in the build system lately, I'm pretty much prepared to r+ anything that looks like a step in the right direction, especially if it can lead to build time wins. Since inefficiencies around the libs tier are the most painful element now, this patch series looks very attractive.
Also, rewriting moz.build files shouldn't be too difficult. 2to3.py is a generic AST transformation tool. We can leverage that for refactoring moz.build files into prettier formats.
Comment 16•10 years ago
|
||
Comment on attachment 8453772 [details] [diff] [review]
part 1 - Move MAKE_FRAMEWORK, SDK_LIBRARY, SHARED_LIBRARY_NAME and STATIC_LIBRARY_NAME to moz.build
Review of attachment 8453772 [details] [diff] [review]:
-----------------------------------------------------------------
After reading the patch, I have no concerns about what the grammar looks like. This patch is obviously a step in the right direction. Defining libraries can actually be grokked by mere mortals now. I think I even learned a thing or two by reading the logic in emitter.py.
The only thing that would make this patch better is more documentation around how to define libraries. Maybe a summary of the different library options and when to use them, etc. But I'm not going to hold back r+ for that. Maybe you can write the docs while I finish reviewing this series...
::: config/rules.mk
@@ -202,5 @@
> -ifdef FORCE_SHARED_LIB
> -ifndef FORCE_STATIC_LIB
> -LIBRARY := $(NULL)
> -endif
> -endif
I will gladly trade lines of make for lines of Python.
::: mozglue/build/Makefile.in
@@ -19,5 @@
> -SDK_LIBRARY = $(REAL_LIBRARY)
> -endif
> -endif
> -
> -endif
I love how blocks like this can be replaced by a single line. So much clearer what the intent is.
::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +59,5 @@
>
> from .gyp_reader import GypSandbox
>
>
> +class OrderedDefaultDict(OrderedDict):
This class should ideally be in mozbuild.util.
@@ +414,5 @@
> + raise SandboxValidationError(
> + 'FINAL_LIBRARY conflicts with IS_FRAMEWORK', sandbox)
> + if is_component:
> + raise SandboxValidationError(
> + 'FINAL_LIBRARY conflicts with IS_COMPONENT', sandbox)
Would you care to make these messages a little more actionable? e.g.
FINAL_LIBRARY implies FORCE_STATIC_LIB; remove one
@@ +421,3 @@
>
> + if libname:
> + args = { 'kind': 0 }
Nit: cuddle braces
::: xpcom/glue/moz.build
@@ -97,5 @@
>
> LIBRARY_NAME = 'xpcomglue_s'
>
> -SDK_LIBRARY = [
> - "%s%s.%s" % (CONFIG['LIB_PREFIX'], 'xpcomglue_s', CONFIG['LIB_SUFFIX']),
More needless complexity abstracted away.
Attachment #8453772 -
Flags: review?(gps) → review+
Updated•10 years ago
|
Attachment #8453713 -
Flags: review?(gps) → review+
Updated•10 years ago
|
Attachment #8453714 -
Flags: review?(gps) → review+
Comment 17•10 years ago
|
||
Comment on attachment 8453743 [details] [diff] [review]
part 4 - Allow to track library linkage for all kinds of programs and libraries
Review of attachment 8453743 [details] [diff] [review]:
-----------------------------------------------------------------
::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +425,5 @@
> is_component = sandbox.get('IS_COMPONENT')
>
> soname = sandbox.get('SONAME')
>
> + args = { 'kind': 0 }
Nit: cuddle
Attachment #8453743 -
Flags: review?(gps) → review+
Comment 18•10 years ago
|
||
Comment on attachment 8453746 [details] [diff] [review]
part 5 - Hook the LIBS and HOST_LIBS moz.build variables to library linkage
Review of attachment 8453746 [details] [diff] [review]:
-----------------------------------------------------------------
::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1193,5 @@
> + % (relpath, lib.shared_name))
> + elif isinstance(obj, (HostLibrary, HostProgram)):
> + assert lib.kind == lib.STATIC
> + backend_file.write('HOST_LIBS += %s/%s\n'
> + % (relpath, lib.static_name))
Does this patch on its result in a clean build? I'd think writing these new variables would confuse rules.mk. Do we dedupe the variables or something?
::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +192,5 @@
> yield obj
>
> + def _link_libraries(self, sandbox, obj, variable):
> + """Add linkage declarations to a given object.
> + """
Nit: You don't need the extra line for the ending """
@@ +205,5 @@
> + raise SandboxValidationError(
> + '%s contains "%s", which matches a %s defined in multiple '
> + 'places:\n %s' % (variable, name, obj.NAME_VAR,
> + '\n '.join(mozpath.join(l.relativedir, 'moz.build')
> + for l in candidates)), sandbox)
Nit: This is a bit harder to read. Maybe extract the mozpath.join(...) into its own variable?
Attachment #8453746 -
Flags: review?(gps) → review+
Comment 19•10 years ago
|
||
Comment on attachment 8453723 [details] [diff] [review]
part 6 - Move most HOST_LIBS to moz.build
Review of attachment 8453723 [details] [diff] [review]:
-----------------------------------------------------------------
I love how moz.build makes things so much cleaner and helps to increase understanding.
Attachment #8453723 -
Flags: review?(gps) → review+
Comment 20•10 years ago
|
||
Comment on attachment 8453723 [details] [diff] [review]
part 6 - Move most HOST_LIBS to moz.build
Review of attachment 8453723 [details] [diff] [review]:
-----------------------------------------------------------------
Oh, did you want to ban HOST_LIBS from Makefile.in?
Comment 21•10 years ago
|
||
Comment on attachment 8453775 [details] [diff] [review]
part 7 - Move some SHARED_LIBRARY_LIBS to moz.build, as LIBS
Review of attachment 8453775 [details] [diff] [review]:
-----------------------------------------------------------------
I'm nearly weeping at how amazing this patch series is.
I love how we can just reference libraries by name now instead of having to know their exact paths in the build config.
I can't wait to see the next patch series. I assume that will involve light magic where we use our new library relationship knowledge to traverse the libs tier more efficiently.
Attachment #8453775 -
Flags: review?(gps) → review+
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #20)
> Oh, did you want to ban HOST_LIBS from Makefile.in?
There's still one for -lz in mozglue/linker, but I guess I could move that to HOST_LDFLAGS or HOST_EXTRA_LIBS and close the loop.
(In reply to Gregory Szorc [:gps] from comment #21)
> I can't wait to see the next patch series. I assume that will involve light
> magic where we use our new library relationship knowledge to traverse the
> libs tier more efficiently.
I have that working for host libs and host programs because all in-tree interdependencies are now declared in moz.build (except one for host_stdc++compat), while there is still a lot left to do for target libs and programs.
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > I'm pondering what to do with the various other kinds of linkage, and I'm
> > very tempted to allow all sorts of value in LIBS/HOST_LIBS:
>
> This sounds nice, but my only worry is that it will be difficult to
> disambiguate--both by developers reading moz.build files and by the build
> system trying to validate entries.
Actually, it's not a problem for the build system itself. It can be seen as a problem for developers, but I, for one, think this is a detail developers should not care about. They need a library, they say they need it. That the library is a shared library on windows and not on linux is not their problem. It's ours (build config peers). And we can easily add tools to ensure that we're not overly using static libraries and whatnot.
It can still turn out that I'm wrong in my thinking, in which case it will still be easy to change automatically. I think it's fine to try better things, even if they turn out not to be great. I don't think it's any worse than the current status-quo.
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #16)
> The only thing that would make this patch better is more documentation
> around how to define libraries. Maybe a summary of the different library
> options and when to use them, etc.
What's the location for such docs?
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #18)
> Comment on attachment 8453746 [details] [diff] [review]
> part 5 - Hook the LIBS and HOST_LIBS moz.build variables to library linkage
>
> Review of attachment 8453746 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +1193,5 @@
> > + % (relpath, lib.shared_name))
> > + elif isinstance(obj, (HostLibrary, HostProgram)):
> > + assert lib.kind == lib.STATIC
> > + backend_file.write('HOST_LIBS += %s/%s\n'
> > + % (relpath, lib.static_name))
>
> Does this patch on its result in a clean build? I'd think writing these new
> variables would confuse rules.mk. Do we dedupe the variables or something?
I'm not sure what you mean here. HOST_LIBS is a StrictOrderingOnAppendList, don't we reject dupes there?
(That said, yes, in the future, we may want to add things like loop detection, but that seems overkill)
Comment 26•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #23)
> It can still turn out that I'm wrong in my thinking, in which case it will
> still be easy to change automatically. I think it's fine to try better
> things, even if they turn out not to be great. I don't think it's any worse
> than the current status-quo.
+1 from me on that. I haven't read the patches, but from your description here they sound like a huge improvement.
Assignee | ||
Comment 27•10 years ago
|
||
One thing that /might/ lead to confusion is the variable name "LIBS", especially if we end up placing system libraries in a different one. I would like, however, to make things such that all libraries that are in-tree (third party non-moz.build-covered included) are all in a single variable, even for the case where those in-tree third party libraries can be replaced by system libraries.
Unrelatedly, there is another case where while the scheme in this patch queue is to link the shared library when there are both a static and shared library generated for a given LIBRARY_NAME, we still want to link the static library. This currently happens for the js engine library on windows, which is built as a shared library, and linked as such to libxul, but still linked statically to a bunch of other stuff. We may want to allow LIBS to contain a STATIC_LIBRARY_NAME for such cases.
Assignee | ||
Comment 28•10 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #26)
> but from your description, here they sound like a huge improvement.
Here is, I think, the most extreme example:
We have this in configure.in:
MOZ_JS_STATIC_LIBS='$(call EXPAND_LIBNAME_PATH,js_static,$(LIBXUL_DIST)/lib)'
MOZ_JS_SHARED_LIBS='$(call EXPAND_LIBNAME_PATH,mozjs,$(LIBXUL_DIST)/lib)'
Then, still in configure.in, we choose one depending on whether we declared we wanted a shared js library and assign it to MOZ_JS_LIBS, which we then use in several locations, adding it to either LIBS or EXTRA_DSO_LDOPTS, depending where you look (which, in itself, is an abuse of semantics (and of the lack of enforcement in rules.mk), since static libraries linked into shared libraries are "supposed" to be in SHARED_LIBRARY_LIBS.
With the patch queue (and a few adjustments to js/src/moz.build), it would become LIBS += ['js'] (or 'mozjs', depending the name we choose).
The downside is that this is going to break --with-libxul-sdk builds, but I have no sympathy for them at the moment (and as a disclaimer, I'll point that I'm probably the one that has been using --with-libxul-sdk for the longest).
Comment 29•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Gregory Szorc [:gps] from comment #16)
> > The only thing that would make this patch better is more documentation
> > around how to define libraries. Maybe a summary of the different library
> > options and when to use them, etc.
>
> What's the location for such docs?
I'd add a "linking" page to build/docs.
Comment 30•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #27)
> One thing that /might/ lead to confusion is the variable name "LIBS",
> especially if we end up placing system libraries in a different one.
I will admit this confused me a bit during review. When I got to the patch that rewrote SHARED_LIBRARY_LIBS in Makefile.in to LIBS in moz.build, I had to look at the previous patches to validate my understanding matched reality.
On the subject of LIBS, perhaps a better variable name would be LINK_LIBS or LINK_AGAINST_LIBS. If we ever need to explicitly control the link type for libraries that have both static and shared forms, perhaps values could be "shared:mozjs" or "static:mozjs." If a library has two forms, we could even enforce that moz.build files are explicit about which form of linking to use, to avoid any ambiguity and surprises.
Assignee | ||
Comment 31•10 years ago
|
||
> If a library has two forms, we could even enforce that moz.build files
> are explicit about which form of linking to use, to avoid any ambiguity
> and surprises.
That would make it necessary to do if CONFIG['JS_SHARED_LIBRARY'] when linking the js library. That's why I suggested to use the STATIC_LIBRARY_NAME, because that never changes (although static:mozjs would work too).
Assignee | ||
Comment 32•10 years ago
|
||
> On the subject of LIBS, perhaps a better variable name would be LINK_LIBS or LINK_AGAINST_LIBS.
Note that when I originally toyed with the idea behind this bug, the variable was LINKED_LIBRARIES. But that seemed too verbose, and I stumbled upon LIBS in sandbox_symbols.py. LINK_LIBS sounds good.
Assignee | ||
Comment 33•10 years ago
|
||
jcranmer suggested USED_LIBS, which rubbed me in the right way, and led me to USE_LIBS.
Pick your bikeshed color.
Comment 34•10 years ago
|
||
I'm fine with USE_LIBS.
Assignee | ||
Comment 35•10 years ago
|
||
At the same time, make the Library data more useful in the build frontend.
Assignee | ||
Updated•10 years ago
|
Attachment #8453772 -
Attachment is obsolete: true
Assignee | ||
Comment 36•10 years ago
|
||
Comment on attachment 8457969 [details] [diff] [review]
part 1 - Move MAKE_FRAMEWORK, SDK_LIBRARY, SHARED_LIBRARY_NAME and STATIC_LIBRARY_NAME to moz.build
Addressed review comments, so carrying r+ over.
Attachment #8457969 -
Flags: review+
Assignee | ||
Comment 37•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8453743 -
Attachment is obsolete: true
Assignee | ||
Comment 38•10 years ago
|
||
Comment on attachment 8457970 [details] [diff] [review]
part 4 - Allow to track library linkage for all kinds of programs and libraries
Addressed review comments.
Attachment #8457970 -
Flags: review+
Assignee | ||
Comment 39•10 years ago
|
||
Significant changes in here, to support changing more things in part 7 (now 8). This adds the discussed static: prefix. Part 7 and 8 clue in as to what moz.build definitions look like, until I write proper documentation.
Attachment #8457973 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8453746 -
Attachment is obsolete: true
Assignee | ||
Comment 40•10 years ago
|
||
Comment on attachment 8457973 [details] [diff] [review]
part 5 - Hook the USE_LIBS and HOST_USE_LIBS moz.build variables to library linkage
Review of attachment 8457973 [details] [diff] [review]:
-----------------------------------------------------------------
::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +103,5 @@
> self.fh.write(buf)
>
> + def write_once(self, buf):
> + if '\n' + buf not in self.fh.getvalue():
> + self.write(buf)
Note this is an awful hack, but necessary to avoid repeating library related stuff when there are several programs or libraries in a directory (avoiding the repetition avoids build failure). It could be done differently, but that would involve much more code, and I didn't feel it was worth.
Assignee | ||
Comment 41•10 years ago
|
||
Note the sandbox_symbols.py description is essentially a copy/paste from the one for SIMPLE_PROGRAMS.
Attachment #8457978 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8453723 -
Attachment is obsolete: true
Assignee | ||
Comment 42•10 years ago
|
||
Attachment #8457979 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8453775 -
Attachment is obsolete: true
Assignee | ||
Comment 43•10 years ago
|
||
Attachment #8457980 -
Flags: review?(gps)
Assignee | ||
Comment 44•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=063b20dcdef7
From there, the plan is to wait for parts 9 and 10, and land after next merge day.
Part 9 will move SHARED_LIBRARY_LIBS, EXTRA_DSO_LDOPTS and LIBS to OS_LIBS or EXTRA_LIBS in Makefiles, and forbid them.
Part 10 will add the much awaited documentation.
More will come in followup bugs.
Assignee | ||
Comment 45•10 years ago
|
||
With more that I spotted when changing variables names for part 9.
Attachment #8458383 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8457980 -
Attachment is obsolete: true
Attachment #8457980 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8458383 -
Attachment is obsolete: true
Attachment #8458383 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8458384 -
Attachment is obsolete: true
Attachment #8458384 -
Flags: review?(gps)
Assignee | ||
Comment 48•10 years ago
|
||
With more adjustments and rebased on top of bug 1040639 and bug 1040641.
https://tbpl.mozilla.org/?tree=Try&rev=2bc7f611faf8
Attachment #8458535 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8458393 -
Attachment is obsolete: true
Attachment #8458393 -
Flags: review?(gps)
Assignee | ||
Comment 49•10 years ago
|
||
With more sanity check to avoid conflicts between import library file name and static library file name.
Attachment #8458589 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8457973 -
Attachment is obsolete: true
Attachment #8457973 -
Flags: review?(gps)
Assignee | ||
Comment 50•10 years ago
|
||
OS_LIBS for libraries that are not part of the gecko tree, EXTRA_LIBS for
libraries, such as NSPR, that are in the tree, but are not handled by
moz.build just yet. Those EXTRA_LIBS may also come from a system library.
However, in cases where the expanded variables are always empty for the
in-tree case, OS_LIBS is used (as for, e.g. MOZ_ZLIB_LIBS). OS_LDFLAGS is
used exclusively for non-library linker flags.
Always pass EXTRA_LIBS before OS_LIBS on linker command lines.
Forbid EXTRA_DSO_LDOPTS, SHARED_LIBRARY_LIBS and LIBS in Makefiles.
https://tbpl.mozilla.org/?tree=Try&rev=90c11a5fa894
Attachment #8458595 -
Flags: review?(gps)
Assignee | ||
Comment 51•10 years ago
|
||
This purposefully doesn't cover host programs and libraries. They are rare enough that this would only add noise, IMHO.
Attachment #8458613 -
Flags: review?(gps)
Updated•10 years ago
|
Attachment #8458589 -
Flags: review?(gps) → review+
Comment 52•10 years ago
|
||
Comment on attachment 8457978 [details] [diff] [review]
part 6 - Emit SimplePrograms for CPP_UNIT_TESTs, and make the corresponding moz.build config look like that of SIMPLE_PROGRAMS
Review of attachment 8457978 [details] [diff] [review]:
-----------------------------------------------------------------
Will grant r+ if you convince me the pattern noted in the other comment must exist.
::: content/base/test/moz.build
@@ +23,3 @@
> ]
>
> +SOURCES += sorted('%s.cpp' % t for t in CPP_UNIT_TESTS)
This pattern is pretty annoying for moz.build authors.
Can we not perform this operation (or similar) inside emitter.py?
Attachment #8457978 -
Flags: review?(gps) → review-
Updated•10 years ago
|
Attachment #8457979 -
Flags: review?(gps) → review+
Updated•10 years ago
|
Attachment #8458613 -
Flags: review?(gps) → review+
Comment 53•10 years ago
|
||
Comment on attachment 8458595 [details] [diff] [review]
part 9 - Replace all EXTRA_DSO_LDOPTS, SHARED_LIBRARY_LIBS and LIBS with EXTRA_LIBS, OS_LIBS or OS_LDFLAGS, appropriately
Review of attachment 8458595 [details] [diff] [review]:
-----------------------------------------------------------------
Do you have any plans to move EXTRA_LIBS, OS_LIBS, and OS_LDFLAGS to moz.build? If nothing else, we can validate that libraries in these aren't a library known to moz.build.
Attachment #8458595 -
Flags: review?(gps) → review+
Comment 54•10 years ago
|
||
Comment on attachment 8458535 [details] [diff] [review]
part 8 - Move most in-tree library linkage information to moz.build, as USE_LIBS
Review of attachment 8458535 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not a huge fan of expanding XPCOM_GLUE_LDOPTS into |USE_LIBS += ['mozalloc', 'xpcomglue_s', 'xul-shared']|. I'd almost rather have a shorthand in moz.build for declaring that. Will the build fail in obvious ways if e.g. mozalloc or xpcomglue_s are omitted from that list? Could we get silent successes? I reckon that can be deferred to a follow-up.
I glanced over large parts of this patch that looked like cookie cutter changes. I did that because bugs in linking configuration should result in obvious build failures.
Attachment #8458535 -
Flags: review?(gps) → review+
Assignee | ||
Comment 55•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #52)
> Comment on attachment 8457978 [details] [diff] [review]
> part 6 - Emit SimplePrograms for CPP_UNIT_TESTs, and make the corresponding
> moz.build config look like that of SIMPLE_PROGRAMS
>
> Review of attachment 8457978 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Will grant r+ if you convince me the pattern noted in the other comment must
> exist.
I won't claim it must exist, but here I'm just looking for consistency with SIMPLE_PROGRAMS (note how the same code is used for both cases). I'm all for changing that, but I'd rather do that in a followup.
(In reply to Gregory Szorc [:gps] from comment #54)
> I'm not a huge fan of expanding XPCOM_GLUE_LDOPTS into |USE_LIBS +=
> ['mozalloc', 'xpcomglue_s', 'xul-shared']|. I'd almost rather have a
> shorthand in moz.build for declaring that.
I'm not a big fan either, but after having written this patch, I'm not a big fan of something like XPCOM_GLUE_LDOPTS either: there were so many places where we actually ended up with xpcomglue_s and xul (or other things) duplicated on the link command line. There are, however, one or two things I want to add that would alleviate this:
- allow intermediate static libs to have shared library dependencies, which would be linked to the final lib (and throw errors if the static lib is linked nowhere where those shared library deps are going to be used)
- define types of programs/shared libraries. Something to say "this is something for gecko", "this is independent", "this is a program with xpcom standalone linkage", etc. which would also replace e.g. resetting MOZ_GLUE_LDFLAGS and/or setting DISABLE_STL_WRAPPING
but I haven't formalized anything yet.
> Will the build fail in obvious ways if e.g. mozalloc or xpcomglue_s
> are omitted from that list? Could we get silent successes? I reckon
> that can be deferred to a follow-up.
I don't think we can get silent successes. You'd get linkage failures (missing symbols) if a lib is missing.
Assignee | ||
Comment 56•10 years ago
|
||
> there were so many places where we actually ended up with xpcomglue_s and
> xul (or other things) duplicated on the link command line
Also, I'm pretty sure there are plenty of those that were only cargo-culting, and are actually useless.
Comment 57•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #55)
> I don't think we can get silent successes. You'd get linkage failures
> (missing symbols) if a lib is missing.
I am mostly concerned with libraries like mozjemalloc that offer a drop-in replacement for existing APIs. Presumably an omission could result in linking against a seemingly compatible library.
Anyway, your comments placated me. I'll grant r+ momentarily.
Comment 58•10 years ago
|
||
Comment on attachment 8457978 [details] [diff] [review]
part 6 - Emit SimplePrograms for CPP_UNIT_TESTs, and make the corresponding moz.build config look like that of SIMPLE_PROGRAMS
Review of attachment 8457978 [details] [diff] [review]:
-----------------------------------------------------------------
Switching to r+ since glandium kinda/sorta has plans to make this suck less.
Attachment #8457978 -
Flags: review- → review+
Assignee | ||
Comment 59•10 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #57)
> (In reply to Mike Hommey [:glandium] from comment #55)
> > I don't think we can get silent successes. You'd get linkage failures
> > (missing symbols) if a lib is missing.
>
> I am mostly concerned with libraries like mozjemalloc that offer a drop-in
> replacement for existing APIs. Presumably an omission could result in
> linking against a seemingly compatible library.
Note it's mozalloc, not mozjemalloc. Mozjemalloc is in mozglue and that's not even handled by USE_LIBS yet. Mozalloc doesn't expose drop-in replacements, it exposes moz_* functions.
Assignee | ||
Comment 60•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9565dc1c02c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c437b9339546
https://hg.mozilla.org/integration/mozilla-inbound/rev/539250f97b4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1befba9483fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/73520ece4b1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce8aee897420
https://hg.mozilla.org/integration/mozilla-inbound/rev/8679d6be0eb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/562e8494fb47
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0988e587a90
https://hg.mozilla.org/integration/mozilla-inbound/rev/108d2af6b1e6
Comment 61•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9565dc1c02c6
https://hg.mozilla.org/mozilla-central/rev/c437b9339546
https://hg.mozilla.org/mozilla-central/rev/539250f97b4c
https://hg.mozilla.org/mozilla-central/rev/1befba9483fb
https://hg.mozilla.org/mozilla-central/rev/73520ece4b1a
https://hg.mozilla.org/mozilla-central/rev/ce8aee897420
https://hg.mozilla.org/mozilla-central/rev/8679d6be0eb5
https://hg.mozilla.org/mozilla-central/rev/562e8494fb47
https://hg.mozilla.org/mozilla-central/rev/a0988e587a90
https://hg.mozilla.org/mozilla-central/rev/108d2af6b1e6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1043128
Comment 62•10 years ago
|
||
Got this with a build for SeaMonkey:
Traceback (most recent call last):
File "./config.status", line 978, in <module>
config_status(**args)
File "/builds/slave/c-cen-t-lnx/build/mozilla/python/mozbuild/mozbuild/config_status.py", line 148, in config_status
summary = the_backend.consume(definitions)
File "/builds/slave/c-cen-t-lnx/build/mozilla/python/mozbuild/mozbuild/backend/base.py", line 186, in consume
for obj in objs:
File "/builds/slave/c-cen-t-lnx/build/mozilla/python/mozbuild/mozbuild/frontend/emitter.py", line 122, in emit
objs = list(self.emit_from_sandbox(out))
File "/builds/slave/c-cen-t-lnx/build/mozilla/python/mozbuild/mozbuild/frontend/emitter.py", line 595, in emit_from_sandbox
'Please remove the latter.', sandbox)
mozbuild.frontend.reader.SandboxValidationError:
==============================
ERROR PROCESSING MOZBUILD FILE
==============================
The error occurred while processing the following file or one of the files it includes:
/builds/slave/c-cen-t-lnx/build/mozilla/extensions/gnomevfs/moz.build
The error occurred when validating the result of the execution. The reported error is:
IS_COMPONENT implies FORCE_SHARED_LIB. Please remove the latter.
configure: error: /builds/slave/c-cen-t-lnx/build/mozilla/configure failed for mozilla
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 63•10 years ago
|
||
Attachment #8466568 -
Flags: review?(mh+mozilla)
Comment 64•10 years ago
|
||
Comment on attachment 8466568 [details] [diff] [review]
gnomevfs moz.build change fix.
r+ from glandium over irc.
Attachment #8466568 -
Flags: review?(mh+mozilla) → review+
Comment 65•10 years ago
|
||
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c94e412090
Updated•10 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 66•10 years ago
|
||
Don't reopen for a fixup that doesn't even affect m-c builds.
Comment 67•10 years ago
|
||
Updated•10 years ago
|
QA 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
•