See bug 584474 comment 99 and bug 584474 comment 100.

Comment on attachment 522323 [details] [diff] [review] Patch Actually, this has too much of a downside. I need to think a little bit more about it.

This version should ensure the list of dependencies is always up-to-date

Comment on attachment 522349 [details] [diff] [review] Patch v2 >diff --git a/config/rules.mk b/config/rules.mk >--- a/config/rules.mk >+++ b/config/rules.mk >@@ -802,21 +802,28 @@ 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 > > # Create dependencies on static (and shared EXTRA_DSO_LIBS) libraries >-DO_EXPAND_LIBS = $(foreach f,$(1),$(if $(filter %.$(LIB_SUFFIX),$(f)),$(if $(wildcard $(f).$(LIBS_DESC_SUFFIX)),$(f).$(LIBS_DESC_SUFFIX),$(if $(wildcard $(f)),$(f))))) >-LIBS_DEPS = $(call DO_EXPAND_LIBS,$(filter %.$(LIB_SUFFIX),$(LIBS))) >-SHARED_LIBRARY_LIBS_DEPS = $(call DO_EXPAND_LIBS,$(SHARED_LIBRARY_LIBS)) >+ifneq (,$(strip $(LIBS) $(SHARED_LIBRARY_LIBS) $(EXTRA_DSO_LDOPTS))) >+$(MDDEPDIR)/libs: Makefile.in >+ @$(EXPAND_LIBS_DEPS) LIBS_DEPS = $(filter %.$(LIB_SUFFIX),$(LIBS)) > $@ >+ @$(EXPAND_LIBS_DEPS) SHARED_LIBRARY_LIBS_DEPS = $(SHARED_LIBRARY_LIBS) >> $@ >+ @$(EXPAND_LIBS_DEPS) DSO_LDOPTS_DEPS = $(EXTRA_DSO_LIBS) $(filter %.$(LIB_SUFFIX), $(EXTRA_DSO_LDOPTS)) >> $@ This is the only bit that bothers me about this patch. It relies on the fact that ExpandArgs only tries to expand things it knows are desc files, so it will pass the variable assignments at the start of each line through verbatim. I'd rather either see a) ExpandLibsDeps changed to expect these variable assignments, and printing them out while passing the rest of the line down to ExpandArgs, or b) just printf'ing the variable names to the file instead of passing them into expandlibs_deps. Everything else looks fine.

As discussed on irc last week.

Comment on attachment 527212 [details] [diff] [review] Patch v3 There are some problems with this patch :-/

Previous patches actually didn't work when starting from a clobber build because dependencies would be filled during the export phase and wouldn't contain lib descriptors or object files. I'm wondering how I tested this initially...

Comment on attachment 527219 [details] [diff] [review] Patch v3.1 Review of attachment 527219 [details] [diff] [review]: r- for the MAKECMDGOALS weirdness, but otherwise it looks fine. Just a few nits, as noted. ::: config/expandlibs_deps.py @@ +53,5 @@ + if os.path.exists(arg + conf.LIBS_DESC_SUFFIX): + objs += [relativize(arg + conf.LIBS_DESC_SUFFIX)] + return objs + +def split_args(args): This could use a brief docstring comment. @@ +55,5 @@ + return objs + +def split_args(args): + result = {} + while len(args): You can just write "while args:". @@ +70,5 @@ + return result + +if __name__ == '__main__': + for key, value in split_args(sys.argv[1:]).iteritems(): + print key, '=', " ".join(ExpandLibsDeps(value)) print "%s = %s" % (key, " ".join(...)) ::: config/rules.mk @@ +811,5 @@ $(error SHARED_LIBRARY_LIBS must contain .$(LIB_SUFFIX) files only) endif # Create dependencies on static (and shared EXTRA_DSO_LIBS) libraries +ifneq (,$(filter libs,$(MAKECMDGOALS))) Ick. I'm not a fan of this, I'd rather have it solved by dependencies.

Addressed review comments. In the end, I was able to do without fiddling with MAKECMDGOAL. This works by setting the dependency file as a dependency of the various lib/program build targets through EXTRA_DEPS, and only include the dependency file if it exists (otherwise, make would create it unconditionally, leading to the same problem the MAKECMDGOAL filter was trying to prevent)

Comment on attachment 529650 [details] [diff] [review] Implement full dependencies for expandlibs Review of attachment 529650 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/rules.mk @@ +810,5 @@ > +ifneq (,$(wildcard $(MDDEPDIR)/libs)) > +include $(MDDEPDIR)/libs > +endif > + > +$(MDDEPDIR)/libs: $(filter %.$(LIBS_DESC_SUFFIX),$(LIBS_DEPS) $(SHARED_LIBRARY_LIBS_DEPS) $(DSO_LDOPTS_DEPS)) Can you just append these deps to the rule above?

