Closed
Bug 584474
Opened 14 years ago
Closed 14 years ago
Stop building intermediate static libs with fakelibs
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: khuey, Assigned: glandium)
References
Details
(Whiteboard: fixed-in-bs)
Attachments
(12 files, 19 obsolete files)
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
In Bug 522770 fakelibs was implemented, making the intermediate static libraries that we build all over the tree unnecessary, but we still build those libraries. We should stop.
Reporter | ||
Comment 1•14 years ago
|
||
I have the beginnings of a patch for this, but it's tricky since we can't chain response files.
Assignee: nobody → me
Comment 2•14 years ago
|
||
Yeah, I think I hit that when I was looking at this originally. You probably need to expand the contents of the fakelib file. (We already put absolute pathnames in there, right?)
Reporter | ||
Comment 3•14 years ago
|
||
This builds on Windows. Needs some cleanup.
Reporter | ||
Comment 4•14 years ago
|
||
This build a libxul build just fine, but needs quite a bit of work before it'll build a non-libxul build.
Attachment #467246 -
Attachment is obsolete: true
Reporter | ||
Comment 5•14 years ago
|
||
Looping in some c-c people. I'll test SM and TB before landing, but be warned that we've got some substantial build changes in the pipe here.
Comment 6•14 years ago
|
||
Note that the rules.mk and configure.in changes probably will need to be done in the comm-central repo as well, either in this bug or a followup, depending of our build system breaking without them or not.
I'd prefer to have a bustage window that is as small as possible, if not zero.
Reporter | ||
Comment 7•14 years ago
|
||
This builds firefox in libxul and non-libxul configurations on Windows. Should be no net change for other platforms.
Basic idea is to fork buildlist.py to a version that expands multiple levels of fake libs. That way instead of gklayout.lib.fake pointing to gkconbase_s.lib, etc it points to nsDOMFile.obj, etc.
Then we stop building intermediate libs and update all of our dependencies. In order to make this a little easier we move anything third-party-ish to $(DIST)/lib (libjpeg, libbz2, zlib, etc) A lot of our third party libs have symbol conflicts with stuff in our code base or other weirdness so they shouldn't participate in this game. The only thing suboptimal at the moment is that js is linked as a static lib through $(DIST)/lib rather than directly, so js changes will break our incremental linking fun when we get there. I'll post another patch to fix that later.
Attachment #467272 -
Attachment is obsolete: true
Attachment #469905 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 8•14 years ago
|
||
A couple additional fixes to generate a couple of libs that c-c apps depend on. We also turn fakelibs off completely for static builds (no intermediate static libs in a static build doesn't work very well).
Turning fakelibs on in c-c static builds probably requires making fakelibs linking transitive (that is, if liba links to libb, and libc links to liba.fake, that needs to pull in libb before c-c is going to work).
Attachment #470128 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 9•14 years ago
|
||
Another benefit of this is that on Windows we waste time during a PGO build doing Link Time Code Generation for every unnecessary static lib. Especially for gklayout this takes several minutes. Once we only link what we care about that cost will go away.
Comment 10•14 years ago
|
||
Comment on attachment 469905 [details] [diff] [review]
Patch
>diff --git a/browser/app/Makefile.in b/browser/app/Makefile.in
>--- a/browser/app/Makefile.in
>+++ b/browser/app/Makefile.in
>@@ -249,8 +249,6 @@ endif
> endif
> endif
>
>-$(PROGRAM): $(DEPTH)/toolkit/xre/$(LIB_PREFIX)xulapp_s.$(LIB_SUFFIX)
>-
Does this dependency get handled in some other way?
> ifneq (,$(filter-out OS2 WINNT WINCE,$(OS_ARCH)))
>
> $(MOZ_APP_NAME):: $(topsrcdir)/build/unix/mozilla.in $(GLOBAL_DEPS)
>diff --git a/config/buildlist.py b/config/buildliblist.py
>copy from config/buildlist.py
>copy to config/buildliblist.py
>--- a/config/buildlist.py
>+++ b/config/buildliblist.py
>@@ -35,10 +35,11 @@
> #
> # ***** END LICENSE BLOCK *****
>
>-'''A generic script to add entries to a file
>-if the entry does not already exist.
>+'''A slightly less generic script to add entries to a file
>+if the entry does not already exist. This replaces an entry ending in .fake
>+with the contents of that .fake file.
>
>-Usage: buildlist.py <filename> <entry> [<entry> ...]
>+Usage: buildliblist.py <filename> <entry> [<entry> ...]
> '''
Please add a couple of unit tests for this script. You should be able to copy the ones for buildlist.py:
http://mxr.mozilla.org/mozilla-central/source/config/tests/unit-buildlist.py
>
> import sys
>@@ -60,7 +61,12 @@ def addEntriesToListFile(listFile, entri
> f = open(listFile, 'a')
> for e in entries:
> if e not in existing:
>- f.write("%s\n" % e)
>+ if not e.endswith(".fake"):
>+ f.write("%s\n" % e.strip())
>+ else:
>+ g = open(e, 'r')
>+ entries.extend(g.readlines())
>+ g.close()
Since we require Python 2.5 now, you can use with statement here:
with open(e, 'r') as g:
entries.extend(g.readlines())
(you probably have to import it from the future to support Python 2.5.)
Also, 'g' isn't a great name for a file variable.
>diff --git a/config/rules.mk b/config/rules.mk
>--- a/config/rules.mk
>+++ b/config/rules.mk
>@@ -126,6 +126,8 @@ ifdef MOZ_FAKELIBS
> # the library name through unchanged.
> EXPAND_FAKELIBS = $(foreach f,$(1),$(if $(wildcard $(f).fake),@$(wildcard $(f).fake),$(f)))
>
>+EXPAND_FAKELIBS_NO_CMD = $(foreach f,$(1),$(if $(wildcard $(f).fake),$(wildcard $(f).fake),$(f)))
I'm not wild about the naming here. Maybe rename the other one to EXPAND_FAKELIBS_LINKER or something? Also, you could probably make them share the same guts, and just make the other one $(if $(call EXPAND_FAKELIBS,$(1)),@$(call EXPAND_FAKELIBS,$(1)))
>+ifdef FAKE_LIBRARY
>+CANONICAL_LIBRARY = $(FAKE_LIBRARY)
>+else
>+CANONICAL_LIBRARY = $(LIBRARY)
>+endif
Nice, I like the naming and this is a good way to handle the differentiation.
>+# Produce a .fake file that just contains the names of the object files.
> # This can be used as a response file to the linker later instead of
> # linking the actual static library.
> ifdef MOZ_FAKELIBS
> ifndef SUPPRESS_FAKELIB
> ifeq (WINNT_,$(HOST_OS_ARCH)_$(.PYMAKE))
>- echo "$(strip $(foreach f,$(OBJS) $(SEPARATE_OBJS) $(LOBJS) $(SUB_LOBJS),$(subst \,\\,$(call core_winabspath,$(f))))) " > $@.fake
>+ $(PYTHON) $(topsrcdir)/config/buildliblist.py $(FAKE_LIBRARY) $(call EXPAND_FAKELIBS_NO_CMD,$(strip $(foreach f,$(OBJS) $(SEPARATE_OBJS) $(LOBJS) $(SUB_LOBJS),$(call core_winabspath,$(f)))))
> else
>- echo "$(strip $(foreach f,$(OBJS) $(SEPARATE_OBJS) $(LOBJS) $(SUB_LOBJS),$(call core_abspath,$(f)))) " > $@.fake
>+ $(PYTHON) $(topsrcdir)/config/buildliblist.py $(FAKE_LIBRARY) $(call EXPAND_FAKELIBS_NO_CMD,$(strip $(foreach f,$(OBJS) $(SEPARATE_OBJS) $(LOBJS) $(SUB_LOBJS),$(call core_abspath,$(f)))))
The only difference here now is core_winabspath vs. core_abspath, right? You could just define core_winabspath to be core_abspath on non-Windows hosts and unify these.
>diff --git a/embedding/components/printingui/src/Makefile.in b/embedding/components/printingui/src/Makefile.in
>--- a/embedding/components/printingui/src/Makefile.in
>+++ b/embedding/components/printingui/src/Makefile.in
>@@ -63,4 +63,4 @@ DIRS = $(PLATFORM_DIR)
> include $(topsrcdir)/config/rules.mk
>
> libs::
>- $(INSTALL) $(PLATFORM_DIR)/$(LIB_PREFIX)printingui_s.$(LIB_SUFFIX) $(wildcard $(PLATFORM_DIR)/$(LIB_PREFIX)printingui_s.$(LIB_SUFFIX).fake) .
>+ $(INSTALL) $(wildcard $(PLATFORM_DIR)/$(LIB_PREFIX)printingui_s.$(LIB_SUFFIX).fake) .
You did this in an earlier Makefile and I forgot to comment there. Drop the $(wildcard) bit, since if for some reason the fakelib doesn't get built, you'll wind up calling "nsinstall .", which will be a weird error. Better to make nsinstall error trying to install a file that doesn't exist, which is at least a clear error.
>diff --git a/toolkit/library/Makefile.in b/toolkit/library/Makefile.in
>--- a/toolkit/library/Makefile.in
>+++ b/toolkit/library/Makefile.in
>@@ -245,6 +245,9 @@ OS_LIBS += $(call EXPAND_LIBNAME,shell32
> ifneq (,$(MOZ_DEBUG)$(NS_TRACE_MALLOC))
> OS_LIBS += $(call EXPAND_LIBNAME,imagehlp)
> endif
>+ifdef MOZ_CRASHREPORTER
>+OS_LIBS += $(call EXPAND_LIBNAME,wininet)
>+endif
The fact that you had to add this means that the linker treats linking directly to the object files differently to linking to the static libs. I guess it can no longer omit dead code the same way? We've always been linking to this same code via the static lib, it's just not used in libxul. Will this have a negative impact on codesize? (Also I wonder how that works wrt PGO, I can't remember if PGO can completely drop dead code for you.)
>diff --git a/xpcom/typelib/xpt/tests/Makefile.in b/xpcom/typelib/xpt/tests/Makefile.in
>--- a/xpcom/typelib/xpt/tests/Makefile.in
>+++ b/xpcom/typelib/xpt/tests/Makefile.in
>@@ -50,7 +50,7 @@ SIMPLE_PROGRAMS = PrimitiveTest$(BIN_SUF
> CSRCS = PrimitiveTest.c SimpleTypeLib.c
>
> LIBS = \
>- $(DIST)/lib/$(LIB_PREFIX)xpt.$(LIB_SUFFIX) \
>+ ../src/$(LIB_PREFIX)xpt.$(LIB_SUFFIX) \
> $(NULL)
Just reformat this to two-space indent while you're here (since you're touching the only useful line in the variable declaration).
Looks good, r=me with those nits fixed.
Attachment #469905 -
Flags: review?(ted.mielczarek) → review+
Comment 11•14 years ago
|
||
Comment on attachment 470128 [details] [diff] [review]
Patch for m-c to make c-c build
>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
>@@ -324,17 +324,17 @@ endif
> endif
>
> ifndef STATIC_LIBRARY_NAME
> ifdef LIBRARY_NAME
> STATIC_LIBRARY_NAME=$(LIBRARY_NAME)
> endif
> endif
>
>-ifeq (WINNT,$(OS_ARCH))
>+ifeq (WINNT_,$(OS_ARCH)_$(BUILD_STATIC_LIBS))
> MOZ_FAKELIBS = 1
> endif
Is this just an intermediate step to supporting fakelibs for static builds, or what? Do you expect we'll be able to support that config once the static-libxul work is done?
>diff --git a/toolkit/xre/Makefile.in b/toolkit/xre/Makefile.in
>--- a/toolkit/xre/Makefile.in
>+++ b/toolkit/xre/Makefile.in
>@@ -41,16 +41,17 @@ topsrcdir = @top_srcdir@
> srcdir = @srcdir@
> VPATH = @srcdir@
>
> include $(DEPTH)/config/autoconf.mk
>
> MODULE = xulapp
> LIBRARY_NAME = xulapp_s
> LIBXUL_LIBRARY = 1
>+SUPPRESS_FAKELIB = 1
This is a bummer, since I wind up rebuilding here all the time when hacking crashreporter. (The crashreporter libs get linked into toolkit/xre, which gets linked into libxul).
Attachment #470128 -
Flags: review?(ted.mielczarek) → review+
Reporter | ||
Comment 12•14 years ago
|
||
(In reply to comment #10)
> Does this dependency get handled in some other way?
Should be in LIBS_DEPS. I will verify.
> Please add a couple of unit tests for this script. You should be able to copy
> the ones for buildlist.py:
> http://mxr.mozilla.org/mozilla-central/source/config/tests/unit-buildlist.py
OK.
> Since we require Python 2.5 now, you can use with statement here:
> with open(e, 'r') as g:
> entries.extend(g.readlines())
> (you probably have to import it from the future to support Python 2.5.)
Ah, neat.
> Also, 'g' isn't a great name for a file variable.
Indeed
> I'm not wild about the naming here. Maybe rename the other one to
> EXPAND_FAKELIBS_LINKER or something? Also, you could probably make them share
> the same guts, and just make the other one $(if $(call
> EXPAND_FAKELIBS,$(1)),@$(call EXPAND_FAKELIBS,$(1)))
Sure.
> The only difference here now is core_winabspath vs. core_abspath, right? You
> could just define core_winabspath to be core_abspath on non-Windows hosts and
> unify these.
We should probably call it something like core_nativeabspath or somethin.
> You did this in an earlier Makefile and I forgot to comment there. Drop the
> $(wildcard) bit, since if for some reason the fakelib doesn't get built, you'll
> wind up calling "nsinstall .", which will be a weird error. Better to make
> nsinstall error trying to install a file that doesn't exist, which is at least
> a clear error.
Sure.
> The fact that you had to add this means that the linker treats linking directly
> to the object files differently to linking to the static libs. I guess it can
> no longer omit dead code the same way? We've always been linking to this same
> code via the static lib, it's just not used in libxul. Will this have a
> negative impact on codesize? (Also I wonder how that works wrt PGO, I can't
> remember if PGO can completely drop dead code for you.)
Linking is no longer transitive. When I link foo.dll by way of bar.lib, all of the import symbols are resolved in bar.lib. When foo.dll links directory to bar/nsBox.obj and bar/nsCircle.obj those import symbols are no longer resolved in the first link (because it no longer happens). When foo.dll is linked the linker needs to resolve all of the import symbols, even if the code that uses them will be discarded. I would think that the linker does The Right Thing (TM) here, but I'll verify.
> Just reformat this to two-space indent while you're here (since you're touching
> the only useful line in the variable declaration).
OK.
Reporter | ||
Comment 13•14 years ago
|
||
s/directory/directly above.
(In reply to comment #11)
> Is this just an intermediate step to supporting fakelibs for static builds, or
> what? Do you expect we'll be able to support that config once the static-libxul
> work is done?
This was a while ago, but IIRC building comm-central statically with fakelibs is hopeless until fakelibs are ported to c-c (because everything of interest in m-c is a fakelib). I don't remember any showstopping obstacles.
> This is a bummer, since I wind up rebuilding here all the time when hacking
> crashreporter. (The crashreporter libs get linked into toolkit/xre, which gets
> linked into libxul).
As Callek would be more than happy to point out, the "right" thing to do is to port fakelibs to c-c. I was reluctant to do that before this patch was finalized.
Comment 14•14 years ago
|
||
(In reply to comment #10)
> Also, 'g' isn't a great name for a file variable.
Actually, single-letter variables might be cutely short, but lead to confusion and bad readability very fast, we should really use more logical variable names (i.e. ones where a reader immediately know what they represent) where possible.
Reporter | ||
Comment 15•14 years ago
|
||
(In reply to comment #14)
> (In reply to comment #10)
> > Also, 'g' isn't a great name for a file variable.
>
> Actually, single-letter variables might be cutely short, but lead to confusion
> and bad readability very fast, we should really use more logical variable names
> (i.e. ones where a reader immediately know what they represent) where possible.
I don't think anybody disagrees.
Comment 16•14 years ago
|
||
I agree in general, but I do tend to use 'f' as a variable for an open file object in Python for terseness. I think it's fairly clear what's going on in those cases.
Comment 17•14 years ago
|
||
(In reply to comment #13)
> As Callek would be more than happy to point out, the "right" thing to do is to
> port fakelibs to c-c. I was reluctant to do that before this patch was
> finalized.
Okay, so the c-c port would resolve these issues. That's fine.
Reporter | ||
Comment 18•14 years ago
|
||
Nits picked
Attachment #469905 -
Attachment is obsolete: true
Attachment #482094 -
Flags: review+
Assignee | ||
Comment 19•14 years ago
|
||
FYI, using linker scripts instead of passing all the object files to gcc when building libxul.so would allow to avoid the command line limitation problems with gcc 4.3. It is possible to pass linker scripts that don't override the default ld linker script by giving it as an object file instead of as an option (-T). Such linker script could contain INPUT commands for each actual object file to include.
As I'd need something pretty similar to fakelibs for objects reordering, I'll do some attempts on the next few days.
Assignee | ||
Comment 20•14 years ago
|
||
This was already in khuey's patch, and is probably due to the final linkage not doing LTO. I'll keep it here for now, until I clear things up.
Assignee | ||
Comment 21•14 years ago
|
||
This reverses the way some libraries are currently copied from a subdirectory to a top directory, such that there is no further need to change these manual exports in the future (cf. upcoming patch for a fakelibs refactoring).
Attachment #483986 -
Flags: review?(khuey)
Assignee | ||
Comment 22•14 years ago
|
||
The dependency on xulapp_s for the browser doesn't apply in all cases, for a starter, and is already taken care of by the presence of that library in the LIBS variable (which, by means of LIBS_DEPS, takes care of the actual dependency)
Attachment #483987 -
Flags: review?(khuey)
Assignee | ||
Comment 23•14 years ago
|
||
This patch does 2 things:
- Unsets AR_EXTRACT on platforms where there is no extraction of libraries (and where "$(AR) x" doesn't work
- Puts AR flags in the AR_FLAGS variable instead of AR, where only the program should be present.
Attachment #483988 -
Flags: review?(khuey)
Assignee | ||
Comment 24•14 years ago
|
||
This approach has various advantages:
- Takes some complexity off rules.mk
- Almost works everywhere, and on all platforms
- Allows an easier hook for bug 603370
- Also allows further improvements such as transitive linking (meaning a lot of the EXTRA_DSO_LDOPTS complexity in toolkit/library could go away)
The expandlibs.py implementation, OTOH, is quite hacky, definitely needs a rewrite and a unit test.
Assignee | ||
Comment 25•14 years ago
|
||
Like wininet on windows, crypto is needed on osx, debug builds only.
Assignee | ||
Comment 26•14 years ago
|
||
(In reply to comment #24)
> The expandlibs.py implementation, OTOH, is quite hacky, definitely needs a
> rewrite and a unit test.
I should also mention that I'm not sure if I broke the dtrace part of rules.mk. As I didn't find anything using MOZILLA_PROBE_LIBS, I'm not sure how to test it.
Reporter | ||
Comment 27•14 years ago
|
||
I should get to these reviews this weekend.
Assignee | ||
Updated•14 years ago
|
Attachment #483985 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•14 years ago
|
Attachment #483994 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 28•14 years ago
|
||
Comment on attachment 483989 [details] [diff] [review]
part4 - WIP - fakelibs refactoring
We would like to get this landed soon, even if it's not entirely clean yet (but it works properly), and clean the expandlibs.py implementation later.
Attachment #483989 -
Flags: review?(khuey)
Reporter | ||
Comment 29•14 years ago
|
||
I can review it soon (for real this time) but idk that we're going to want to take these changes before branch.
Reporter | ||
Comment 30•14 years ago
|
||
Comment on attachment 483987 [details] [diff] [review]
part 2 - Remove useless make dependency in browser/app/Makefile.in
I vaguely remember xulrunner needing the same patch.
Attachment #483987 -
Flags: review?(khuey) → review+
Reporter | ||
Comment 31•14 years ago
|
||
Comment on attachment 483986 [details] [diff] [review]
part 1 - Use a special value of EXPORT_LIBRARY to copy libraries in other directories
I love the idea behind this change, but I don't like the $(filter-out 1,$(EXPORT_LIBRARY) business. Can we add
> ifeq ($(EXPORT_LIBRARY), 1)
> ifdef IS_COMPONENT
> EXPORT_LIBRARY = $(DEPTH)/staticlib/components
> else
> EXPORT_LIBRARY = $(DEPTH)/staticlib
> endif
> endif
?
As an aside, I'm not sure whether there's a compelling reason for the distinction between components and non-components in $(DEPTH)/staticlibs.
Reporter | ||
Comment 32•14 years ago
|
||
Comment on attachment 483988 [details] [diff] [review]
part 3 - Fix AR related variables in configure.in
On what platforms are we still using AR_EXTRACT? I would think that ideally we'd ditch that with fakelibs.
Assignee | ||
Comment 33•14 years ago
|
||
(In reply to comment #30)
> Comment on attachment 483987 [details] [diff] [review]
> part 2 - Remove useless make dependency in browser/app/Makefile.in
>
> I vaguely remember xulrunner needing the same patch.
At first glance it doesn't.
(In reply to comment #32)
> Comment on attachment 483988 [details] [diff] [review]
> part 3 - Fix AR related variables in configure.in
>
> On what platforms are we still using AR_EXTRACT? I would think that ideally
> we'd ditch that with fakelibs.
Actually, all where it's defined, because libffi.a is not built with the mozilla rules, and there could be, at any moment, other libs builds without the mozilla rules. While it technically isn't strictly necessary to extract the objects before linking them (we could link the libs directly), it is iirc necessary for bug 603370.
(In reply to comment #31)
> Comment on attachment 483986 [details] [diff] [review]
> part 1 - Use a special value of EXPORT_LIBRARY to copy libraries in other
> directories
>
> I love the idea behind this change, but I don't like the $(filter-out
> 1,$(EXPORT_LIBRARY) business. Can we add
> > ifeq ($(EXPORT_LIBRARY), 1)
> > ifdef IS_COMPONENT
> > EXPORT_LIBRARY = $(DEPTH)/staticlib/components
> > else
> > EXPORT_LIBRARY = $(DEPTH)/staticlib
> > endif
> > endif
> ?
Will do
> As an aside, I'm not sure whether there's a compelling reason for the
> distinction between components and non-components in $(DEPTH)/staticlibs.
I really have no idea. I just kept it as it was.
Reporter | ||
Comment 34•14 years ago
|
||
Comment on attachment 483989 [details] [diff] [review]
part4 - WIP - fakelibs refactoring
Not explicitly r-ing because this looks really good, but I would like to see another revision before r+ing.
>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
>@@ -78,21 +78,16 @@ CHECK_VARS := \
> # checks for internal spaces or trailing spaces in the variable
> # named by $x
> check-variable = $(if $(filter-out 0 1,$(words $($(x))z)),$(error Spaces are not allowed in $(x)))
>
> $(foreach x,$(CHECK_VARS),$(check-variable))
>
> core_abspath = $(if $(findstring :,$(1)),$(1),$(if $(filter /%,$(1)),$(1),$(CURDIR)/$(1)))
>
>-nullstr :=
>-space :=$(nullstr) # EOL
>-
>-core_winabspath = $(firstword $(subst /, ,$(call core_abspath,$(1)))):$(subst $(space),,$(patsubst %,\\%,$(wordlist 2,$(words $(subst /, ,$(call core_abspath,$(1)))), $(strip $(subst /, ,$(call core_abspath,$(1)))))))
>-
> # FINAL_TARGET specifies the location into which we copy end-user-shipped
> # build products (typelibs, components, chrome).
> #
> # It will usually be the well-loved $(DIST)/bin, today, but can also be an
> # XPI-contents staging directory for ambitious and right-thinking extensions.
> FINAL_TARGET = $(if $(XPI_NAME),$(DIST)/xpi-stage/$(XPI_NAME),$(DIST)/bin)
>
> ifdef XPI_NAME
OMG TABS!! Turn all of these tabs into 4 space indents. Also, fill in the license boilerplate appropriately. This script is a bit hard to understand, would be easier if things were broken into functions.
>diff --git a/config/expandlibs.py.in b/config/expandlibs.py.in
>+class expand(object):
>+ def __init__(self, file, ar_mode):
>+ self.tmp = ""
>+ if not file.endswith(LIB_SUFFIX):
>+ self.objs = [file]
>+ return
>+ if len(IMPORT_LIB_SUFFIX):
>+ if os.path.exists(file[:-len(LIB_SUFFIX)] + "." + IMPORT_LIB_SUFFIX):
>+ self.objs = [file[:-len(LIB_SUFFIX)] + "." + IMPORT_LIB_SUFFIX]
Assuming that there's nothing in the standard library to do this, can you split this extension swap logic out into a separate function?
>+ return
>+ else:
>+ if os.path.exists(file[:-len(LIB_SUFFIX)] + DLL_SUFFIX):
>+ self.objs = [file[:-len(LIB_SUFFIX)] + DLL_SUFFIX]
>+ return
>+ if not ar_mode and (os.path.exists(file) or not os.path.exists(file + ".objs")):
>+ self.objs = [file]
>+ return
>+ if ar_mode and not os.path.exists(file + ".objs"):
>+ if not os.path.exists(file) or len(AR_EXTRACT) == 0:
>+ self.objs = [file]
>+ return
>+ self.tmp = tempfile.mkdtemp(dir=os.curdir)
>+ subprocess.call(AR_EXTRACT + [os.path.abspath(file)], cwd=self.tmp)
>+ self.objs = []
>+ for root, dirs, files in os.walk(self.tmp):
>+ self.objs += map(lambda f: os.path.join(root, f), [f for f in files if f.endswith(OBJ_SUFFIX)])
>+ return
>+ self.objs = []
>+ f = open(file + ".objs")
>+ for line in f.readlines():
>+ line.strip()
>+ [name, value] = re.split(' *= *', line, 2)
>+ if name == "OBJS":
>+ self.objs += value.split()
>+ if name == "LIBS":
>+ for lib in value.split():
>+ self.objs += expand(lib, ar_mode).objs
>+ f.close()
>+
>+if __name__ == '__main__':
>+ cmd = []
>+ expands = []
>+ print_only = 0;
>+ ar_mode = 0;
>+ linkerscript = 0;
>+ tmps = []
>+ del sys.argv[0]
>+ if sys.argv[0] == "--ld":
>+ linkerscript = 1;
>+ del sys.argv[0]
>+ if sys.argv[0] == "--gen":
>+ objs = []
>+ libs = []
>+ for f in sys.argv[1:]:
>+ if f.endswith(OBJ_SUFFIX):
>+ objs += [os.path.abspath(f)]
>+ elif f.endswith(LIB_SUFFIX):
>+ libs += [os.path.abspath(f)]
>+ if len(objs) > 0:
>+ print "OBJS = " + " ".join(objs)
>+ if len(libs) > 0:
>+ print "LIBS = " + " ".join(libs)
>+ exit(0)
>+ elif sys.argv[0] == "--print":
>+ print_only = 1;
>+ del sys.argv[0]
>+ elif sys.argv[0] == "--ar":
>+ ar_mode = 1;
>+ del sys.argv[0]
>+ for arg in sys.argv:
>+ e = expand(arg, ar_mode)
>+ expands += [e]
>+ if e.objs != [arg] and not print_only and not ar_mode:
>+ fd, tmp = tempfile.mkstemp(suffix=".list",dir=os.curdir)
>+ f = os.fdopen(fd, "w")
>+ if linkerscript:
>+ f.writelines(map(lambda f: "INPUT(" + f + ")\n", e.objs))
>+ print tmp
>+ print "".join(map(lambda f: " INPUT(" + f + ")\n", e.objs))
>+ cmd += [tmp]
>+ else:
>+ f.writelines(map(lambda f: f + "\n", e.objs))
>+ print tmp
>+ print "".join(map(lambda f: " " + f + "\n", e.objs))
>+ cmd += ["@"+tmp]
>+ f.close();
>+ tmps += [tmp]
>+ else:
>+ cmd += e.objs
>+ if print_only:
>+ sys.stderr.write(str(cmd)+"\n")
>+ print " ".join(cmd)
>+ else:
>+ print cmd
>+ retval = subprocess.call(cmd)
>+ if not ar_mode:
>+ for tmp in tmps:
>+ os.remove(tmp)
>+ else:
>+ for e in expands:
>+ if e.tmp:
>+ shutil.rmtree(e.tmp, True)
>+ exit(retval)
Somebody else should probably take a look at the linker script bits. They LGTM but I don't know much about them.
>diff --git a/config/rules.mk b/config/rules.mk
>@@ -245,21 +218,22 @@ endif # ENABLE_TESTS
> #
> # If BUILD_STATIC_LIBS or FORCE_STATIC_LIB is set, build a static library.
> # Otherwise, build a shared library.
> #
>
> ifndef LIBRARY
> ifdef STATIC_LIBRARY_NAME
> LIBRARY := $(LIB_PREFIX)$(STATIC_LIBRARY_NAME).$(LIB_SUFFIX)
>-ifdef MOZ_FAKELIBS
>-ifndef SUPPRESS_FAKELIB
>-FAKE_LIBRARY = $(LIBRARY).fake
>-endif # SUPPRESS_FAKELIB
>-endif # MOZ_FAKELIBS
>+# Only build actual library if it is installed in DIST/lib or SDK
>+ifeq (,$(SDK_LIBRARY)$(DIST_INSTALL)$(NO_EXPAND_LIBS))
>+LIBRARY := $(LIBRARY).objs
>+else
>+LIBRARY += $(LIBRARY).objs
>+endif
> endif # STATIC_LIBRARY_NAME
> endif # LIBRARY
>
> ifndef HOST_LIBRARY
> ifdef HOST_LIBRARY_NAME
> HOST_LIBRARY := $(LIB_PREFIX)$(HOST_LIBRARY_NAME).$(LIB_SUFFIX)
> endif
> endif
Condense the logic here so that if we're not building the actual library we don't set LIBRARY and then overwrite it.
>@@ -847,37 +811,31 @@ ifdef MODULE_NAME
> endif
> endif # BUILD_STATIC_LIBS
> else # !IS_COMPONENT
> $(PYTHON) $(MOZILLA_DIR)/config/buildlist.py $(FINAL_LINK_LIBS) $(STATIC_LIBRARY_NAME)
> endif # IS_COMPONENT
> endif # EXPORT_LIBRARY
> endif # LIBRARY_NAME
>
>+ifneq (,$(filter-out %.$(LIB_SUFFIX),$(SHARED_LIBRARY_LIBS)))
>+$(error SHARED_LIBRARY_LIBS must contain .$(LIB_SUFFIX) files only)
>+endif
>+
>+EXPAND_LIBS := $(PYTHON) $(DEPTH)/config/expandlibs.py
>+ifeq ($(filter-out Linux Android,$(OS_TARGET))$(GNU_CC),1)
>+EXPAND_LIBS += --ld
>+endif
>+
> # Create dependencies on static (and shared EXTRA_DSO_LIBS) libraries
>-LIBS_DEPS = $(filter %.$(LIB_SUFFIX), $(LIBS))
>-HOST_LIBS_DEPS = $(filter %.$(LIB_SUFFIX), $(HOST_LIBS))
>-DSO_LDOPTS_DEPS = $(EXTRA_DSO_LIBS) $(filter %.$(LIB_SUFFIX), $(EXTRA_DSO_LDOPTS))
>-
>-ifndef _LIBNAME_RELATIVE_PATHS
>-
>-LIBS_DEPS += $(filter -l%, $(LIBS))
>-HOST_LIBS_DEPS += $(filter -l%, $(HOST_LIBS))
>-DSO_LDOPTS_DEPS += $(filter -l%, $(EXTRA_DSO_LDOPTS))
>-
>-_LIBDIRS = $(patsubst -L%,%,$(filter -L%, $(LIBS) $(HOST_LIBS) $(EXTRA_DSO_LDOPTS)))
>-ifneq (,$(_LIBDIRS))
>-vpath $(LIB_PREFIX)%.$(LIB_SUFFIX) $(_LIBDIRS)
>-ifdef IMPORT_LIB_SUFFIX
>-vpath $(LIB_PREFIX)%.$(IMPORT_LIB_SUFFIX) $(_LIBDIRS)
>-endif # IMPORT_LIB_SUFFIX
>-vpath $(DLL_PREFIX)%$(DLL_SUFFIX) $(_LIBDIRS)
>-endif # _LIBDIRS
>-
>-endif # _LIBNAME_RELATIVE_PATHS
>+DO_EXPAND_LIBS = $(if $(1),$(shell $(EXPAND_LIBS) --print $(1)))
>+LIBS_DEPS = $(call DO_EXPAND_LIBS,$(filter %.$(LIB_SUFFIX),$(LIBS)))
>+SHARED_LIBRARY_LIBS_DEPS = $(call DO_EXPAND_LIBS,$(SHARED_LIBRARY_LIBS))
>+HOST_LIBS_DEPS = $(filter %.$(LIB_SUFFIX),$(HOST_LIBS))
>+DSO_LDOPTS_DEPS = $(call DO_EXPAND_LIBS,$(EXTRA_DSO_LIBS) $(filter %.$(LIB_SUFFIX), $(EXTRA_DSO_LDOPTS)))
>
> # Dependancies which, if modified, should cause everything to rebuild
> GLOBAL_DEPS += Makefile Makefile.in $(DEPTH)/config/autoconf.mk $(topsrcdir)/config/config.mk
>
> ##############################################
> ifdef PARALLEL_DIRS
> libs:: $(PARALLEL_DIRS_libs)
>
I'm not crazy about the shell invocation here (that's really expensive on Windows) but I don't see an obvious way around it.
>@@ -885,19 +843,19 @@ libs:: $(PARALLEL_DIRS_libs)
> +@$(call SUBMAKE,libs,$*)
> endif
>
> libs:: $(SUBMAKEFILES) $(MAKE_DIRS) $(HOST_LIBRARY) $(LIBRARY) $(SHARED_LIBRARY) $(IMPORT_LIBRARY) $(HOST_PROGRAM) $(PROGRAM) $(HOST_SIMPLE_PROGRAMS) $(SIMPLE_PROGRAMS) $(JAVA_LIBRARY)
> ifndef NO_DIST_INSTALL
> ifdef LIBRARY
> ifdef EXPORT_LIBRARY # Stage libs that will be linked into a static build
> ifdef IS_COMPONENT
>- $(INSTALL) $(IFLAGS1) $(LIBRARY) $(FAKE_LIBRARY) $(if $(filter-out 1,$(EXPORT_LIBRARY)),$(EXPORT_LIBRARY),$(DEPTH)/staticlib/components)
>+ $(INSTALL) $(IFLAGS1) $(LIBRARY) $(if $(filter-out 1,$(EXPORT_LIBRARY)),$(EXPORT_LIBRARY),$(DEPTH)/staticlib/components)
> else
>- $(INSTALL) $(IFLAGS1) $(LIBRARY) $(FAKE_LIBRARY) $(if $(filter-out 1,$(EXPORT_LIBRARY)),$(EXPORT_LIBRARY),$(DEPTH)/staticlib)
>+ $(INSTALL) $(IFLAGS1) $(LIBRARY) $(if $(filter-out 1,$(EXPORT_LIBRARY)),$(EXPORT_LIBRARY),$(DEPTH)/staticlib)
> endif
> endif # EXPORT_LIBRARY
> ifdef DIST_INSTALL
> ifdef IS_COMPONENT
> $(error Shipping static component libs makes no sense.)
> else
> $(INSTALL) $(IFLAGS1) $(LIBRARY) $(DIST)/lib
> endif
The same thing I said about $(filter-out on EXPORT_LIBRARY on the previous patches still applies.
>@@ -1312,73 +1210,39 @@ ifeq ($(OS_ARCH),OpenVMS)
> fi
> ifdef IS_COMPONENT
> @if test ! -f $(VMS_SYMVEC_FILE); then \
> echo Creating generic component options file $(VMS_SYMVEC_FILE); \
> cp $(VMS_SYMVEC_FILE_COMP) $(VMS_SYMVEC_FILE); \
> fi
> endif
> endif # OpenVMS
>-ifdef NO_LD_ARCHIVE_FLAGS
>-ifdef SHARED_LIBRARY_LIBS
>- @rm -f $(SUB_SHLOBJS)
>- @for lib in $(SHARED_LIBRARY_LIBS); do $(AR_EXTRACT) $${lib}; $(CLEANUP2); done
>-ifeq ($(OS_ARCH),Darwin)
>- @echo Making symlinks to the original object files in the archive libraries $(SHARED_LIBRARY_LIBS)
>- @for lib in $(SHARED_LIBRARY_LIBS); do \
>- libdir=`echo $$lib|sed -e 's,/[^/]*\.a,,'`; \
>- ofiles=`$(AR_LIST) $${lib}`; \
>- for ofile in $$ofiles; do \
>- if [ -f $$libdir/$$ofile ]; then \
>- rm -f $$ofile; \
>- ln -s $$libdir/$$ofile $$ofile; \
>- fi; \
>- done; \
>- done
>+ifdef DTRACE_LIB_DEPENDENT
>+ifndef XP_MACOSX
>+ dtrace -G -C -s $(MOZILLA_DTRACE_SRC) -o $(DTRACE_PROBE_OBJ) $(shell $(EXPAND_LIBS) --print $(MOZILLA_PROBE_LIBS))
> endif
>-endif # SHARED_LIBRARY_LIBS
>-endif # NO_LD_ARCHIVE_FLAGS
>-ifdef DTRACE_LIB_DEPENDENT
>- @rm -f $(PROBE_LOBJS)
>- @for lib in $(MOZILLA_PROBE_LIBS); do $(AR_EXTRACT) $${lib}; $(CLEANUP2); done
>-ifndef XP_MACOSX
>- dtrace -G -C -s $(MOZILLA_DTRACE_SRC) -o $(DTRACE_PROBE_OBJ) $(PROBE_LOBJS)
>-endif
>- @for lib in $(MOZILLA_PROBE_LIBS); do \
>- ofiles=`$(AR_LIST) $${lib}`; \
>- $(AR_DELETE) $${lib} $$ofiles; \
>- done
>- $(MKSHLIB) $(SHLIB_LDSTARTFILE) $(OBJS) $(LOBJS) $(SUB_SHLOBJS) $(DTRACE_PROBE_OBJ) $(PROBE_LOBJS) $(RESFILE) $(LDFLAGS) $(EXTRA_DSO_LDOPTS) $(call EXPAND_FAKELIBS,$(OS_LIBS) $(EXTRA_LIBS)) $(DEF_FILE) $(SHLIB_LDENDFILE)
>- @rm -f $(PROBE_LOBJS)
>+ $(EXPAND_LIBS) $(MKSHLIB) $(SHLIB_LDSTARTFILE) $(OBJS) $(LOBJS) $(DTRACE_PROBE_OBJ) $(MOZILLA_PROBE_LIBS) $(RESFILE) $(LDFLAGS) $(if $(SHARED_LIBRARY_LIBS),$(MKSHLIB_FORCE_ALL) $(SHARED_LIBRARY_LIBS) $(MKSHLIB_UNFORCE_ALL)) $(EXTRA_DSO_LDOPTS) $(OS_LIBS) $(EXTRA_LIBS) $(DEF_FILE) $(SHLIB_LDENDFILE)
> @rm -f $(DTRACE_PROBE_OBJ)
>- @for lib in $(MOZILLA_PROBE_LIBS); do \
>- if [ -L $${lib} ]; then rm -f `readlink $${lib}`; fi; \
>- done
>- @rm -f $(MOZILLA_PROBE_LIBS)
>-
> else # ! DTRACE_LIB_DEPENDENT
>- $(MKSHLIB) $(SHLIB_LDSTARTFILE) $(OBJS) $(DTRACE_PROBE_OBJ) $(LOBJS) $(SUB_SHLOBJS) $(RESFILE) $(LDFLAGS) $(EXTRA_DSO_LDOPTS) $(call EXPAND_FAKELIBS,$(OS_LIBS) $(EXTRA_LIBS)) $(DEF_FILE) $(SHLIB_LDENDFILE)
>+ $(EXPAND_LIBS) $(MKSHLIB) $(SHLIB_LDSTARTFILE) $(OBJS) $(DTRACE_PROBE_OBJ) $(LOBJS) $(RESFILE) $(LDFLAGS) $(if $(SHARED_LIBRARY_LIBS),$(MKSHLIB_FORCE_ALL) $(SHARED_LIBRARY_LIBS) $(MKSHLIB_UNFORCE_ALL)) $(EXTRA_DSO_LDOPTS) $(OS_LIBS) $(EXTRA_LIBS) $(DEF_FILE) $(SHLIB_LDENDFILE)
> endif # DTRACE_LIB_DEPENDENT
>
> ifeq (_WINNT,$(GNU_CC)_$(OS_ARCH))
> ifdef MSMANIFEST_TOOL
> ifdef EMBED_MANIFEST_AT
> @if test -f $@.manifest; then \
> mt.exe -NOLOGO -MANIFEST $@.manifest -OUTPUTRESOURCE:$@\;$(EMBED_MANIFEST_AT); \
> rm -f $@.manifest; \
> fi
> endif # EMBED_MANIFEST_AT
> endif # MSVC with manifest tool
> ifdef MOZ_PROFILE_GENERATE
> touch -t `date +%Y%m%d%H%M.%S -d "now+5seconds"` pgo.relink
> endif
> endif # WINNT && !GCC
>-ifneq ($(OS_ARCH),Darwin)
>- @rm -f $(SUB_SHLOBJS)
>-endif # Darwin
> @rm -f foodummyfilefoo $(DELETE_AFTER_LINK)
> chmod +x $@
> ifdef ENABLE_STRIP
> $(STRIP) $@
> endif
> ifdef MOZ_POST_DSO_LIB_COMMAND
> $(MOZ_POST_DSO_LIB_COMMAND) $@
> endif
Remove SHLIB_LD[START|END]FILE. As far as I can tell that's cruft left over from the ancient days.
I assume the linkerscript magic takes care of the platforms without --whole-archive?
Attachment #483989 -
Flags: review?(khuey)
Reporter | ||
Updated•14 years ago
|
Attachment #483988 -
Flags: review?(khuey) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #483986 -
Flags: review?(khuey)
Reporter | ||
Updated•14 years ago
|
Attachment #483985 -
Flags: review?(ted.mielczarek) → review+
Comment 35•14 years ago
|
||
Comment on attachment 483994 [details] [diff] [review]
Link crypto library on mac debug builds
Why is this debug only?
Attachment #483994 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 36•14 years ago
|
||
(In reply to comment #35)
> Comment on attachment 483994 [details] [diff] [review]
> Link crypto library on mac debug builds
>
> Why is this debug only?
Because it only failed on debug builds on tryserver when I tried.
Comment 37•14 years ago
|
||
That is...worrisome.
Assignee | ||
Comment 38•14 years ago
|
||
With comment 31 addressed. There had been a few changes on m-c in the Makefile.in this patch changed, adding the copied library/fake library to GARBAGE, so I just removed this logic from the Makefiles and added it in rules.mk when using EXPORT_LIBRARY != 1.
Assignee: khuey → mh+mozilla
Attachment #483986 -
Attachment is obsolete: true
Attachment #491474 -
Flags: review?(khuey)
Assignee | ||
Comment 39•14 years ago
|
||
Unbitrotted (small context change). Carrying r+ over.
Attachment #483988 -
Attachment is obsolete: true
Attachment #491475 -
Flags: review+
Reporter | ||
Updated•14 years ago
|
Attachment #491474 -
Flags: review?(khuey) → review+
Assignee | ||
Updated•14 years ago
|
OS: Windows 7 → All
Hardware: x86 → All
Assignee | ||
Comment 40•14 years ago
|
||
Comment on attachment 483994 [details] [diff] [review]
Link crypto library on mac debug builds
This doesn't work since bug 599475 (http://hg.mozilla.org/mozilla-central/rev/0012ca751ce1)
Attachment #483994 -
Attachment is obsolete: true
Assignee | ||
Comment 41•14 years ago
|
||
In the previous iteration, there was a missing AR_EXTRACT reset for msvc in js/src/configure.in ; the AR_EXTRACT reset should also not be necessary on platforms using emxomfar.
Attachment #491475 -
Attachment is obsolete: true
Attachment #493252 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 42•14 years ago
|
||
Unbitrot (context change only)
Attachment #483985 -
Attachment is obsolete: true
Assignee | ||
Comment 43•14 years ago
|
||
While only crypto was necessary before, now the internal md5 implementation that ends up in breadpad_common_s is necessary too after bug 599475.
Attachment #493254 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•14 years ago
|
Attachment #470128 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #482094 -
Attachment is obsolete: true
Assignee | ||
Comment 44•14 years ago
|
||
There are two instances of test Makefiles that list xpcomglue_s in LIBS, while config/rules.mk already adds them for CPP_UNIT_TESTS.
Attachment #493255 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 45•14 years ago
|
||
Comment 24 (except the comment about expandlibs.py, which is now clean) and Comment 26 still apply. This was successfully tested on try.
Attachment #483989 -
Attachment is obsolete: true
Attachment #493256 -
Flags: review?(ted.mielczarek)
Comment 46•14 years ago
|
||
Comment on attachment 493252 [details] [diff] [review]
part 3 - Fix AR related variables in configure.in
Why are you unsetting AR_EXTRACT here? Is it just because we don't intend to build real static libs anymore?
Updated•14 years ago
|
Attachment #493254 -
Flags: review?(ted.mielczarek) → review+
Comment 47•14 years ago
|
||
Comment on attachment 493255 [details] [diff] [review]
part 6 - Remove duplicate libxpcomglue_s in link flags
The netwerk/test Makefile is kind of goofy since most of those are still SIMPLE_PROGRAMS, but there's one CPP_UNIT_TEST thrown in. Anyway, this patch should be fine. While you're there, would you mind reformatting the blocks you're touching to be two-space indent, with a continuation on the first line? Like:
LIBS = \
$(EXTRA_DSO_LIBS) \
...
($NULL)
Attachment #493255 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 48•14 years ago
|
||
(In reply to comment #46)
> Comment on attachment 493252 [details] [diff] [review]
> part 3 - Fix AR related variables in configure.in
>
> Why are you unsetting AR_EXTRACT here? Is it just because we don't intend to
> build real static libs anymore?
No, but because it's always set to "$(AR) -x", and that value (obviously) doesn't work on the platforms where I unset it. The AR_EXTRACT value is used in expandlibs.py in part 7, so that's why this clean up is required.
Assignee | ||
Comment 49•14 years ago
|
||
(In reply to comment #48)
> (In reply to comment #46)
> > Comment on attachment 493252 [details] [diff] [review] [details]
> > part 3 - Fix AR related variables in configure.in
> >
> > Why are you unsetting AR_EXTRACT here? Is it just because we don't intend to
> > build real static libs anymore?
>
> No, but because it's always set to "$(AR) -x", and that value (obviously)
> doesn't work on the platforms where I unset it. The AR_EXTRACT value is used in
> expandlibs.py in part 7, so that's why this clean up is required.
Oh, and for those where there was an actual value ("$(AR) -extract"), afaik, this doesn't work, as lib.exe doesn't work this way: it can only extract one file from an archive, and you need to give the filename.
Comment 50•14 years ago
|
||
Comment on attachment 493252 [details] [diff] [review]
part 3 - Fix AR related variables in configure.in
Ok, thanks for the explanation.
Attachment #493252 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 51•14 years ago
|
||
(In reply to comment #47)
> Comment on attachment 493255 [details] [diff] [review]
> part 6 - Remove duplicate libxpcomglue_s in link flags
>
> The netwerk/test Makefile is kind of goofy since most of those are still
> SIMPLE_PROGRAMS, but there's one CPP_UNIT_TEST thrown in. Anyway, this patch
> should be fine. While you're there, would you mind reformatting the blocks
> you're touching to be two-space indent, with a continuation on the first line?
> Like:
> LIBS = \
> $(EXTRA_DSO_LIBS) \
> ...
> ($NULL)
I guess you mean only for storage/test/Makefile.in, since other blocks in network/test/Makefile.in are not indented like that. I can do that when landing.
Comment 52•14 years ago
|
||
No, fix the ones you're touching in netwerk/test as well. Consistency is nice, but it's not worth mass-changing them. If you're already touching them, though, it's fine to reformat them.
Assignee | ||
Comment 53•14 years ago
|
||
Same as part 2, but for comm-central. See rationale in comment 22.
Attachment #494425 -
Flags: review?(bugzilla)
Comment 54•14 years ago
|
||
Comment on attachment 494425 [details] [diff] [review]
Remove useless make dependency in */app/Makefile.in - checked in
stealing review from mark. This is good. (Land whenever)
Attachment #494425 -
Flags: review?(bugzilla) → review+
Comment 57•14 years ago
|
||
Comment on attachment 494425 [details] [diff] [review]
Remove useless make dependency in */app/Makefile.in - checked in
I've pushed this to comm-central as it doesn't affect anything and gets it out the way for when the rest is ready to land:
http://hg.mozilla.org/comm-central/rev/bf5708130178
Attachment #494425 -
Attachment description: Remove useless make dependency in */app/Makefile.in → Remove useless make dependency in */app/Makefile.in - checked in
Comment 58•14 years ago
|
||
Comment on attachment 493256 [details] [diff] [review]
part 7 - Replace fakelibs with a more sophisticated library expansion system
>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
>+EXPAND_LIBS = $(PYTHON) $(call core_abspath,$(topsrcdir)/config/expandlibs.py) -c $(DEPTH)/config/autoconf.mk
Do you really need that core_abspath? $(topsrcdir) should be a usable path.
I'm also not wild about using autoconf.mk directly. I think you'd avoid a lot of complexity by just generating your own separate config file, or exporting the variables you need so you can use them as environment variables.
>diff --git a/config/expandlibs.py b/config/expandlibs.py
>new file mode 100644
>--- /dev/null
>+++ b/config/expandlibs.py
A few high-level comments on this file:
* This script seems to have a lot of functions, but they're controlled by options, so it's a little confusing. Maybe you could have a non-option argument to determine the function of the script? Maybe separate top-level scripts that import a common module?
* Please add a usage comment up top somewhere with a brief description of what the script does and the options and what they do.
* Please add docstring comments to each function with a brief description of what the function does.
>diff --git a/config/rules.mk b/config/rules.mk
>--- a/config/rules.mk
>+++ b/config/rules.mk
> $(PROGRAM): $(PROGOBJS) $(LIBS_DEPS) $(EXTRA_DEPS) $(EXE_DEF_FILE) $(RESFILE) $(GLOBAL_DEPS)
> @rm -f $@.manifest
> ifeq (WINCE,$(OS_ARCH))
>- $(LD) -NOLOGO -OUT:$@ $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(PROGOBJS) $(RESFILE) $(call EXPAND_FAKELIBS,$(LIBS) $(EXTRA_LIBS) $(OS_LIBS))
>+ $(EXPAND_LIBS) --exec --uselist -- $(LD) -NOLOGO -OUT:$@ $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(PROGOBJS) $(RESFILE) $(LIBS) $(EXTRA_LIBS) $(OS_LIBS)
I think that "$(EXPAND_LIBS) --exec --uselist --" deserves its own make variable to hide the details. Perhaps EXEC_WITH_EXPANSION or something?
>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -8094,16 +8094,48 @@ if test -n "$MIPSPRO_CXX" -o -n "$COMPAQ
> AC_DEFINE(CPP_THROW_NEW, [])
> else
> AC_DEFINE(CPP_THROW_NEW, [throw()])
> fi
> AC_LANG_C
>
> dnl ========================================================
> dnl =
>+dnl = Check what kind of list files are supported by the
>+dnl = linker
>+dnl =
>+dnl ========================================================
>+
>+AC_CACHE_CHECK(what kind of list files are supported by the linker,
>+ EXPAND_LIBS_LIST_STYLE,
Are you actually using this variable anywhere? It doesn't show up anywhere useful in this patch. If you are, is this something we really need to test for? We don't support that many compilers, I don't think.
r- for a few things, but I really like the cleanup this patch does, and this is a much nicer approach to the whole concept.
Attachment #493256 -
Flags: review?(ted.mielczarek) → review-
Assignee | ||
Comment 59•14 years ago
|
||
(In reply to comment #58)
> Comment on attachment 493256 [details] [diff] [review]
> part 7 - Replace fakelibs with a more sophisticated library expansion system
>
> >diff --git a/config/config.mk b/config/config.mk
> >--- a/config/config.mk
> >+++ b/config/config.mk
> >+EXPAND_LIBS = $(PYTHON) $(call core_abspath,$(topsrcdir)/config/expandlibs.py) -c $(DEPTH)/config/autoconf.mk
>
> Do you really need that core_abspath? $(topsrcdir) should be a usable path.
Absolutely true. I don't even remember how it got there.
> I'm also not wild about using autoconf.mk directly. I think you'd avoid a lot
> of complexity by just generating your own separate config file, or exporting
> the variables you need so you can use them as environment variables.
Do you mean adding export before their definition in autoconf.mk ? That could work.
> >diff --git a/config/expandlibs.py b/config/expandlibs.py
> >new file mode 100644
> >--- /dev/null
> >+++ b/config/expandlibs.py
>
> A few high-level comments on this file:
> * This script seems to have a lot of functions, but they're controlled by
> options, so it's a little confusing. Maybe you could have a non-option argument
> to determine the function of the script? Maybe separate top-level scripts that
> import a common module?
It actually has only 2 functions. So would you prefer, say, expandlibs-gen.py and expandlibs-exec.py scripts?
> * Please add a usage comment up top somewhere with a brief description of what
> the script does and the options and what they do.
Don't the OptionParser setup already fulfil at least half of that?
> * Please add docstring comments to each function with a brief description of
> what the function does.
Will do
> I think that "$(EXPAND_LIBS) --exec --uselist --" deserves its own make
> variable to hide the details. Perhaps EXEC_WITH_EXPANSION or something?
Another option could be to modify LD, CC, CCC, MKSHLIBS, AR, etc.
Actually, it might be better to just do that, so that people are not expected to know how expandlibs works when adding new compiler/linker/whatever commands.
> >+AC_CACHE_CHECK(what kind of list files are supported by the linker,
> >+ EXPAND_LIBS_LIST_STYLE,
>
> Are you actually using this variable anywhere? It doesn't show up anywhere
> useful in this patch. If you are, is this something we really need to test for?
> We don't support that many compilers, I don't think.
It's actually used in expandlibs.py, as config.expand_libs_list_style. It's not entirely necessary at the moment, but it's too dangerous IMHO not to use it. Let me explain.
The reason we need to use list files in the first place is that on the buildbots, the linux kernel version is old enough that the command line arguments length is limited to 128K, and with a huge list of object files, like when linking libxul.so, that just overflows.
There are two ways to provide object lists to gcc:
- Using list files, such as the ones used by MSVC (so, a plain list of file paths, included on the command line with @listfile).
- Using some kind of linker script, which is then passed to ld (that one contains "INPUT(file)" lines and is included on the command line with its file name without @ or any other fancy character).
The first one is what works mostly everywhere.
Unfortunately, the gcc version used on buildbots, combined to the linux kernel limitation explained above, makes it almost impossible to use this technique: the gcc version used on buildbots expands the @listfile argument when calling its internal collect2 command, such that the command line is too long, and the command fails. Newer kernels don't have this limitation (they have a limitation, but it's much higher), and newer gcc versions (though i don't remember when this was fixed) don't have this problem either because they pass @listfile to collect2.
So that only leaves the second option as a working solution.
There is another option, but it's too risky: limiting the command line length. By using relative paths instead of absolute, the command line when linking libxul.so could be reduced quite significantly, but it's really too close to 128K to be reliable: adding a few more object files will make the problem appear again.
Assignee | ||
Comment 60•14 years ago
|
||
With the way expandlibs works, doing so leads to statically linking libmozjs in, without stripping it as dead code (which is what currently happens)
Attachment #501643 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 61•14 years ago
|
||
This is the same as part 8, but for applications in comm-central. This can be safely applied now (except if I screw up the patch ;) but it doesn't require anything else from this bug).
Attachment #501649 -
Flags: review?(bugspam.Callek)
Comment 62•14 years ago
|
||
(In reply to comment #59)
> Do you mean adding export before their definition in autoconf.mk ? That could
> work.
Yeah, that should be fine.
> It actually has only 2 functions. So would you prefer, say, expandlibs-gen.py
> and expandlibs-exec.py scripts?
That might be cleaner, honestly.
> Don't the OptionParser setup already fulfil at least half of that?
Sort of. It is nice to be able to skim the top of a file and get a sense for what it does. I'd settle for that, and leave --help to provide option descriptions.
> > I think that "$(EXPAND_LIBS) --exec --uselist --" deserves its own make
> > variable to hide the details. Perhaps EXEC_WITH_EXPANSION or something?
>
> Another option could be to modify LD, CC, CCC, MKSHLIBS, AR, etc.
> Actually, it might be better to just do that, so that people are not expected
> to know how expandlibs works when adding new compiler/linker/whatever commands.
Either way is fine with me, as long as we can hide the complexity somewhat.
> It's actually used in expandlibs.py, as config.expand_libs_list_style. It's not
> entirely necessary at the moment, but it's too dangerous IMHO not to use it.
> Let me explain.
Ok, sounds sane (if unfortunate).
Updated•14 years ago
|
Attachment #501643 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 63•14 years ago
|
||
Unbitrot. Carrying over r+ from attachment 483985 [details] [diff] [review]
Attachment #493253 -
Attachment is obsolete: true
Attachment #503424 -
Flags: review+
Comment 64•14 years ago
|
||
Comment on attachment 501649 [details] [diff] [review]
Same as part8, for c-c
LGTM, but Standard8 should get power of veto on the mail/ portion.
Attachment #501649 -
Flags: review?(bugspam.Callek)
Attachment #501649 -
Flags: review+
Attachment #501649 -
Flags: feedback?(bugzilla)
Assignee | ||
Comment 65•14 years ago
|
||
(In reply to comment #64)
> Comment on attachment 501649 [details] [diff] [review]
> Same as part8, for c-c
>
> LGTM, but Standard8 should get power of veto on the mail/ portion.
I found out that app code in browser and xulrunner was actually actively using NSPR, so that one doesn't need to be moved (it actually leads to a failure to build on mac). I guess c-c apps also do so, so I'll update the patches.
Assignee | ||
Comment 66•14 years ago
|
||
(In reply to comment #65)
> (In reply to comment #64)
> > Comment on attachment 501649 [details] [diff] [review]
> > Same as part8, for c-c
> >
> > LGTM, but Standard8 should get power of veto on the mail/ portion.
>
> I found out that app code in browser and xulrunner was actually actively using
> NSPR, so that one doesn't need to be moved (it actually leads to a failure to
> build on mac). I guess c-c apps also do so, so I'll update the patches.
I looks like the app code in suite, mailnews and calendar don't use NSPR, so this version of the patch should work. I did try to push to try-c-c, but all I got was build failures occurring before the configure script runs, so I can't know for sure this is okay.
Assignee | ||
Comment 67•14 years ago
|
||
Attachment #504432 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 68•14 years ago
|
||
As said in comment 65, it turns out xulrunner and browser code make use of NSPR functions from their code, so they need to link against NSPR. The previous patch worked on linux because NSPR symbols were resolved at link time through libxul, but this didn't work on mac.
Attachment #501643 -
Attachment is obsolete: true
Attachment #504433 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 69•14 years ago
|
||
Addressed review comments, and made some small adjustments, as well.
For the record, exporting the variables from autoconf.mk ended up not being possible because the values for some _SUFFIX variables overrode the ones set in NSS, which are different, breaking the build on windows. In the end, I made the main expandlibs.py file a generated file from configure.
Attachment #493256 -
Attachment is obsolete: true
Attachment #504434 -
Flags: review?(ted.mielczarek)
Comment 70•14 years ago
|
||
(In reply to comment #66)
> (In reply to comment #65)
> > (In reply to comment #64)
> > > Comment on attachment 501649 [details] [diff] [review]
> > > Same as part8, for c-c
> > >
> > > LGTM, but Standard8 should get power of veto on the mail/ portion.
> >
> > I found out that app code in browser and xulrunner was actually actively using
> > NSPR, so that one doesn't need to be moved (it actually leads to a failure to
> > build on mac). I guess c-c apps also do so, so I'll update the patches.
>
> I looks like the app code in suite, mailnews and calendar don't use NSPR, so
> this version of the patch should work.
Well, I thought they did, but maybe not.
> I did try to push to try-c-c, but all I
> got was build failures occurring before the configure script runs, so I can't
> know for sure this is okay.
That's not quite true. The suite builds all failed due to bug 626310 which has just been landed (not cycled yet). The calendar (sunbird) builds failed on the configure stage as they are no longer supported.
The Thunderbird builds had a mixed reaction - Linux64 & Mac32/64 passed. Linux 32 failed due to a builder error. Windows failed with this:
http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1295253873.1295261149.8044.gz
xpcomglue_s.lib(nsStringAPI.obj) : error LNK2019: unresolved external symbol __imp__PR_sscanf referenced in function "public: int __thiscall nsACString::ToInteger(unsigned int *,unsigned int)const " (?ToInteger@nsACString@@QBEHPAII@Z)
Assignee | ||
Comment 71•14 years ago
|
||
(In reply to comment #70)
> xpcomglue_s.lib(nsStringAPI.obj) : error LNK2019: unresolved external symbol
> __imp__PR_sscanf referenced in function "public: int __thiscall
> nsACString::ToInteger(unsigned int *,unsigned int)const "
> (?ToInteger@nsACString@@QBEHPAII@Z)
Oh, that's a case of the xpcom glue using NSPR... so while app doesn't use it directly, the xpcom glue does. I'll update the patch, then.
Assignee | ||
Comment 72•14 years ago
|
||
Attachment #504440 -
Flags: review?(bugzilla)
Comment 73•14 years ago
|
||
Comment on attachment 501649 [details] [diff] [review]
Same as part8, for c-c
I believe this is the obsolete version.
Attachment #501649 -
Attachment is obsolete: true
Attachment #501649 -
Flags: feedback?(bugzilla)
Updated•14 years ago
|
Attachment #504440 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 74•14 years ago
|
||
(In reply to comment #73)
> Comment on attachment 501649 [details] [diff] [review]
> Same as part8, for c-c
>
> I believe this is the obsolete version.
It was. Sorry for the confusion. Thanks.
You should be able to push this change to c-c without waiting for the other patches to reach m-c.
Comment 75•14 years ago
|
||
Comment on attachment 504440 [details] [diff] [review]
[checked in] Same as part8, for c-c ; v2
Checked into comm-central: http://hg.mozilla.org/comm-central/rev/44bfbaecd266
Attachment #504440 -
Attachment description: Same as part8, for c-c ; v2 → [checked in] Same as part8, for c-c ; v2
Updated•14 years ago
|
Attachment #504432 -
Flags: review?(ted.mielczarek) → review+
Comment 76•14 years ago
|
||
Comment on attachment 504433 [details] [diff] [review]
part 8 - Applications don't need to link against libraries that are either part of libxul or only used by libxul
># HG changeset patch
># Parent 937afe0069ad1f03ee20adabe1ecc66920cbfaca
>Bug 584474 part 8 - Applications don't need to link against libraries that are either part of libxul or only used by libxul
>
>diff --git a/browser/app/Makefile.in b/browser/app/Makefile.in
>--- a/browser/app/Makefile.in
>+++ b/browser/app/Makefile.in
>@@ -119,26 +119,31 @@ APP_XPCOM_LIBS = $(XPCOM_GLUE_LDOPTS)
> else
> MOZILLA_INTERNAL_API = 1
> APP_XPCOM_LIBS = $(XPCOM_LIBS)
> endif
>
> LIBS += \
> $(STATIC_COMPONENTS_LINKER_PATH) \
> $(EXTRA_DSO_LIBS) \
>- $(MOZ_JS_LIBS) \
> $(APP_XPCOM_LIBS) \
> $(NSPR_LIBS) \
>+ $(NULL)
>+
>+ifdef BUILD_STATIC_LIBS
>+LIBS += \
>+ $(MOZ_JS_LIBS) \
> $(TK_LIBS) \
> $(NULL)
Can you change the formatting to be two-space indent while you're here?
Attachment #504433 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 77•14 years ago
|
||
(In reply to comment #76)
> Can you change the formatting to be two-space indent while you're here?
Just on that part or the whole file?
Comment 78•14 years ago
|
||
Just in the sections you're touching is fine.
Comment 79•14 years ago
|
||
Attachment 504434 [details] [diff] no longer applies cleanly on trunk. This patch does. I didn't really change anything, but here's the result of qrefresh on this patch in case you're interested: <http://hg.mozilla.org/users/ehsan.akhgari_gmail.com/mq/rev/1b701da8c094>
Attachment #504434 -
Attachment is obsolete: true
Attachment #509792 -
Flags: review?(ted.mielczarek)
Attachment #504434 -
Flags: review?(ted.mielczarek)
Comment 80•14 years ago
|
||
Comment on attachment 509792 [details] [diff] [review]
part 9 - Replace fakelibs with a more sophisticated library expansion system
># HG changeset patch
># Parent f129cc1d94d9cc0e7fad63609b3561b222c231c2
>Bug 584474 part 9 - Replace fakelibs with a more sophisticated library expansion system
>
>diff --git a/allmakefiles.sh b/allmakefiles.sh
>--- a/allmakefiles.sh
>+++ b/allmakefiles.sh
>@@ -64,16 +64,17 @@ build/pgo/js-input/Makefile
> build/unix/Makefile
> build/win32/Makefile
> build/win32/crashinjectdll/Makefile
> config/Makefile
> config/autoconf.mk
> config/mkdepend/Makefile
> config/nspr/Makefile
> config/doxygen.cfg
>+config/expandlibs.py
I'm not wild about preprocessing entire Python scripts, I'd really prefer just a small config file or something, but I'm not going to block you on that one issue. Although, if you did that, it would make it pretty easy to write some unittests for expandlibs, which would be a nice addition. (See http://mxr.mozilla.org/mozilla-central/source/config/tests/ )
>diff --git a/config/Makefile.in b/config/Makefile.in
>--- a/config/Makefile.in
>+++ b/config/Makefile.in
>@@ -163,17 +163,17 @@ export:: $(STL_WRAPPERS_SENTINEL)
> GARBAGE += $(STL_WRAPPERS_SENTINEL)
> GARBAGE_DIRS += stl_wrappers
> endif
>
> install::
> $(SYSINSTALL) $(IFLAGS1) $(DEPTH)/mozilla-config.h $(DESTDIR)$(includedir)
>
> GARBAGE += \
>- $(FINAL_LINK_COMPS) $(FINAL_LINK_LIBS) $(FINAL_LINK_COMP_NAMES) buildid $(srcdir)/*.pyc
>+ $(FINAL_LINK_COMPS) $(FINAL_LINK_LIBS) $(FINAL_LINK_COMP_NAMES) buildid $(srcdir)/*.pyc *.pyc
Can you swap the tab for two spaces while you're here?
>diff --git a/config/expandlibs-exec.py b/config/expandlibs-exec.py
>new file mode 100644
>--- /dev/null
>+++ b/config/expandlibs-exec.py
>+class ExpandArgsMore(ExpandArgs):
>+ def __init__(self, args):
>+ super(ExpandArgsMore, self).__init__(args)
>+ self.tmp = []
>+
>+ def __del__(self):
>+ '''Automatically remove temporary files that an instance creates'''
>+ for tmp in self.tmp:
>+ if os.path.isdir(tmp):
>+ shutil.rmtree(tmp, True)
>+ else:
>+ os.remove(tmp)
You know, since we require Python 2.5+ on trunk now, you could implement this class as a Context Manager and use it in a with statement:
http://docs.python.org/release/2.5.2/lib/typecontextmanager.html
Then you'd just write your logic in main as:
with ExpandArgsMore(args) as args:
...
and the cleanup would happen in the __exit__. I guess it's not a big deal either way, just nice to have it scoped in a cleaner fashion.
>+ def makelist(self):
>+ '''Replaces object file names with a temporary list file, using a
>+ list format depending on the EXPAND_LIBS_LIST_STYLE variable
>+ '''
>+ objs = filter(lambda item: os.path.splitext(item)[1] == OBJ_SUFFIX, self)
>+ if not len(objs): return
>+ fd, tmp = tempfile.mkstemp(suffix=".list",dir=os.curdir)
>+ if EXPAND_LIBS_LIST_STYLE == "linkerscript":
>+ content = map(lambda f: "INPUT(" + f + ")\n", objs)
>+ ref = tmp
>+ elif EXPAND_LIBS_LIST_STYLE == "list":
>+ content = map(lambda f: f + "\n", objs)
>+ ref = "@" + tmp
These would read more cleanly as string joins/list comprehensions (or generator expressions), I think, maybe:
content = "\n".join("INPUT(%s)" % o for o in objs)
and
content = "\n".join(objs)
>+ else:
>+ os.remove(tmp)
>+ return
>+ self.tmp.append(tmp)
>+ f = os.fdopen(fd, "w")
>+ f.writelines(content)
>+ f.close()
>+ idx = self.index(objs[0])
>+ newlist = self[0:idx] + [ref] + filter(lambda item: item not in objs, self[idx:])
You can write the filter as [item for item in self[idx:] if item not in objs].
>diff --git a/config/expandlibs.py.in b/config/expandlibs.py.in
>new file mode 100644
>--- /dev/null
>+++ b/config/expandlibs.py.in
>+class LibDescriptor(dict):
>+ KEYS = ['OBJS', 'LIBS']
>+
>+ def __init__(self, filename=None):
>+ '''Creates an instance of a lib descriptor, read from filename when
>+ given'''
>+ super(LibDescriptor, self).__init__()
>+ for key in self.KEYS:
>+ self[key] = []
>+ if not filename: return
>+ f = open(filename, 'r')
with open(filename, 'r') as f:
>+ for line in f:
>+ pair = line.split('=', 2)
>+ if len(pair) > 1:
>+ (key, value) = map(lambda s: s.strip(), pair)
I think using map on a 2-item list is a bit excessive. Just write it as key, value = pair[0].strip(), pair[1].strip() ?
>+ def __str__(self):
>+ '''Serializes the lib descriptor'''
>+ return '\n'.join(map(lambda k: '%s = %s' % (k, ' '.join(self[k])),
>+ filter(lambda k: k in self and len(self[k]), self.KEYS)))
There's probably a nicer way to write this with a generator expression, but I am running out of steam in this review. In general, it's more Pythonic to use list comprehensions or generator expressions instead of map. Plus, you can do a map/filter all in one by tacking on the "if" clause.
>+class ExpandArgs(list):
>+ def __init__(self, args):
>+ '''Creates a clone of the |args| list and performs file expansion on
>+ each item it contains'''
>+ super(ExpandArgs, self).__init__()
>+ for arg in args:
>+ self += self._expand(arg)
I think you could just write this as:
super(ExpandArgs, self).__init__(self._expand(arg) for arg in args)
right? Assuming you change _expand to return a string and not a single-element list. Although I guess that breaks _expand_desc. Hm. I guess you could write it like:
super(ExpandArgs, self).__init__(a for arg in args for a in self._expand(arg))
although the doubly-nested generator expression probably winds up less readable, so I dunno then.
>diff --git a/config/rules.mk b/config/rules.mk
>--- a/config/rules.mk
>+++ b/config/rules.mk
> ifndef LIBRARY
> ifdef STATIC_LIBRARY_NAME
>+_LIBRARY := $(LIB_PREFIX)$(STATIC_LIBRARY_NAME).$(LIB_SUFFIX)
>+# Only build actual library if it is installed in DIST/lib or SDK
>+ifeq (,$(SDK_LIBRARY)$(DIST_INSTALL)$(NO_EXPAND_LIBS))
>+LIBRARY := $(_LIBRARY).$(LIBS_DESC_SUFFIX)
>+else
>+LIBRARY := $(_LIBRARY) $(_LIBRARY).$(LIBS_DESC_SUFFIX)
>+endif
> endif # STATIC_LIBRARY_NAME
> endif # LIBRARY
It feels a little weird to have $(LIBRARY) wind up containing both the real and fake static library.
r=me since this review is 99% style nits.
Attachment #509792 -
Flags: review?(ted.mielczarek) → review+
Comment 81•14 years ago
|
||
I have imported the patches in this bug in my mq for the past few days in order to be able to build libxul on Mac faster. Today, when I pulled from mozilla-central and rebuilt, I got this build error when linking libxul:
http://pastebin.mozilla.org/1052351
Seems like something has changed, which would make landing these patches in its current form could break libxul builds on mozilla-central.
Assignee | ||
Comment 82•14 years ago
|
||
Did you clobber?
Comment 83•14 years ago
|
||
(In reply to comment #82)
> Did you clobber?
No, I just disabled libxul again for now. But I did send my mq patches to the try server, and both debug and opt builds were successful, which may suggest that clobbering could have fixed the problem. So, I think you can ignore comment 82 for now. Sorry for the noise.
Assignee | ||
Comment 84•14 years ago
|
||
This addresses review from comment 80, and adds a testcase. Not entirely sure the testcase is very readable.
Attachment #514763 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 85•14 years ago
|
||
MOZ_JS_STATIC_LIBS is set to a proper value earlier in configure.in so no need to override later. I'm also removing a -ljs_static flag that was added to CPP_UNIT_TESTS for whatever reason (really, no idea, everything builds fine without, and that wasn't in the reviewed patch it went with ; see bug 625962)
Attachment #515013 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 86•14 years ago
|
||
(In reply to comment #85)
> Created attachment 515013 [details] [diff] [review]
> Another additional patch for part 9
>
> MOZ_JS_STATIC_LIBS is set to a proper value earlier in configure.in so no need
> to override later. I'm also removing a -ljs_static flag that was added to
> CPP_UNIT_TESTS for whatever reason (really, no idea, everything builds fine
> without, and that wasn't in the reviewed patch it went with ; see bug 625962)
Note that the latter breaks bug 551138
Comment 87•14 years ago
|
||
Comment on attachment 514763 [details] [diff] [review]
Additional patch for part 9, addressing review comments
>diff --git a/config/expandlibs.py.in b/config/expandlibs.py
>rename from config/expandlibs.py.in
>rename to config/expandlibs.py
>--- a/config/expandlibs.py.in
>+++ b/config/expandlibs.py
>+ if not content: return
Put the return on a new line.
> def __str__(self):
> '''Serializes the lib descriptor'''
>- return '\n'.join(map(lambda k: '%s = %s' % (k, ' '.join(self[k])),
>- filter(lambda k: k in self and len(self[k]), self.KEYS)))
>+ return '\n'.join(['%s = %s' % (k, ' '.join(self[k])) for k in self.KEYS if len(self[k])])
You don't need the [] here, it's just a generator expression if you leave them off. (You don't need the actual list, just something to pass to join.)
I just skimmed the unit tests, as long as they pass and you think they're testing something useful that's good enough for me. It's nice to have a place to stick tests if we have to fix bugs in this in the future, or add new features.
Attachment #514763 -
Flags: review?(ted.mielczarek) → review+
Updated•14 years ago
|
Attachment #515013 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 88•14 years ago
|
||
http://hg.mozilla.org/projects/build-system/rev/5a2868b3e63b
http://hg.mozilla.org/projects/build-system/rev/bcb252531278
http://hg.mozilla.org/projects/build-system/rev/d541802d1931
http://hg.mozilla.org/projects/build-system/rev/97aacaa069c8
http://hg.mozilla.org/projects/build-system/rev/22570be102af
http://hg.mozilla.org/projects/build-system/rev/5de65a480c6d
http://hg.mozilla.org/projects/build-system/rev/f59a24b5903b
http://hg.mozilla.org/projects/build-system/rev/528d6def0f97
http://hg.mozilla.org/projects/build-system/rev/143c8e9082af
Attachment #509792 -
Attachment is obsolete: true
Attachment #514763 -
Attachment is obsolete: true
Attachment #515013 -
Attachment is obsolete: true
Attachment #515055 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: fixed-in-bs
Reporter | ||
Comment 89•14 years ago
|
||
I think Maemo is broken with those patches.
Assignee | ||
Comment 90•14 years ago
|
||
The weird thing is the maemo gtk build had succeeded earlier, and the QT one succeeded but went purple... I don't know where the success gtk build went :(
Assignee | ||
Comment 91•14 years ago
|
||
Ah I hadn't seen it was a new commit. You need to clobber.
Reporter | ||
Comment 92•14 years ago
|
||
The builds claim they are clobbers.
Comment 93•14 years ago
|
||
Is it expected that this patch dumps a list of all the .o files being linked into libxul to stderr while building?
Assignee | ||
Comment 94•14 years ago
|
||
Yes it is, though at some point, we may want to make it less verbose.
Comment 95•14 years ago
|
||
That was my point, actually. Right now it's verbose enough to get in the way of things I was trying to do...
Assignee | ||
Comment 96•14 years ago
|
||
Attachment #516893 -
Flags: review?(ted.mielczarek)
Updated•14 years ago
|
Attachment #516893 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 97•14 years ago
|
||
Landed part 10.
http://hg.mozilla.org/projects/build-system/rev/cb0f1dcf6fae
Comment 98•14 years ago
|
||
I'm not sure if this can also be fixed in this bug, but I thought I would also anyway.
Let's say you change something in layout forms. Currently you have to use this command to rebuild:
make -C layout/forms && make -C layout/build && make -C toolkit/library
The layout/build part is necessary even if your change doesn't have any effect on the source files in layout/build. And without that, make -C toolkit/library doesn't rebuild libxul, as if none of the input files have changed.
The critical thing happening in layout/build seems to be this:
make libs
/opt/local/bin/python2.7 /Users/ehsanakhgari/moz/mozilla-central/config/pythonpath.py -I../../config /Users/ehsanakhgari/moz/mozilla-central/config/expandlibs-gen.py nsLayoutModule.o nsContentDLF.o nsLayoutStatics.o ../base/libgkbase_s.a ../forms/libgkforms_s.a ../generic/libgkgeneric_s.a ../ipc/libgkipc_s.a ../style/libgkstyle_s.a ../tables/libgktable_s.a ../xul/base/src/libgkxulbase_s.a ../../content/base/src/libgkconbase_s.a ../../content/canvas/src/libgkconcvs_s.a ../../content/events/src/libgkconevents_s.a ../../content/html/content/src/libgkconhtmlcon_s.a ../../content/html/document/src/libgkconhtmldoc_s.a ../../content/xml/content/src/libgkconxmlcon_s.a ../../content/xml/document/src/libgkconxmldoc_s.a ../../content/xslt/src/base/libtxbase_s.a ../../content/xslt/src/xml/libtxxml_s.a ../../content/xslt/src/xpath/libtxxpath_s.a ../../content/xslt/src/xslt/libtxxslt_s.a ../../content/xbl/src/libgkconxbl_s.a ../../content/xul/document/src/libgkconxuldoc_s.a ../../view/src/libgkview_s.a ../../dom/base/libjsdombase_s.a ../../dom/src/events/libjsdomevents_s.a ../../dom/src/json/libjson_s.a ../../dom/src/jsurl/libjsurl_s.a ../../dom/src/storage/libjsdomstorage_s.a ../../dom/src/offline/libjsdomoffline_s.a ../../dom/src/geolocation/libjsdomgeolocation_s.a ../../dom/src/notification/libjsdomnotification_s.a ../../dom/system/libdomsystem_s.a ../../dom/src/threads/libdomthreads_s.a ../../dom/indexedDB/libdom_indexeddb_s.a ../../editor/libeditor/text/libtexteditor_s.a ../../editor/libeditor/base/libeditorbase_s.a ../../parser/html/libhtml5p_s.a ../../caps/src/libcaps_s.a ../../dom/system/cocoa/libdomsystemcocoa_s.a ../../media/libvorbis/lib/libvorbis.a ../../media/libogg/src/libogg.a ../../content/media/libgkconmedia_s.a ../../media/libtheora/lib/libtheora.a ../../content/media/ogg/libgkconogg_s.a ../../content/media/webm/libgkconwebm_s.a ../../media/libnestegg/src/libnestegg.a ../../media/libvpx/libvpx.a ../../content/media/wave/libgkconwave_s.a ../../media/libsydneyaudio/src/libsydneyaudio.a ../printing/libgkprinting_s.a ../xul/base/src/tree/src/libgkxultree_s.a ../xul/base/src/grid/libgkxulgrid_s.a ../../content/xul/content/src/libgkconxulcon_s.a ../../content/xul/templates/src/libgkconxultmpl_s.a ../inspector/src/libinspector_s.a ../mathml/libgkmathml_s.a ../../content/mathml/content/src/libgkcontentmathml_s.a ../../content/xtf/src/libgkcontentxtf_s.a ../svg/base/src/libgksvgbase_s.a ../../content/svg/document/src/libgkconsvgdoc_s.a ../../content/svg/content/src/libgkcontentsvg_s.a ../../content/smil/libgkconsmil_s.a ../../editor/libeditor/html/libhtmleditor_s.a ../../editor/txtsvc/src/libtxtsvc_s.a ../../js/src/xpconnect/src/libxpconnect_s.a > libgklayout.a.desc
My knowledge of the build system is very limited, but it seems to me that the build system looks at the lastmod timestamp of these *.desc files (such as libgklayout.a.desc), and if one of them has changed, it attempts to open them and use them.
This is sort of a big pain, since it adds an extra build step for people who work on Gecko stuff (to be more precise, stuff which used to be linked into libgklayout).
Is it possible to modify the build system to not look at the timestamp for these files, and instead only look at the timestamp of the object files listed inside them?
Assignee | ||
Comment 99•14 years ago
|
||
(In reply to comment #98)
> Is it possible to modify the build system to not look at the timestamp for
> these files, and instead only look at the timestamp of the object files listed
> inside them?
It is possible, but the downside is that it means executing a python process in each and every directory you enter, even when running rules such as clean or export. On Windows, this could affect build time badly. I could do a test on try to see how much of a bad effect it has, but I'm not sure the build times on try are stable enough to allow that.
The first implementation actually was expanding the dependencies, but I thought it wasn't strictly necessary: the .desc files shouldn't be updated unless a file it points to is modified, anyways. Except that I now realize that this doesn't work for indirect dependencies.
BUT, provided you don't add object files or libraries, the following should just work for you:
make -C layout/forms && make -B -C toolkit/library
(add the libxul filename to the latter if you don't want to rebuild the 4 object files in toolkit/library)
That's actually one step less :)
Comment 100•14 years ago
|
||
We could probably cheat and generate a dependency file that's includable as a Makefile (like we do with compiler-generated deps), specifying a dependency for a .desc file on all the object files it lists, but I don't know if that's worth it.
Reporter | ||
Comment 101•14 years ago
|
||
If we can pull off non-recursive make relatively soon that will give us that for free; if not, I think we should come back and do that.
Assignee | ||
Comment 103•14 years ago
|
||
Justin reverted a part of a patch
http://hg.mozilla.org/try/rev/6aeab14f8db5
But I don't see any place where these variables are used...
Reporter | ||
Comment 104•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5a2868b3e63b
http://hg.mozilla.org/mozilla-central/rev/bcb252531278
http://hg.mozilla.org/mozilla-central/rev/d541802d1931
http://hg.mozilla.org/mozilla-central/rev/97aacaa069c8
http://hg.mozilla.org/mozilla-central/rev/22570be102af
http://hg.mozilla.org/mozilla-central/rev/5de65a480c6d
http://hg.mozilla.org/mozilla-central/rev/f59a24b5903b
http://hg.mozilla.org/mozilla-central/rev/528d6def0f97
http://hg.mozilla.org/mozilla-central/rev/143c8e9082af
http://hg.mozilla.org/mozilla-central/rev/cb0f1dcf6fae
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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
•