Clean up 'shell=True' in using subprocess module (tensorflow#12347)

* Set default shell=False in configure.py

* Fix wrong paths when building for other python

* Clean up 'shell=True' in using subprocess module

* Fix a regexp in configure.py

* Check existence before executing

For platform specific commands, use python functions instead if possible

* Fix typo in input prompt

* Fix a ldconfig mistake made in 3b6db82

Author: wangqr

configure.py

@@ -25,6 +25,11 @@
 import subprocess
 import sys
 
+try:
+  from shutil import which
+except ImportError:
+  from distutils.spawn import find_executable as which
+
 _TF_BAZELRC = '.tf_configure.bazelrc'
 _DEFAULT_CUDA_VERSION = '8.0'
 _DEFAULT_CUDNN_VERSION = '6'
@@ -53,6 +58,10 @@ def is_ppc64le():
   return platform.machine() == 'ppc64le'
 
 
+def is_cygwin():
+  return platform.system().startswith('CYGWIN_NT')
+
+
 def get_input(question):
   try:
     try:
@@ -121,13 +130,20 @@ def write_action_env_to_bazelrc(var_name, var):
   write_to_bazelrc('build --action_env %s="%s"' % (var_name, str(var)))
 
 
-def run_shell(cmd):
-  return subprocess.check_output(cmd, shell=True).decode('UTF-8').strip()
+def run_shell(cmd, allow_non_zero=False):
+  if allow_non_zero:
+    try:
+      output = subprocess.check_output(cmd)
+    except subprocess.CalledProcessError as e:
+      output = e.output
+  else:
+    output = subprocess.check_output(cmd)
+  return output.decode('UTF-8').strip()
 
 
 def cygpath(path):
   """Convert path from posix to windows."""
-  return run_shell('cygpath  -m "%s"' % path)
+  return run_shell(['cygpath', '-m', path])
 
 
 def get_python_path(environ_cp, python_bin_path):
@@ -136,16 +152,14 @@ def get_python_path(environ_cp, python_bin_path):
   if environ_cp.get('PYTHONPATH'):
     python_paths = environ_cp.get('PYTHONPATH').split(':')
   try:
-    check_input = [python_bin_path, '-c',
-                   'import site; print("\\n".join(site.getsitepackages()))']
-    library_paths = subprocess.check_output(
-        check_input).decode('UTF-8').strip().split("\n")
+    library_paths = run_shell(
+        [python_bin_path, '-c',
+         'import site; print("\\n".join(site.getsitepackages()))']).split("\n")
   except subprocess.CalledProcessError:
-    check_input = [python_bin_path, '-c',
-        'from distutils.sysconfig import get_python_lib;' +
-        'print(get_python_lib())']
-    library_paths = [subprocess.check_output(
-        check_input).decode('UTF-8').strip()]
+    library_paths = [run_shell(
+        [python_bin_path, '-c',
+         'from distutils.sysconfig import get_python_lib;'
+         'print(get_python_lib())'])]
 
   all_paths = set(python_paths + library_paths)
 
@@ -158,8 +172,7 @@ def get_python_path(environ_cp, python_bin_path):
 
 def get_python_major_version(python_bin_path):
   """Get the python major version."""
-  check_input = [python_bin_path, '-c', 'import sys; print(sys.version[0])']
-  return subprocess.check_output(check_input).decode('UTF-8').strip()
+  return run_shell([python_bin_path, '-c', 'import sys; print(sys.version[0])'])
 
 
 def setup_python(environ_cp, bazel_version):
@@ -173,8 +186,8 @@ def setup_python(environ_cp, bazel_version):
         environ_cp, 'PYTHON_BIN_PATH', ask_python_bin_path,
         default_python_bin_path)
     # Check if the path is valid
-    if (os.path.isfile(python_bin_path) and os.access(
-        python_bin_path, os.X_OK)) or (os.path.isdir(python_bin_path)):
+    if os.path.isfile(python_bin_path) and os.access(
+        python_bin_path, os.X_OK):
       break
     elif not os.path.exists(python_bin_path):
       print('Invalid python path: %s cannot be found.' % python_bin_path)
@@ -183,7 +196,7 @@ def setup_python(environ_cp, bazel_version):
     environ_cp['PYTHON_BIN_PATH'] = ''
 
   # Convert python path to Windows style before checking lib and version
