Generate dependentlibs.list from actual library dependencies

RESOLVED FIXED in mozilla16

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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)
Created attachment 632220 [details] [diff] [review]
Generate dependentlibs.list from actual library dependencies
Attachment #632220 - Flags: review?(ted.mielczarek)
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.
Attachment #632220 - Flags: review?(ted.mielczarek) → review+
(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?
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/569f816a542b
Target Milestone: --- → mozilla16
(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
Backed out. There's a race condition in xpcom/stub/Makefile.in
https://hg.mozilla.org/integration/mozilla-inbound/rev/03341a974e4a
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.
Attachment #634846 - Flags: review?(ted.mielczarek)
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.
Attachment #634846 - Flags: review?(ted.mielczarek) → review+
Folded both patches when landing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a57052eb374b
https://hg.mozilla.org/mozilla-central/rev/a57052eb374b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 767024
Depends on: 766975
Blocks: 767403

Updated

5 years ago
Depends on: 769607
You need to log in before you can comment on or make changes to this bug.