Last Comment Bug 644608 - Implement full dependencies for expandlibs
: Implement full dependencies for expandlibs
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Mike Hommey [:glandium]
:
: Gregory Szorc [:gps]
Mentors:
: 688999 (view as bug list)
Depends on: 741287 748001
Blocks: 584474
  Show dependency treegraph
 
Reported: 2011-03-24 08:28 PDT by Mike Hommey [:glandium]
Modified: 2012-04-23 11:03 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (5.09 KB, patch)
2011-03-28 02:29 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Patch v2 (13.24 KB, patch)
2011-03-28 06:43 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Patch v3 (14.92 KB, patch)
2011-04-20 01:21 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Patch v3.1 (15.10 KB, patch)
2011-04-20 02:31 PDT, Mike Hommey [:glandium]
ted: review-
Details | Diff | Splinter Review
Implement full dependencies for expandlibs (15.70 KB, patch)
2011-05-03 01:18 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Implement full dependencies for expandlibs. (16.24 KB, patch)
2012-03-14 10:23 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Fixups (8.33 KB, patch)
2012-04-06 07:59 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2011-03-24 08:28:18 PDT
See bug 584474 comment 99 and bug 584474 comment 100.
Comment 1 Mike Hommey [:glandium] 2011-03-28 02:29:42 PDT
Created attachment 522323 [details] [diff] [review]
Patch
Comment 2 Mike Hommey [:glandium] 2011-03-28 02:32:27 PDT
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.
Comment 3 Mike Hommey [:glandium] 2011-03-28 06:43:31 PDT
Created attachment 522349 [details] [diff] [review]
Patch v2

This version should ensure the list of dependencies is always up-to-date
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-04-15 08:41:28 PDT
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.
Comment 5 Mike Hommey [:glandium] 2011-04-20 01:21:03 PDT
Created attachment 527212 [details] [diff] [review]
Patch v3

As discussed on irc last week.
Comment 6 Mike Hommey [:glandium] 2011-04-20 01:54:43 PDT
Comment on attachment 527212 [details] [diff] [review]
Patch v3

There are some problems with this patch :-/
Comment 7 Mike Hommey [:glandium] 2011-04-20 02:31:34 PDT
Created attachment 527219 [details] [diff] [review]
Patch v3.1

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 8 Ted Mielczarek [:ted.mielczarek] 2011-04-28 10:43:41 PDT
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.
Comment 9 Mike Hommey [:glandium] 2011-05-03 01:18:40 PDT
Created attachment 529650 [details] [diff] [review]
Implement full dependencies for expandlibs

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 10 Ted Mielczarek [:ted.mielczarek] 2011-05-12 07:50:49 PDT
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?
Comment 11 Mike Hommey [:glandium] 2011-05-12 07:54:07 PDT
(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.
Comment 12 Ted Mielczarek [:ted.mielczarek] 2011-05-12 07:59:39 PDT
Oh, right.
Comment 13 Mike Hommey [:glandium] 2011-05-16 05:46:54 PDT
http://hg.mozilla.org/mozilla-central/rev/6f4d392ed4aa
Comment 14 Mike Hommey [:glandium] 2011-05-16 06:54:39 PDT
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
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-16 06:57:37 PDT
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?
Comment 16 Mike Hommey [:glandium] 2011-05-16 07:00:48 PDT
(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.
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-16 07:02:43 PDT
(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.
Comment 18 Mike Hommey [:glandium] 2011-05-16 07:31:42 PDT
Backed out.
http://hg.mozilla.org/mozilla-central/rev/60cfdbaf76fb
Comment 19 Mike Hommey [:glandium] 2011-07-06 23:58:44 PDT
Landed a hopefully working version on build-system
http://hg.mozilla.org/projects/build-system/rev/8b7150ba4450
Comment 20 Mike Hommey [:glandium] 2011-07-07 05:52:15 PDT
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 \ ?
Comment 21 Justin Lebar (not reading bugmail) 2011-09-26 09:54:56 PDT
*** Bug 688999 has been marked as a duplicate of this bug. ***
Comment 22 Justin Lebar (not reading bugmail) 2011-09-29 14:50:56 PDT
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
Comment 23 Mike Hommey [:glandium] 2011-09-29 22:43:08 PDT
(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
Comment 24 Justin Lebar (not reading bugmail) 2011-09-30 07:02:32 PDT
Now I get

make[6]: *** No rule to make target `.deps', needed by `.deps/libs'.  Stop.
Comment 25 Justin Lebar (not reading bugmail) 2011-09-30 07:08:23 PDT
...but it maybe works if I add

$(MDDEPDIR): $(CURDIR)/$(MDDEPDIR)

under the rule for $(CURDIR)/$(MDDEPDIR).
Comment 26 Mike Hommey [:glandium] 2012-03-14 10:23:45 PDT
Created attachment 605833 [details] [diff] [review]
Implement full dependencies for expandlibs.

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.
Comment 28 Ed Morley [:emorley] 2012-03-31 05:35:44 PDT
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
Comment 29 Ed Morley [:emorley] 2012-03-31 05:43:32 PDT
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.
Comment 31 Mike Hommey [:glandium] 2012-04-06 04:39:40 PDT
Fixup for Windows builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/828112609eb9
Comment 32 Mike Hommey [:glandium] 2012-04-06 04:59:25 PDT
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
Comment 33 Mike Hommey [:glandium] 2012-04-06 07:59:49 PDT
Created attachment 612887 [details] [diff] [review]
Fixups

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.

Note You need to log in before you can comment on or make changes to this bug.