The default bug view has changed. See this FAQ.

dependentlibs.py depends on dumpbin

RESOLVED FIXED in mozilla16

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jacek Caban, Assigned: Jacek Caban)

Tracking

Trunk
mozilla16
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.70 KB, patch
glandium
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 637839 [details] [diff] [review]
fix v1.0

This causes problems with mingw compilation, since it's a MSVC tool. There is an alternative that looks really promising. It's pefile python library [1]. The attached patch imports current trunk version of pefile and uses it in dependentlibs.py instead of dumpbin. This way we can handle PE files without any external dependency. Although the patch is quite big, it's because it imports whole pefile.py. This file required no modifications at all, so there is no maintenance cost. The library is quite powerful - it allows both extensive analyzing of PE files and modifying them. We don't need most of it right not, but the code is in place if needed at some point and it's easier to import it all than split parts (also in terms of maintenance). The license of pefile is MIT, so, as far as I understand, there is nothing special we have to do about it.

The modification of dependentlibs.py is very minimal.

[1] http://code.google.com/p/pefile/
Attachment #637839 - Flags: review?(mh+mozilla)
Comment on attachment 637839 [details] [diff] [review]
fix v1.0

I'm not a big fan of pulling 200KB of external code just to do that. I'd be more in favor of something like what i did for virtualenv for OSX in https://github.com/pypa/virtualenv/pull/289/files, instead of importing macholib. Alternatively, doesn't mingw come with an objdump that knows how to read PE files? You could also parse its output.
Attachment #637839 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 2

5 years ago
Created attachment 640563 [details] [diff] [review]
fix v2.0

I'd prefer to follow your first suggestion, since parsing output of tools not really intended to be parsed is always hacky, but let's go with much simpler solution of using objdump. The attached patch uses objdump if dumpbin fails to execute.
Attachment #637839 - Attachment is obsolete: true
Attachment #640563 - Flags: review?(mh+mozilla)
Comment on attachment 640563 [details] [diff] [review]
fix v2.0

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

::: xpcom/stub/dependentlibs.py
@@ +20,5 @@
> +    try:
> +        proc = subprocess.Popen(['dumpbin', '-imports', lib], stdout = subprocess.PIPE)
> +    except OSError:
> +        # dumpbin is missing, probably mingw compilation. Try using objdump.
> +        return dependentlibs_objdump(lib)

Please make that dependentlibs_mingw_objdump, since the mingw objdump output is significantly different from that of ELF objdump. r=me with that.
Attachment #640563 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 4

5 years ago
Thanks for the review, pushed fixed version:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e25fe33268d
https://hg.mozilla.org/mozilla-central/rev/9e25fe33268d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.