add Subproc class to handle env change needed for subprocess (#747)

Due to bugs in pyinstaller and setuptools we need to modify the
environment when forking a command.

cli/dcoscli/help/main.py

@@ -1,9 +1,8 @@
-import subprocess
 from concurrent.futures import ThreadPoolExecutor
 
 import dcoscli
 import docopt
-from dcos import cmds, emitting, options, subcommand, util
+from dcos import cmds, emitting, options, subcommand, subprocess, util
 from dcos.errors import DCOSException
 from dcoscli.subcommand import (default_command_documentation,
                                 default_command_info, default_doc)
@@ -107,4 +106,4 @@ def _help_command(command):
         return 0
     else:
         executable = subcommand.command_executables(command)
-        return subprocess.call([executable, command, '--help'])
+        return subprocess.Subproc().call([executable, command, '--help'])

cli/dcoscli/node/main.py

@@ -1,12 +1,11 @@
 import functools
 import os
-import subprocess
 
 import dcoscli
 import docopt
 import six
 from dcos import (cmds, config, cosmospackage, emitting, errors, http, mesos,
-                  util)
+                  subprocess, util)
 from dcos.errors import DCOSException, DefaultError
 from dcoscli import log, tables
 from dcoscli.package.main import confirm, get_cosmos_url
@@ -587,4 +586,4 @@ def _ssh(leader, slave, option, config_file, user, master_proxy, command):
                          "network than DC/OS, consider using "
                          "`--master-proxy`"))
 
-    return subprocess.call(cmd, shell=True)
+    return subprocess.Subproc().call(cmd, shell=True)

cli/dcoscli/service/main.py

@@ -1,9 +1,7 @@
-import subprocess
-
 import dcoscli
 import docopt
 import six
-from dcos import cmds, emitting, marathon, mesos, util
+from dcos import cmds, emitting, marathon, mesos, subprocess, util
 from dcos.errors import DCOSException, DefaultError
 from dcoscli import log, tables
 from dcoscli.subcommand import default_command_info, default_doc
@@ -277,4 +275,4 @@ def _log_marathon(follow, lines, ssh_config_file):
 
     emitter.publish(DefaultError("Running `{}`".format(cmd)))
 
-    return subprocess.call(cmd, shell=True)
+    return subprocess.Subproc().call(cmd, shell=True)

dcos/subcommand.py

@@ -1,6 +1,5 @@
 from __future__ import print_function
 
-import functools
 import hashlib
 import json
 import os
@@ -11,11 +10,12 @@ import subprocess
 import sys
 import zipfile
 from distutils.version import LooseVersion
-from subprocess import PIPE, Popen
+from subprocess import PIPE
 
 import requests
 from dcos import constants, util
 from dcos.errors import DCOSException
+from dcos.subprocess import Subproc
 
 logger = util.get_logger(__name__)
 
@@ -159,36 +159,6 @@ def documentation(executable_path):
     return (path_noun, info(executable_path, path_noun))
 
 
-def executable_env(fn):
-    """Decorator for environment fork/execs should run under
-
-    Setuptools overrides path to executable from virtualenv,
-    modify this so we can specify a different path
-
-    :param fn: function that fork/execs
-    :type fn: function
-    :rtype: Response
-    :returns: Response
-    """
-
-    @functools.wraps(fn)
-    def update_running_env(*args, **kwargs):
-        """Run fork/exec under modified environment
-
-        :param response: response from cosmos
-        :type response: Response
-        :returns: Response or raises Exception
-        :rtype: valid response
-        """
-
-        with util.set_env('__PYVENV_LAUNCHER__', None):
-            cmd = fn(*args, **kwargs)
-        return cmd
-
-    return update_running_env
-
-
-@executable_env
 def info(executable_path, path_noun):
     """Collects subcommand information
 
@@ -200,13 +170,12 @@ def info(executable_path, path_noun):
     :rtype: str
     """
 