-  if is_windows():
+  if is_cygwin():
     python_bin_path = cygpath(python_bin_path)
 
   # Get PYTHON_LIB_PATH
@@ -193,20 +206,20 @@ def setup_python(environ_cp, bazel_version):
     if environ_cp.get('USE_DEFAULT_PYTHON_LIB_PATH') == '1':
       python_lib_path = python_lib_paths[0]
     else:
-      print('Found possible Python library paths:\n%s' %
-            '\n'.join(python_lib_paths))
+      print('Found possible Python library paths:\n  %s' %
+            '\n  '.join(python_lib_paths))
       default_python_lib_path = python_lib_paths[0]
       python_lib_path = get_input(
-          'Please input the desired Python library path to use.  Default is %s'
-          % python_lib_paths[0])
+          'Please input the desired Python library path to use.  '
+          'Default is [%s]\n' % python_lib_paths[0])
       if not python_lib_path:
         python_lib_path = default_python_lib_path
     environ_cp['PYTHON_LIB_PATH'] = python_lib_path
 
   python_major_version = get_python_major_version(python_bin_path)
 
   # Convert python path to Windows style before writing into bazel.rc
-  if is_windows():
+  if is_cygwin():
     python_lib_path = cygpath(python_lib_path)
 
   # Set-up env variables used by python_configure.bzl
@@ -428,11 +441,10 @@ def check_bazel_version(min_version):
   Returns:
     The bazel version detected.
   """
-  try:
-    curr_version = run_shell('bazel --batch version')
-  except subprocess.CalledProcessError:
+  if which('bazel') is None:
     print('Cannot find bazel. Please install bazel.')
     sys.exit(0)
+  curr_version = run_shell(['bazel', '--batch', 'version'])
 
   for line in curr_version.split('\n'):
     if 'Build label: ' in line:
@@ -524,7 +536,7 @@ def get_from_env_or_user_or_default(environ_cp, var_name, ask_for_var,
 
 def set_clang_cuda_compiler_path(environ_cp):
   """Set CLANG_CUDA_COMPILER_PATH."""
-  default_clang_path = run_shell('which clang || true')
+  default_clang_path = which('clang') or ''
   ask_clang_path = ('Please specify which clang should be used as device and '
                     'host compiler. [Default is %s]: ') % default_clang_path
 
@@ -547,12 +559,12 @@ def set_clang_cuda_compiler_path(environ_cp):
 
 def set_gcc_host_compiler_path(environ_cp):
   """Set GCC_HOST_COMPILER_PATH."""
-  default_gcc_host_compiler_path = run_shell('which gcc || true')
+  default_gcc_host_compiler_path = which('gcc') or ''
   cuda_bin_symlink = '%s/bin/gcc' % environ_cp.get('CUDA_TOOLKIT_PATH')
 
   if os.path.islink(cuda_bin_symlink):
     # os.readlink is only available in linux
-    default_gcc_host_compiler_path = run_shell('readlink %s' % cuda_bin_symlink)
+    default_gcc_host_compiler_path = os.path.realpath(cuda_bin_symlink)
 
   ask_gcc_path = (
       'Please specify which gcc should be used by nvcc as the '
@@ -587,7 +599,7 @@ def set_tf_cuda_version(environ_cp):
 
     # Find out where the CUDA toolkit is installed
     default_cuda_path = _DEFAULT_CUDA_PATH
-    if is_windows():
+    if is_cygwin():
       default_cuda_path = cygpath(
           environ_cp.get('CUDA_PATH', _DEFAULT_CUDA_PATH_WIN))
     elif is_linux():
@@ -628,7 +640,7 @@ def set_tf_cuda_version(environ_cp):
 def set_tf_cunn_version(environ_cp):
   """Set CUDNN_INSTALL_PATH and TF_CUDNN_VERSION."""
   ask_cudnn_version = (
-      '"Please specify the cuDNN version you want to use. '
+      'Please specify the cuDNN version you want to use. '
       '[Leave empty to default to cuDNN %s.0]: ') % _DEFAULT_CUDNN_VERSION
 
   while True:
@@ -647,7 +659,7 @@ def set_tf_cunn_version(environ_cp):
     # unusable. Going through one more level of expansion to handle that.
     cudnn_install_path = os.path.realpath(
         os.path.expanduser(cudnn_install_path))
-    if is_windows():
+    if is_cygwin():
       cudnn_install_path = cygpath(cudnn_install_path)
 
     if is_windows():
@@ -669,12 +681,10 @@ def set_tf_cunn_version(environ_cp):
 
     # Try another alternative for Linux
     if is_linux():
-      if subprocess.call(['which', 'ldconfig']):
-        ldconfig_bin = '/sbin/ldconfig'
-      else:
-        ldconfig_bin = 'ldconfig'
-      cudnn_path_from_ldconfig = run_shell(
-          r'%s -p | sed -n "s/.*libcudnn.so .* => \(.*\)/\\1/p"' % ldconfig_bin)
+      ldconfig_bin = which('ldconfig') or '/sbin/ldconfig'
+      cudnn_path_from_ldconfig = run_shell([ldconfig_bin, '-p'])
+      cudnn_path_from_ldconfig = re.search('.*libcudnn.so .* => (.*)',
+                                           cudnn_path_from_ldconfig).group(1)
       if os.path.exists('%s.%s' % (cudnn_path_from_ldconfig, tf_cudnn_version)):
         cudnn_install_path = os.path.dirname(cudnn_path_from_ldconfig)
         break
@@ -707,11 +717,15 @@ def get_native_cuda_compute_capabilities(environ_cp):
   """
   device_query_bin = os.path.join(
       environ_cp.get('CUDA_TOOLKIT_PATH'), 'extras/demo_suite/deviceQuery')