(In reply to comment #10) > ::: config/rules.mk > @@ +810,5 @@ > > +ifneq (,$(wildcard $(MDDEPDIR)/libs)) > > +include $(MDDEPDIR)/libs > > +endif > > + > > +$(MDDEPDIR)/libs: $(filter %.$(LIBS_DESC_SUFFIX),$(LIBS_DEPS) $(SHARED_LIBRARY_LIBS_DEPS) $(DSO_LDOPTS_DEPS)) > > Can you just append these deps to the rule above? Nope, because the variables are not defined until the file is included.

Oh, right.

Needed a fixup for make clean to work properly (which made a red on linux opt builds with PGO doing a make clean) http://hg.mozilla.org/mozilla-central/rev/357a308588cd

So this means I can change content/base/src/nsXMLHttpRequest.cpp and build in content/base and then toolkit/library and The Right Thing (TM) will happen, but I still have to build in content/base, right?

(In reply to comment #15) > So this means I can change content/base/src/nsXMLHttpRequest.cpp and build > in content/base and then toolkit/library and The Right Thing (TM) will > happen, but I still have to build in content/base, right? That was already the case. What this however means is that you can now add or remove a file in content/base/src/, build in content/base and then toolkit/library and The Right Thing (TM) will happen.

(In reply to comment #16) > (In reply to comment #15) > > So this means I can change content/base/src/nsXMLHttpRequest.cpp and build > > in content/base and then toolkit/library and The Right Thing (TM) will > > happen, but I still have to build in content/base, right? > > That was already the case. Well, not quite. I used to have to build in layout/build in between ... which this patch removes the need for. I just wanted to be sure how much win we got. This should be blogged about/etc.

Backed out. http://hg.mozilla.org/mozilla-central/rev/60cfdbaf76fb

Landed a hopefully working version on build-system http://hg.mozilla.org/projects/build-system/rev/8b7150ba4450

Backed out, again. http://hg.mozilla.org/projects/build-system/rev/43af93422421 The error doesn't make any sense, though, because this happens shortly before the error: nsinstall -m 444 ./nspr4_s.lib ./nspr4.dll ./nspr4.lib ./nspr4.pdb e:/builds/moz2_slave/bld-system-w32/build/obj-firefox/dist/lib And this is the error: make[5]: Entering directory `/e/builds/moz2_slave/bld-system-w32/build/obj-firefox/js/src' make[5]: *** No rule to make target `..\..\dist\lib\nspr4.lib', needed by `mozjs.dll'. Stop. Or would that be an nth case of / vs \ ?

When I build with this patch on Linux-64, I get make[6]: Entering directory `/home/jlebar/code/moz/ff-inbound/release/memory/mozutils' /bin/sh: cannot create .deps/libs: Directory nonexistent make[6]: *** [.deps/libs] Error 2

(In reply to Justin Lebar [:jlebar] from comment #22) > When I build with this patch on Linux-64, I get > > make[6]: Entering directory > `/home/jlebar/code/moz/ff-inbound/release/memory/mozutils' > /bin/sh: cannot create .deps/libs: Directory nonexistent > make[6]: *** [.deps/libs] Error 2 Try changing: $(MDDEPDIR)/libs: Makefile.in to $(MDDEPDIR)/libs: Makefile.in $(MDDEPDIR) in config/rules.mk

Now I get make[6]: *** No rule to make target `.deps', needed by `.deps/libs'. Stop.

...but it maybe works if I add $(MDDEPDIR): $(CURDIR)/$(MDDEPDIR) under the rule for $(CURDIR)/$(MDDEPDIR).

It's essentially the same patch as the old one, with a few additional bits in expandlibs_deps.py to store / instead of \ on windows.

Backed out since suspected cause of later failures: { make[6]: *** No rule to make target `../../dom/telephony/libdomtelephony_s.a', needed by `libgklayout.a.desc'. Stop. make[5]: *** [libs] Error 2 make[4]: *** [libs_tier_platform] Error 2 make[3]: *** [tier_platform] Error 2 make[2]: *** [default] Error 2 make[1]: *** [realbuild] Error 2 make: *** [build] Error 2 } https://tbpl.mozilla.org/php/getParsedLog.php?id=10528201&tree=Mozilla-Inbound etc https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a329aa4bc026 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2195f743e7dc https://hg.mozilla.org/integration/mozilla-inbound/rev/d151eaf1985c

Meant to add, some of the failures occurred on periodic clobbers (eg https://tbpl.mozilla.org/php/getParsedLog.php?id=10528651&tree=Mozilla-Inbound), so whilst I've retriggered clobbers on the 369ad04efa1f push to give you more info, I don't believe it will fix the issue on all platforms.

Fixup for Windows builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/828112609eb9

And backout because of the subtle breakage in layout/media (and probably many other places doing the same kind of deal): https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea78fe84b7c

The layout/media part is for "No rule to make target `winmm.lib', needed by `gkmedias.dll'" errors. The config/rules.mk part is to avoid e.g. libogg, libtremor, etc. objects to be rebuilt from layout/media/ because the .o dependencies have a further dependency on Makefile.