Last Comment Bug 763893 - Generate dependentlibs.list from actual library dependencies
: Generate dependentlibs.list from actual library dependencies
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mike Hommey [:glandium]
:
: Gregory Szorc [:gps]
Mentors:
Depends on: 766975 767024 769607
Blocks: 767403
  Show dependency treegraph
 
Reported: 2012-06-12 05:44 PDT by Mike Hommey [:glandium]
Modified: 2012-06-29 04:03 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Generate dependentlibs.list from actual library dependencies (5.90 KB, patch)
2012-06-12 06:05 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Fixup (3.74 KB, patch)
2012-06-20 04:42 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2012-06-12 05:44:21 PDT
Currently, dependentlibs.list is created from a more or less hardcoded list of libraries, which is error-prone and limited (see bug 762621 for an example of problem this creates)
Comment 1 Mike Hommey [:glandium] 2012-06-12 06:05:22 PDT
Created attachment 632220 [details] [diff] [review]
Generate dependentlibs.list from actual library dependencies
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-06-18 12:18:42 PDT
Comment on attachment 632220 [details] [diff] [review]
Generate dependentlibs.list from actual library dependencies

Review of attachment 632220 [details] [diff] [review]:
-----------------------------------------------------------------

I'm assuming that the ordering that comes out of these tools is the proper library dependency order? I suppose your recursive-traversal will make that work in any event, right?

::: xpcom/stub/Makefile.in
@@ +55,5 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  libs:: $(FINAL_TARGET)/dependentlibs.list
>  
> +$(FINAL_TARGET)/dependentlibs.list: dependentlibs.py $(FINAL_TARGET)/$(SHARED_LIBRARY) $(addprefix $(FINAL_TARGET)/,$(shell cat $(FINAL_TARGET)/dependentlibs.list))

I guess $(shell) doesn't actually fail if the command fails, but we'll still get some error output here. You could wrap this last dep in $(if $(wildcard $(FINAL_TARGET)/dependentlibs.list)) to make that nicer.

::: xpcom/stub/dependentlibs.py
@@ +12,5 @@
> +import sys
> +
> +def dependentlibs_dumpbin(lib):
> +    '''Returns the list of dependencies declared in the given DLL'''
> +    proc = subprocess.Popen(['dumpbin', '-imports', lib], stdout = subprocess.PIPE, stderr = subprocess.PIPE)

If you don't care about stderr in these cases (which you don't seem to), you could skip PIPEing it, skip the .communicate, and just do "for line in proc.stdout:". You'd also have to proc.wait() at the end.

@@ +35,5 @@
> +        tmp = line.split(' ', 3)
> +        if len(tmp) > 2 and tmp[2] == '(NEEDED)':
> +            # NEEDED lines look like:
> +            # 0x00000001 (NEEDED)             Shared library: [libname]
> +            match = re.search('\[(.*)\]', tmp[3])

You only check for len(tmp) > 2 above, you probably want to check > 3.

@@ +67,5 @@
> +    in the same directory'''
> +    deps = []
> +    dir = os.path.dirname(lib)
> +    for dep in func(lib):
> +        if dep in deps or os.path.isabs(dep):

You could probably (prematurely) optimize this a little bit by using a set() to hold the "already seen-ness" of the files. I'm assuming you want a list because the ordering is important? You could maintain both, but that might be a pain.
Comment 3 Mike Hommey [:glandium] 2012-06-18 22:44:35 PDT
(In reply to Ted Mielczarek [:ted] from comment #2)
> Comment on attachment 632220 [details] [diff] [review]
> Generate dependentlibs.list from actual library dependencies
> 
> Review of attachment 632220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm assuming that the ordering that comes out of these tools is the proper
> library dependency order? I suppose your recursive-traversal will make that
> work in any event, right?

That's the intent, at least :)

> @@ +67,5 @@
> > +    in the same directory'''
> > +    deps = []
> > +    dir = os.path.dirname(lib)
> > +    for dep in func(lib):
> > +        if dep in deps or os.path.isabs(dep):
> 
> You could probably (prematurely) optimize this a little bit by using a set()
> to hold the "already seen-ness" of the files. I'm assuming you want a list
> because the ordering is important? You could maintain both, but that might
> be a pain.

Or use an OrderedSet ; the recipe at http://code.activestate.com/recipes/576694/ is >= 2.6, I guess it's possible to make it work on 2.5. (and we don't need the whole thing either)
On the other hand, that's a lot of code for little benefit. Do we really care?
Comment 4 Ted Mielczarek [:ted.mielczarek] 2012-06-19 05:18:34 PDT
It's not worth that much effort, no. Hence my "premature" comment. Since this only gets run once per build anyway, it's not a big deal.
Comment 6 Justin Wood (:Callek) 2012-06-20 00:09:38 PDT
(In reply to Mike Hommey [:glandium] from comment #5)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/569f816a542b

if this sticks, after merge to central, rs+=me to remove extradependlibs.mk from all c-c
Comment 7 Mike Hommey [:glandium] 2012-06-20 01:29:29 PDT
Backed out. There's a race condition in xpcom/stub/Makefile.in
https://hg.mozilla.org/integration/mozilla-inbound/rev/03341a974e4a
Comment 8 Mike Hommey [:glandium] 2012-06-20 04:42:36 PDT
Created attachment 634846 [details] [diff] [review]
Fixup

The race condition was that dependentlibs.list could be attempted to be generated before libxpcom is copied to FINAL_TARGET.
Since that happens during libs, it's not really fixable through dependencies. So I just added an option for a library search path, which allows to generate dependentlibs.list without libxpcom being in FINAL_TARGET, where its dependencies are.

Another problem is that b2g doesn't have a libmozalloc.so (it's built statically), but the lib appears in dependentlibs.list (sic), and the dependency breaks incremental builds from before to after. This is fixable with a clobber, but this also means this would fail if we ever rename a lib, remove a lib, etc.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2012-06-20 08:16:16 PDT
Comment on attachment 634846 [details] [diff] [review]
Fixup

Review of attachment 634846 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/stub/dependentlibs.py
@@ +66,5 @@
> +def dependentlibs(lib, libpaths, func):
> +    '''For a given library, returns the list of recursive dependencies that can
> +    be found in the given list of paths'''
> +    assert(libpaths)
> +    assert(isinstance(libpaths, list))

Seems like overkill, but okay.
Comment 10 Mike Hommey [:glandium] 2012-06-20 23:05:28 PDT
Folded both patches when landing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a57052eb374b
Comment 11 Ed Morley [:emorley] 2012-06-21 04:02:36 PDT
https://hg.mozilla.org/mozilla-central/rev/a57052eb374b

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