Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 769607 - depends on dumpbin
: depends on dumpbin
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Jacek Caban
: Gregory Szorc [:gps]
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

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, 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/
@@ +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:
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:46:35 PDT

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