Closed Bug 584474 Opened 14 years ago Closed 13 years ago

Stop building intermediate static libs with fakelibs

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: khuey, Assigned: glandium)

References

Details

(Whiteboard: fixed-in-bs)

Attachments

(12 files, 19 obsolete files)

809 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
11.47 KB, patch
khuey
: review+
Details | Diff | Splinter Review
3.41 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.04 KB, patch
ted
: review+
Details | Diff | Splinter Review
997 bytes, patch
ted
: review+
Details | Diff | Splinter Review
1.84 KB, patch
Callek
: review+
Details | Diff | Splinter Review
871 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
3.81 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.63 KB, patch
ted
: review+
Details | Diff | Splinter Review
2.30 KB, patch
standard8
: review+
Details | Diff | Splinter Review
107.39 KB, patch
glandium
: review+
Details | Diff | Splinter Review
3.30 KB, 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.
I have the beginnings of a patch for this, but it's tricky since we can't chain response files.
Assignee: nobody → me
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?)
Attached patch First pass (obsolete) — Splinter Review
This builds on Windows.  Needs some cleanup.
Attached patch Slightly cleaner (obsolete) — Splinter Review
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
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.
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.
Attached patch Patch (obsolete) — Splinter Review
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)
Attached patch Patch for m-c to make c-c build (obsolete) — Splinter Review
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)
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 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 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+
(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.
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.
(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.
(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.
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.
(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.
Attached patch Patch (obsolete) — Splinter Review
Nits picked
Attachment #469905 - Attachment is obsolete: true
Attachment #482094 - Flags: review+
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.
Blocks: 603370
Depends on: 605146
Attached patch Add wininet when linking xul.dll (obsolete) — Splinter Review
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.
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)
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)
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)
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.
Depends on: 605153
Like wininet on windows, crypto is needed on osx, debug builds only.
(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.
I should get to these reviews this weekend.
Attachment #483985 - Flags: review?(ted.mielczarek)
Attachment #483994 - Flags: review?(ted.mielczarek)
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)
I can review it soon (for real this time) but idk that we're going to want to take these changes before branch.
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+
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.
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.
(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.
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)
Attachment #483988 - Flags: review?(khuey) → review+
Attachment #483985 - Flags: review?(ted.mielczarek) → review+
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+
(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.
That is...worrisome.
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)
Unbitrotted (small context change). Carrying r+ over.
Attachment #483988 - Attachment is obsolete: true
Attachment #491475 - Flags: review+
Attachment #491474 - Flags: review?(khuey) → review+
OS: Windows 7 → All
Hardware: x86 → All
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
Depends on: 614544
No longer depends on: 614544
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)
Unbitrot (context change only)
Attachment #483985 - Attachment is obsolete: true
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)
Attachment #470128 - Attachment is obsolete: true
Attachment #482094 - Attachment is obsolete: true
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)
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 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?
Attachment #493254 - Flags: review?(ted.mielczarek) → review+
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+
(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.
(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 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+
(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.
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.
Same as part 2, but for comm-central. See rationale in comment 22.
Attachment #494425 - Flags: review?(bugzilla)
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 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 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-
(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.
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)
Attached patch Same as part8, for c-c (obsolete) — Splinter Review
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)
(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).
Attachment #501643 - Flags: review?(ted.mielczarek) → review+
Unbitrot. Carrying over r+ from attachment 483985 [details] [diff] [review]
Attachment #493253 - Attachment is obsolete: true
Attachment #503424 - Flags: review+
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)
(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.
(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.
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)
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)
(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)
(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.
Attachment #504440 - Flags: review?(bugzilla)
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)
Attachment #504440 - Flags: review?(bugzilla) → review+
(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 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
Attachment #504432 - Flags: review?(ted.mielczarek) → review+
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+
(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?
Just in the sections you're touching is fine.
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 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+
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.
Did you clobber?
(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.
This addresses review from comment 80, and adds a testcase. Not entirely sure the testcase is very readable.
Attachment #514763 - Flags: review?(ted.mielczarek)
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)
(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 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+
Attachment #515013 - Flags: review?(ted.mielczarek) → review+
Whiteboard: fixed-in-bs
I think Maemo is broken with those patches.
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 :(
Ah I hadn't seen it was a new commit. You need to clobber.
The builds claim they are clobbers.
Is it expected that this patch dumps a list of all the .o files being linked into libxul to stderr while building?
Yes it is, though at some point, we may want to make it less verbose.
That was my point, actually.  Right now it's verbose enough to get in the way of things I was trying to do...
Attachment #516893 - Flags: review?(ted.mielczarek) → review+
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?
(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 :)
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.
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.
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...
Depends on: 644081
Depends on: 644608
Depends on: 648134
Depends on: 658251
Depends on: 680848
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.