Last Comment Bug 769607 - dependentlibs.py depends on dumpbin
: dependentlibs.py depends on dumpbin
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Jacek Caban
:
:
Mentors:
Depends on:
Blocks: 763893
  Show dependency treegraph
 
Reported: 2012-06-29 04:03 PDT by Jacek Caban
Modified: 2012-07-10 15:46 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (189.07 KB, patch)
2012-06-29 04:03 PDT, Jacek Caban
mh+mozilla: review-
Details | Diff | Splinter Review
fix v2.0 (1.70 KB, patch)
2012-07-10 04:05 PDT, Jacek Caban
mh+mozilla: review+
Details | Diff | Splinter Review

Description Jacek Caban 2012-06-29 04:03:17 PDT
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/
Comment 1 Mike Hommey [:glandium] 2012-06-29 05:53:11 PDT
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.
Comment 2 Jacek Caban 2012-07-10 04:05:43 PDT
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.
Comment 3 Mike Hommey [:glandium] 2012-07-10 04:27:55 PDT
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.
Comment 4 Jacek Caban 2012-07-10 05:35:13 PDT
Thanks for the review, pushed fixed version:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e25fe33268d
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:46:35 PDT
https://hg.mozilla.org/mozilla-central/rev/9e25fe33268d

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