MAINT: use list-based APIs to call subprocesses (#15714)

* MAINT: use list-based APIs to call subprocesses
* TST, MAINT: add a test for mingw32ccompiler.build_import, clean up lib2def
Co-authored-by: Matti Picus <matti.picus@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

4 files changed, 79 insertions, 34 deletions

diff --git a/numpy/distutils/lib2def.py b/numpy/distutils/lib2def.py
index c6d445d09..820ed71f5 100644
--- a/numpy/distutils/lib2def.py
+++ b/numpy/distutils/lib2def.py
@@ -22,7 +22,7 @@ __version__ = '0.1a'
 
 py_ver = "%d%d" % tuple(sys.version_info[:2])
 
-DEFAULT_NM = 'nm -Cs'
+DEFAULT_NM = ['nm', '-Cs']
 
 DEF_HEADER = """LIBRARY         python%s.dll
 ;CODE           PRELOAD MOVEABLE DISCARDABLE
@@ -59,13 +59,16 @@ libfile, deffile = parse_cmd()"""
         deffile = None
     return libfile, deffile
 
-def getnm(nm_cmd = ['nm', '-Cs', 'python%s.lib' % py_ver]):
+def getnm(nm_cmd=['nm', '-Cs', 'python%s.lib' % py_ver], shell=True):
     """Returns the output of nm_cmd via a pipe.
 
-nm_output = getnam(nm_cmd = 'nm -Cs py_lib')"""
-    f = subprocess.Popen(nm_cmd, shell=True, stdout=subprocess.PIPE, universal_newlines=True)
-    nm_output = f.stdout.read()
-    f.stdout.close()
+nm_output = getnm(nm_cmd = 'nm -Cs py_lib')"""
+    p = subprocess.Popen(nm_cmd, shell=shell, stdout=subprocess.PIPE,
+                         stderr=subprocess.PIPE, universal_newlines=True)
+    nm_output, nm_err = p.communicate()
+    if p.returncode != 0:
+        raise RuntimeError('failed to run "%s": "%s"' % (
+                                     ' '.join(nm_cmd), nm_err))
     return nm_output
 
 def parse_nm(nm_output):
@@ -107,7 +110,7 @@ if __name__ == '__main__':
         deffile = sys.stdout
     else:
         deffile = open(deffile, 'w')
-    nm_cmd = [str(DEFAULT_NM), str(libfile)]
-    nm_output = getnm(nm_cmd)
+    nm_cmd = DEFAULT_NM + [str(libfile)]
+    nm_output = getnm(nm_cmd, shell=False)
     dlist, flist = parse_nm(nm_output)
     output_def(dlist, flist, DEF_HEADER, deffile)
diff --git a/numpy/distutils/mingw32ccompiler.py b/numpy/distutils/mingw32ccompiler.py
index 031b7c95f..7cb6fadcc 100644
--- a/numpy/distutils/mingw32ccompiler.py
+++ b/numpy/distutils/mingw32ccompiler.py
@@ -64,10 +64,10 @@ class Mingw32CCompiler(distutils.cygwinccompiler.CygwinCCompiler):
         # we need to support 3.2 which doesn't match the standard
         # get_versions methods regex
         if self.gcc_version is None:
-            p = subprocess.Popen(['gcc', '-dumpversion'], shell=True,
-                                 stdout=subprocess.PIPE)
-            out_string = p.stdout.read()
-            p.stdout.close()
+            try:
+                out_string  = subprocess.check_output(['gcc', '-dumpversion'])
+            except (OSError, CalledProcessError):
+                out_string = ""  # ignore failures to match old behavior
             result = re.search(r'(\d+\.\d+)', out_string)
             if result:
                 self.gcc_version = StrictVersion(result.group(1))
@@ -278,8 +278,8 @@ def find_python_dll():
     raise ValueError("%s not found in %s" % (dllname, lib_dirs))
 
 def dump_table(dll):
-    st = subprocess.Popen(["objdump.exe", "-p", dll], stdout=subprocess.PIPE)
-    return st.stdout.readlines()
+    st = subprocess.check_output(["objdump.exe", "-p", dll])
+    return st.split(b'\n')
 
 def generate_def(dll, dfile):
     """Given a dll file location,  get all its exported symbols and dump them
@@ -304,15 +304,14 @@ def generate_def(dll, dfile):
     if len(syms) == 0:
         log.warn('No symbols found in %s' % dll)
 
-    d = open(dfile, 'w')
-    d.write('LIBRARY        %s\n' % os.path.basename(dll))
-    d.write(';CODE          PRELOAD MOVEABLE DISCARDABLE\n')
-    d.write(';DATA          PRELOAD SINGLE\n')
-    d.write('\nEXPORTS\n')
-    for s in syms:
-        #d.write('@%d    %s\n' % (s[0], s[1]))
-        d.write('%s\n' % s[1])
-    d.close()
+    with open(dfile, 'w') as d:
+        d.write('LIBRARY        %s\n' % os.path.basename(dll))
+        d.write(';CODE          PRELOAD MOVEABLE DISCARDABLE\n')
+        d.write(';DATA          PRELOAD SINGLE\n')
+        d.write('\nEXPORTS\n')
+        for s in syms:
+            #d.write('@%d    %s\n' % (s[0], s[1]))
+            d.write('%s\n' % s[1])
 
 def find_dll(dll_name):
 
@@ -465,7 +464,7 @@ def _build_import_library_amd64():
 
     # generate import library from this symbol list
     cmd = ['dlltool', '-d', def_file, '-l', out_file]
-    subprocess.Popen(cmd)
+    subprocess.check_call(cmd)
 
 def _build_import_library_x86():
     """ Build the import libraries for Mingw32-gcc on Windows
@@ -499,16 +498,19 @@ def _build_import_library_x86():
 
     def_name = "python%d%d.def" % tuple(sys.version_info[:2])
     def_file = os.path.join(sys.prefix, 'libs', def_name)
-    nm_cmd = '%s %s' % (lib2def.DEFAULT_NM, lib_file)
-    nm_output = lib2def.getnm(nm_cmd)
+    nm_output = lib2def.getnm(
+            lib2def.DEFAULT_NM + [lib_file], shell=False)
     dlist, flist = lib2def.parse_nm(nm_output)
-    lib2def.output_def(dlist, flist, lib2def.DEF_HEADER, open(def_file, 'w'))
+    with open(def_file, 'w') as fid:
+        lib2def.output_def(dlist, flist, lib2def.DEF_HEADER, fid)
 
     dll_name = find_python_dll ()
-    args = (dll_name, def_file, out_file)
-    cmd = 'dlltool --dllname "%s" --def "%s" --output-lib "%s"' % args
-    status = os.system(cmd)
-    # for now, fail silently
+
+    cmd = ["dlltool",
+           "--dllname", dll_name,
+           "--def", def_file,
+           "--output-lib", out_file]
+    status = subprocess.check_output(cmd)
     if status:
         log.warn('Failed to build import library for gcc. Linking will fail.')
     return
@@ -541,6 +543,8 @@ if sys.platform == 'win32':
         # Value from msvcrt.CRT_ASSEMBLY_VERSION under Python 3.3.0
         # on Windows XP:
         _MSVCRVER_TO_FULLVER['100'] = "10.0.30319.460"
+        # Python 3.7 uses 1415, but get_build_version returns 140 ??
+        _MSVCRVER_TO_FULLVER['140'] = "14.15.26726.0"
         if hasattr(msvcrt, "CRT_ASSEMBLY_VERSION"):
             major, minor, rest = msvcrt.CRT_ASSEMBLY_VERSION.split(".", 2)
             _MSVCRVER_TO_FULLVER[major + minor] = msvcrt.CRT_ASSEMBLY_VERSION
diff --git a/numpy/distutils/misc_util.py b/numpy/distutils/misc_util.py
index ea8128eab..9f9e9f1ac 100644
--- a/numpy/distutils/misc_util.py
+++ b/numpy/distutils/misc_util.py
@@ -1853,8 +1853,7 @@ class Configuration:
         """Return path's SVN revision number.
         """
         try:
-            output = subprocess.check_output(
-                ['svnversion'], shell=True, cwd=path)
+            output = subprocess.check_output(['svnversion'], cwd=path)
         except (subprocess.CalledProcessError, OSError):
             pass
         else:
@@ -1884,7 +1883,7 @@ class Configuration:
         """
         try:
             output = subprocess.check_output(
-                ['hg identify --num'], shell=True, cwd=path)
+                ['hg', 'identify', '--num'], cwd=path)
         except (subprocess.CalledProcessError, OSError):
             pass
         else:
diff --git a/numpy/distutils/tests/test_mingw32ccompiler.py b/numpy/distutils/tests/test_mingw32ccompiler.py
new file mode 100644
index 000000000..100fba429
--- /dev/null
+++ b/numpy/distutils/tests/test_mingw32ccompiler.py
@@ -0,0 +1,39 @@
+import shutil
+import subprocess
+import sys
+import pytest
+
+from numpy.distutils import mingw32ccompiler
+
+
+@pytest.mark.skipif(sys.platform != 'win32', reason='win32 only test')
+def test_build_import():
+    '''Test the mingw32ccompiler.build_import_library, which builds a
+    `python.a` from the MSVC `python.lib`
+    '''
+
+    # make sure `nm.exe` exists and supports the current python version. This
+    # can get mixed up when the PATH has a 64-bit nm but the python is 32-bit
+    try:
+        out = subprocess.check_output(['nm.exe', '--help'])
+    except FileNotFoundError:
+        pytest.skip("'nm.exe' not on path, is mingw installed?")
+    supported = out[out.find(b'supported targets:'):]
+    if sys.maxsize < 2**32 and b'pe-i386' not in supported:
+        raise ValueError("'nm.exe' found but it does not support 32-bit "
+                         "dlls when using 32-bit python")
+    elif b'pe-x86-64' not in supported:
+        raise ValueError("'nm.exe' found but it does not support 64-bit "
+                         "dlls when using 64-bit python")
+    # Hide the import library to force a build
+    has_import_lib, fullpath = mingw32ccompiler._check_for_import_lib()
+    if has_import_lib: 
+        shutil.move(fullpath, fullpath + '.bak')
+
+    try: 
+        # Whew, now we can actually test the function
+        mingw32ccompiler.build_import_library()
+
+    finally:
+        if has_import_lib:
+            shutil.move(fullpath + '.bak', fullpath)