-  cmd = (r'"%s" | grep "Capability" | grep -o "[0-9]*\.[0-9]*" | sed '
-         '":a;{N;s/\\n/,/};ba"') % device_query_bin
-  try:
-    output = run_shell(cmd)
-  except subprocess.CalledProcessError:
+  if os.path.isfile(device_query_bin) and os.access(device_query_bin, os.X_OK):
+    try:
+      output = run_shell(device_query_bin).split('\n')
+      pattern = re.compile('[0-9]*\\.[0-9]*')
+      output = [pattern.search(x) for x in output if 'Capability' in x]
+      output = ','.join(x.group() for x in output if x is not None)
+    except subprocess.CalledProcessError:
+      output = ''
+  else:
     output = ''
   return output
 
@@ -792,7 +806,7 @@ def set_other_cuda_vars(environ_cp):
 
 def set_host_cxx_compiler(environ_cp):
   """Set HOST_CXX_COMPILER."""
-  default_cxx_host_compiler = run_shell('which g++ || true')
+  default_cxx_host_compiler = which('g++') or ''
   ask_cxx_host_compiler = (
       'Please specify which C++ compiler should be used as'
       ' the host C++ compiler. [Default is %s]: ') % default_cxx_host_compiler
@@ -815,7 +829,7 @@ def set_host_cxx_compiler(environ_cp):
 
 def set_host_c_compiler(environ_cp):
   """Set HOST_C_COMPILER."""
-  default_c_host_compiler = run_shell('which gcc || true')
+  default_c_host_compiler = which('gcc') or ''
   ask_c_host_compiler = (
       'Please specify which C compiler should be used as the'
       ' host C compiler. [Default is %s]: ') % default_c_host_compiler
@@ -869,9 +883,9 @@ def set_computecpp_toolkit_path(environ_cp):
 
 def set_mpi_home(environ_cp):
   """Set MPI_HOME."""
-  cmd = ('dirname $(dirname $(which mpirun)) || dirname $(dirname $(which '
-         'mpiexec))  || true')
-  default_mpi_home = run_shell(cmd)
+  default_mpi_home = which('mpirun') or which('mpiexec') or ''
+  default_mpi_home = os.path.dirname(os.path.dirname(default_mpi_home))
+
   ask_mpi_home = ('Please specify the MPI toolkit folder. [Default is %s]: '
                  ) % default_mpi_home
   while True:

tensorflow/tools/ci_build/update_version.py

@@ -71,8 +71,7 @@ def check_all_files():
 
 def replace_with_sed(query, filename):
   """Replace with sed when regex is required."""
-  subprocess.check_call("sed -i -r -e \"%s\" \"%s\"" % (query, filename),
-                        shell=True)
+  subprocess.check_call(['sed', '-i', '-r', '-e', query, filename])
 
 
 class Version(object):
@@ -277,9 +276,8 @@ def check_for_lingering_string(lingering_string):
   """Check for given lingering strings."""
   formatted_string = lingering_string.replace(".", r"\.")
   try:
