644608 - Implement full dependencies for expandlibs

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

See bug 584474 comment 99 and bug 584474 comment 100.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Patch

Comment 2

[Mike Hommey [:glandium]]

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]]

Attachment: Patch v2

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

Comment 4

[(not currently active) Ted Mielczarek]

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]]

Attachment: Patch v3

As discussed on irc last week.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 527212 [details] [diff] [review]
Patch v3

There are some problems with this patch :-/

Comment 7

[Mike Hommey [:glandium]]

Attachment: 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

[(not currently active) Ted Mielczarek]

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]]

Attachment: 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

[(not currently active) Ted Mielczarek]

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]]

(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

[(not currently active) Ted Mielczarek]

Oh, right.

Comment 13

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/6f4d392ed4aa

Comment 14

[Mike Hommey [:glandium]]

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 (Exited; not receiving bugmail, old account, do not use)]

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]]

(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 (Exited; not receiving bugmail, old account, do not use)]

(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]]

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

Comment 19

[Mike Hommey [:glandium]]

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

Comment 20

[Mike Hommey [:glandium]]

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 22

[Justin Lebar (not reading bugmail)]

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]]

(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)]

Now I get

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

Comment 25

[Justin Lebar (not reading bugmail)]

...but it maybe works if I add

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

under the rule for $(CURDIR)/$(MDDEPDIR).

Comment 26

[Mike Hommey [:glandium]]

Attachment: 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 27

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c315c0bd14

Comment 28

[Ed Morley [:emorley]]

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]]

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 30

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afbb19dccbe2

Comment 31

[Mike Hommey [:glandium]]

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

Comment 32

[Mike Hommey [:glandium]]

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]]

Attachment: 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.

Comment 34

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a54c9b8aa9e4

Comment 35

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a54c9b8aa9e4