Implement full dependencies for expandlibs

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

({dev-doc-needed})

Trunk
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

See bug 584474 comment 99 and bug 584474 comment 100.
Created attachment 522323 [details] [diff] [review]
Patch
Attachment #522323 - Flags: review?(ted.mielczarek)
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.
Attachment #522323 - Flags: review?(ted.mielczarek)
Created attachment 522349 [details] [diff] [review]
Patch v2

This version should ensure the list of dependencies is always up-to-date
Attachment #522323 - Attachment is obsolete: true
Attachment #522349 - Flags: review?(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.
Attachment #522349 - Flags: review?(ted.mielczarek) → review+
Created attachment 527212 [details] [diff] [review]
Patch v3

As discussed on irc last week.
Attachment #522349 - Attachment is obsolete: true
Attachment #527212 - Flags: review?(ted.mielczarek)
Comment on attachment 527212 [details] [diff] [review]
Patch v3

There are some problems with this patch :-/
Attachment #527212 - Flags: review?(ted.mielczarek)
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...
Attachment #527212 - Attachment is obsolete: true
Attachment #527219 - Flags: review?(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.
Attachment #527219 - Flags: review?(ted.mielczarek) → review-
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)
Attachment #529650 - Flags: review?(ted.mielczarek)
Attachment #527219 - Attachment is obsolete: true
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?
Attachment #529650 - Flags: review?(ted.mielczarek) → review+
(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.
http://hg.mozilla.org/mozilla-central/rev/6f4d392ed4aa
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
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.
Keywords: dev-doc-needed
Backed out.
http://hg.mozilla.org/mozilla-central/rev/60cfdbaf76fb
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Landed a hopefully working version on build-system
http://hg.mozilla.org/projects/build-system/rev/8b7150ba4450
Whiteboard: fixed-in-bs
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 \ ?
Whiteboard: fixed-in-bs
Duplicate of this bug: 688999
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).
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.
Attachment #605833 - Flags: review?(ted.mielczarek)
Attachment #529650 - Attachment is obsolete: true
Attachment #605833 - Flags: review?(ted.mielczarek) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/82c315c0bd14
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
Target Milestone: mozilla6 → ---
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.
Depends on: 741287
https://hg.mozilla.org/integration/mozilla-inbound/rev/afbb19dccbe2
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
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.
Attachment #612887 - Flags: review?(ted.mielczarek)
Attachment #612887 - Flags: review?(ted.mielczarek) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a54c9b8aa9e4
https://hg.mozilla.org/mozilla-central/rev/a54c9b8aa9e4
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Depends on: 748001
You need to log in before you can comment on or make changes to this bug.