-    linger_strs = subprocess.check_output("grep -rnoH \"%s\" \"%s\""
-                                          % (formatted_string, TF_SRC_DIR),
-                                          shell=True).split("\n")
+    linger_strs = subprocess.check_output(
+        ['grep', '-rnoH', formatted_string, TF_SRC_DIR]).split("\n")
   except subprocess.CalledProcessError:
     linger_strs = []
 

tensorflow/tools/pip_package/check_load_py_test.py

@@ -42,11 +42,11 @@ def main():
   # Get all py_test target, note bazel query result will also include
   # cuda_py_test etc.
   try:
-    targets = subprocess.check_output(
-        'bazel query "kind(py_test, //tensorflow/contrib/... + '
+    targets = subprocess.check_output([
+        'bazel', 'query',
+        'kind(py_test, //tensorflow/contrib/... + '
         '//tensorflow/python/... - '
-        '//tensorflow/contrib/tensorboard/...)"',
-        shell=True).strip()
+        '//tensorflow/contrib/tensorboard/...)']).strip()
   except subprocess.CalledProcessError as e:
     targets = e.output
 
@@ -68,9 +68,8 @@ def main():
   files_missing_load = []
   for build_file in build_files:
     updated_build_file = subprocess.check_output(
-        'buildozer -stdout "new_load //tensorflow:tensorflow.bzl py_test" ' +
-        build_file,
-        shell=True)
+        ['buildozer', '-stdout', 'new_load //tensorflow:tensorflow.bzl py_test',
+         build_file])
     with open(build_file, 'r') as f:
       if f.read() != updated_build_file:
         files_missing_load.append(build_file)

tensorflow/tools/pip_package/pip_smoke_test.py

@@ -25,16 +25,16 @@
 
 import subprocess
 
-PIP_PACKAGE_QUERY = """bazel query \
-  'deps(//tensorflow/tools/pip_package:build_pip_package)'"""
+PIP_PACKAGE_QUERY_EXPRESSION = \
+  'deps(//tensorflow/tools/pip_package:build_pip_package)'
 
-PY_TEST_QUERY = """bazel query 'deps(\
+PY_TEST_QUERY_EXPRESSION = 'deps(\
   filter("^((?!benchmark).)*$",\
   kind(py_test,\
   //tensorflow/python/... \
   + //tensorflow/contrib/... \
   - //tensorflow/contrib/tensorboard/... \
-  - attr(tags, "manual|no_pip", //tensorflow/...))), 1)'"""
+  - attr(tags, "manual|no_pip", //tensorflow/...))), 1)'
 
 # Hard-coded blacklist of files if not included in pip package
 # TODO(amitpatankar): Clean up blacklist.
@@ -83,15 +83,15 @@ def main():
   """
 
   # pip_package_dependencies_list is the list of included files in pip packages
-  pip_package_dependencies = subprocess.check_output(
-      PIP_PACKAGE_QUERY, shell=True)
+  pip_package_dependencies = subprocess.check_output([
+      'bazel', 'query', PIP_PACKAGE_QUERY_EXPRESSION])
   pip_package_dependencies_list = pip_package_dependencies.strip().split("\n")
   print("Pip package superset size: %d" % len(pip_package_dependencies_list))
 
   # tf_py_test_dependencies is the list of dependencies for all python
   # tests in tensorflow
-  tf_py_test_dependencies = subprocess.check_output(
-      PY_TEST_QUERY, shell=True)
+  tf_py_test_dependencies = subprocess.check_output([
+      'bazel', 'query', PY_TEST_QUERY_EXPRESSION])
   tf_py_test_dependencies_list = tf_py_test_dependencies.strip().split("\n")
   print("Pytest dependency subset size: %d" % len(tf_py_test_dependencies_list))
 
@@ -124,9 +124,9 @@ def main():
     for missing_dependency in missing_dependencies:
       print("\nMissing dependency: %s " % missing_dependency)
       print("Affected Tests:")
-      rdep_query = """bazel query 'rdeps(kind(py_test, \
-      //tensorflow/python/...), %s)'""" % missing_dependency
-      affected_tests = subprocess.check_output(rdep_query, shell=True)
+      rdep_query = 'rdeps(kind(py_test, \
+      //tensorflow/python/...), %s)' % missing_dependency
+      affected_tests = subprocess.check_output(['bazel', 'query', rdep_query])
       affected_tests_list = affected_tests.split("\n")[:-2]
       print("\n".join(affected_tests_list))