-    out = subprocess.check_output(
+    out = Subproc().check_output(
         [executable_path, path_noun, '--info'])
 
     return out.decode('utf-8').strip()
 
 
-@executable_env
 def config_schema(executable_path, noun=None):
     """Collects subcommand config schema
 
@@ -220,7 +189,7 @@ def config_schema(executable_path, noun=None):
     if noun is None:
         noun = noun(executable_path)
 
-    out = subprocess.check_output(
+    out = Subproc().check_output(
         [executable_path, noun, '--config-schema'])
 
     return json.loads(out.decode('utf-8'))
@@ -597,7 +566,6 @@ def _install_with_pip(
     return None
 
 
-@executable_env
 def _execute_command(command):
     """
     :param command: a command to execute
@@ -608,7 +576,7 @@ def _execute_command(command):
 
     logger.info('Calling: %r', command)
 
-    process = subprocess.Popen(
+    process = Subproc().Popen(
         command,
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE)
@@ -703,7 +671,6 @@ class SubcommandProcess():
         self._command = command
         self._args = args
 
-    @executable_env
     def run_and_capture(self):
         """
         Run a command and capture exceptions. This is a blocking call
@@ -711,8 +678,9 @@ class SubcommandProcess():
         :rtype: int, str | None
         """
 
-        subproc = Popen([self._executable,  self._command] + self._args,
-                        stderr=PIPE)
+        subproc = subprocess.Subproc().Popen(
+            [self._executable,  self._command] + self._args,
+            stderr=PIPE)
 
         err = ''
         while subproc.poll() is None:

dcos/subprocess.py

@@ -0,0 +1,64 @@
+import os
+import subprocess
+
+
+class Subproc():
+    """Represents a wrapper for subprocess
+    """
+
+    def __init__(self):
+        self._env = os.environ.copy()
+        lib_path = 'LD_LIBRARY_PATH'
+        lib_path_value = self._env.get(lib_path + '_ORIG')
+        # remove pyinstaller overriding `LD_LIBRARY_PATH`
+        # remove with stable release of fix:
+        # https://github.com/pyinstaller/pyinstaller/pull/2148
+        if lib_path_value is not None:
+            self._env[lib_path] = lib_path_value
+        elif self._env.get(lib_path):
+            del self._env[lib_path]
+
+        # Setuptools overrides path to executable from virtualenv,
+        # modify this so we can specify a different path
+        launcher = '__PYVENV_LAUNCHER__'
+        pyvenv_launcher = self._env.get(launcher)
+        if pyvenv_launcher is not None:
+            del self._env[launcher]
+
+    def check_output(self, args, stdin=None, stderr=None, shell=False):
+        """
+        call subprocess.check_ouput with modified environment
+        """
+
+        return subprocess.check_output(
+            args,
+            stdin=stdin,
+            stderr=stderr,
+            shell=shell,
+            env=self._env)
+
+    def Popen(self, args, stdin=None, stdout=None, stderr=None, shell=False):
+        """
+        call subprocess.Popen with modified environment
+        """
+
+        return subprocess.Popen(
+            args,
+            stdin=stdin,
+            stdout=stdout,
+            stderr=stderr,
+            shell=shell,
+            env=self._env)
+
+    def call(self, args, stdin=None, stdout=None, stderr=None, shell=False):
+        """
+        call subprocess.call with modified environment
+        """
+
+        return subprocess.call(
+            args,
+            stdin=stdin,
+            stdout=stdout,
+            stderr=stderr,
+            shell=shell,
+            env=self._env)

dcos/util.py

@@ -32,31 +32,6 @@ def get_logger(name):
     return logging.getLogger(name)
 
 
-@contextlib.contextmanager
-def set_env(env_var, new_value):
-    """A context manager for temporary updating env vars
-
-    :param env_var: name of environment variable to alter, None to remove
-    :type env_var: str
-    :param new_val: new value to change env_var to
-    :type new_var: str | None
-    :rtype: str | None
-    """
-
-    old_value = os.environ.get(env_var)
-    if new_value is not None:
-        os.environ[env_var] = new_value
-    elif new_value is None and old_value is not None:
-        del os.environ[env_var]
-    try:
-        yield
-    finally:
-        if old_value is not None:
-            os.environ[env_var] = old_value
-        elif old_value is None and new_value is not None:
-            del os.environ[env_var]
-
-
 @contextlib.contextmanager
 def tempdir():
     """A context manager for temporary directories.