Note: There are a few cases of duplicates in user autocompletion which are being worked on. depends on dumpbin

RESOLVED FIXED in mozilla16



Build Config
5 years ago
5 years ago


(Reporter: Jacek Caban, Assigned: Jacek Caban)


Windows 7

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

1.70 KB, patch
: review+
Details | Diff | Splinter Review


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 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 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 is very minimal.

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

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/
@@ +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+

Comment 4

5 years ago
Thanks for the review, pushed fixed version:
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.