Fix environment variables being passed to subprocess while packaging

Fixes aws#501

chalice/compat.py

@@ -8,17 +8,6 @@
 
 if os.name == 'nt':
     # windows
-    # On windows running python in a subprocess with no environment variables
-    # will cause several issues. In order for our subprocess to run normally we
-    # manually copy the relevant environment variables from the parent process.
-    subprocess_python_base_environ = {
-        # http://bugs.python.org/issue8557
-        'PATH': os.environ['PATH']
-    }  # type: Dict[str, Any]
-    # http://bugs.python.org/issue20614
-    if 'SYSTEMROOT' in os.environ:
-        subprocess_python_base_environ['SYSTEMROOT'] = os.environ['SYSTEMROOT']
-
     # This is the actual patch used on windows to prevent distutils from
     # compiling C extensions. The msvc compiler base class has its compile
     # method overridden to raise a CompileError. This can be caught by
@@ -94,10 +83,6 @@ def raise_compile_error(*args, **kwargs):  # type: ignore
     pip_no_compile_c_env_vars = {}  # type: Dict[str, Any]
 else:
     # posix
-    # On posix you can start python in a subprocess with no environment
-    # variables and it will run normally.
-    subprocess_python_base_environ = {}
-
     # On posix systems setuptools/distutils uses the CC env variable to
     # locate a C compiler for building C extensions. All we need to do is set
     # it to /var/false, and the module building process will fail to build.

chalice/deploy/packager.py

@@ -8,9 +8,8 @@
 from zipfile import ZipFile  # noqa
 
 from typing import Any, Set, List, Optional, Tuple, Iterable, Callable  # noqa
-from typing import Dict  # noqa
+from typing import Dict, MutableMapping  # noqa
 from chalice.compat import lambda_abi
-from chalice.compat import subprocess_python_base_environ
 from chalice.compat import pip_no_compile_c_env_vars
 from chalice.compat import pip_no_compile_c_shim
 from chalice.utils import OSUtils
@@ -23,6 +22,7 @@
 
 StrMap = Dict[str, Any]
 OptStrMap = Optional[StrMap]
+EnvVars = MutableMapping
 OptStr = Optional[str]
 OptBytes = Optional[bytes]
 
@@ -263,7 +263,7 @@ def __init__(self, osutils, pip_runner=None):
         # type: (OSUtils, Optional[PipRunner]) -> None
         self._osutils = osutils
         if pip_runner is None:
-            pip_runner = PipRunner(SubprocessPip())
+            pip_runner = PipRunner(SubprocessPip(osutils))
         self._pip = pip_runner
 
     def _is_compatible_wheel_filename(self, filename):
@@ -576,13 +576,18 @@ def get_package_name_and_version(self, sdist_path):
 
 class SubprocessPip(object):
     """Wrapper around calling pip through a subprocess."""
+    def __init__(self, osutils=None):
+        # type: (Optional[OSUtils]) -> None
+        if osutils is None:
+            osutils = OSUtils()
+        self._osutils = osutils
+
     def main(self, args, env_vars=None, shim=None):
-        # type: (List[str], OptStrMap, OptStr) -> Tuple[int, Optional[bytes]]
+        # type: (List[str], EnvVars, OptStr) -> Tuple[int, Optional[bytes]]
         if env_vars is None:
-            env_vars = {}
+            env_vars = self._osutils.environ()
         if shim is None:
             shim = ''
-        env_vars.update(subprocess_python_base_environ)
         python_exe = sys.executable
         run_pip = 'import pip, sys; sys.exit(pip.main(%s))' % args
         exec_string = '%s%s' % (shim, run_pip)
@@ -597,12 +602,15 @@ def main(self, args, env_vars=None, shim=None):
 
 class PipRunner(object):
     """Wrapper around pip calls used by chalice."""
-    def __init__(self, pip):
-        # type: (SubprocessPip) -> None
+    def __init__(self, pip, osutils=None):
+        # type: (SubprocessPip, Optional[OSUtils]) -> None
+        if osutils is None:
+            osutils = OSUtils()
         self._wrapped_pip = pip
+        self._osutils = osutils
 
     def _execute(self, command, args, env_vars=None, shim=None):
-        # type: (str, List[str], OptStrMap, OptStr) -> Tuple[int, OptBytes]
+        # type: (str, List[str], EnvVars, OptStr) -> Tuple[int, OptBytes]
         """Execute a pip command with the given arguments."""
         main_args = [command] + args
         rc, err = self._wrapped_pip.main(main_args, env_vars=env_vars,
@@ -613,7 +621,7 @@ def build_wheel(self, wheel, directory, compile_c=True):
         # type: (str, str, bool) -> None
         """Build an sdist into a wheel file."""
         arguments = ['--no-deps', '--wheel-dir', directory, wheel]
-        env_vars = {}  # type: StrMap
+        env_vars = self._osutils.environ()
         shim = ''
         if not compile_c:
             env_vars.update(pip_no_compile_c_env_vars)

chalice/utils.py

@@ -10,6 +10,7 @@
 import click
 from typing import IO, Dict, List, Any, Tuple, Iterator, BinaryIO  # noqa
 from typing import Optional  # noqa
+from typing import MutableMapping  # noqa
 
 from chalice.constants import WELCOME_PROMPT
 
@@ -78,6 +79,10 @@ def create_zip_file(source_dir, outfile):
 class OSUtils(object):
     ZIP_DEFLATED = zipfile.ZIP_DEFLATED
 
+    def environ(self):
+        # type: () -> MutableMapping
+        return os.environ
+
     def open(self, filename, mode):
         # type: (str, str) -> IO
         return open(filename, mode)

tests/functional/test_package.py

@@ -54,6 +54,7 @@ def __init__(self):
         self._side_effects = defaultdict(lambda: [])
 
     def main(self, args, env_vars=None, shim=None):
+
         cmd, args = args[0], args[1:]
         self._calls[cmd].append((args, env_vars, shim))
         try:
@@ -162,9 +163,17 @@ def osutils():
 
 
 @pytest.fixture
-def pip_runner():
+def empty_env_osutils():
+    class EmptyEnv(object):
+        def environ(self):
+            return {}
+    return EmptyEnv()
+
+
+@pytest.fixture
+def pip_runner(empty_env_osutils):
     pip = FakePip()
-    pip_runner = PipRunner(pip)
+    pip_runner = PipRunner(pip, osutils=empty_env_osutils)
     return pip, pip_runner
 
 

tests/unit/deploy/test_packager.py

@@ -39,10 +39,29 @@ def calls(self):
 
 
 @pytest.fixture
-def pip_runner():
-    pip = FakePip()
-    pip_runner = PipRunner(pip)
-    return pip, pip_runner
+def pip_factory():
+    def create_pip_runner(osutils=None):
+        pip = FakePip()
+        pip_runner = PipRunner(pip, osutils=osutils)
+        return pip, pip_runner
+    return create_pip_runner
+
+
+class CustomEnv(OSUtils):
+    def __init__(self, env):
+        self._env = env
+
+    def environ(self):
+        return self._env
+
+
+@pytest.fixture
+def pip_runner_custom_env(custom_env_osutils):
+    def create_pip_runner(env):
+        pip = FakePip()
+        pip_runner = PipRunner(pip, osutils=custom_env_osutils(env))
+        return pip, pip_runner
+    return create_pip_runner
 
 
 @pytest.fixture
@@ -129,9 +148,20 @@ def test_does_error_code_propagate(self):
 
 
 class TestPipRunner(object):
-    def test_build_wheel(self, pip_runner):
+    def test_does_propagate_env_vars(self, pip_factory):
+        osutils = CustomEnv({'foo': 'bar'})
+        pip, runner = pip_factory(osutils)
+        wheel = 'foobar-1.2-py3-none-any.whl'
+        directory = 'directory'
+        runner.build_wheel(wheel, directory)
+        call = pip.calls[0]
+
+        assert 'foo' in call.env_vars
+        assert call.env_vars['foo'] == 'bar'
+
+    def test_build_wheel(self, pip_factory):
         # Test that `pip wheel` is called with the correct params
-        pip, runner = pip_runner
+        pip, runner = pip_factory()
         wheel = 'foobar-1.0-py3-none-any.whl'
         directory = 'directory'
         runner.build_wheel(wheel, directory)
@@ -140,13 +170,14 @@ def test_build_wheel(self, pip_runner):
         call = pip.calls[0]
         assert call.args == ['wheel', '--no-deps', '--wheel-dir',
                              directory, wheel]
-        assert call.env_vars == {}
+        for compile_env_var in pip_no_compile_c_env_vars:
+            assert compile_env_var not in call.env_vars
         assert call.shim == ''
 
-    def test_build_wheel_without_c_extensions(self, pip_runner):
+    def test_build_wheel_without_c_extensions(self, pip_factory):
         # Test that `pip wheel` is called with the correct params when we
         # call it with compile_c=False. These will differ by platform.
-        pip, runner = pip_runner
+        pip, runner = pip_factory()
         wheel = 'foobar-1.0-py3-none-any.whl'
         directory = 'directory'
         runner.build_wheel(wheel, directory, compile_c=False)
@@ -155,13 +186,14 @@ def test_build_wheel_without_c_extensions(self, pip_runner):
         call = pip.calls[0]
         assert call.args == ['wheel', '--no-deps', '--wheel-dir',
                              directory, wheel]
-        assert call.env_vars == pip_no_compile_c_env_vars
+        for compile_env_var in pip_no_compile_c_env_vars:
+            assert compile_env_var in call.env_vars
         assert call.shim == pip_no_compile_c_shim
 
-    def test_download_all_deps(self, pip_runner):
+    def test_download_all_deps(self, pip_factory):
         # Make sure that `pip download` is called with the correct arguments
         # for getting all sdists.
-        pip, runner = pip_runner
+        pip, runner = pip_factory()
         runner.download_all_dependencies('requirements.txt', 'directory')
 
         assert len(pip.calls) == 1
@@ -171,10 +203,10 @@ def test_download_all_deps(self, pip_runner):
         assert call.env_vars is None
         assert call.shim is None
 
-    def test_download_wheels(self, pip_runner):
+    def test_download_wheels(self, pip_factory):
         # Make sure that `pip download` is called with the correct arguments
         # for getting lambda compatible wheels.
-        pip, runner = pip_runner
+        pip, runner = pip_factory()
         packages = ['foo', 'bar', 'baz']
         runner.download_manylinux_wheels(packages, 'directory')
         if sys.version_info[0] == 2:
@@ -190,29 +222,29 @@ def test_download_wheels(self, pip_runner):
             assert pip.calls[i].env_vars is None
             assert pip.calls[i].shim is None
 
-    def test_download_wheels_no_wheels(self, pip_runner):
-        pip, runner = pip_runner
+    def test_download_wheels_no_wheels(self, pip_factory):
+        pip, runner = pip_factory()
         runner.download_manylinux_wheels([], 'directory')
         assert len(pip.calls) == 0
 
-    def test_raise_no_such_package_error(self, pip_runner):
-        pip, runner = pip_runner
+    def test_raise_no_such_package_error(self, pip_factory):
+        pip, runner = pip_factory()
         pip.add_return((1, (b'Could not find a version that satisfies the '
                             b'requirement BadPackageName ')))
         with pytest.raises(NoSuchPackageError) as einfo:
             runner.download_all_dependencies('requirements.txt', 'directory')
         assert str(einfo.value) == ('Could not satisfy the requirement: '
                                     'BadPackageName')
 
-    def test_raise_other_unknown_error_during_downloads(self, pip_runner):
-        pip, runner = pip_runner
+    def test_raise_other_unknown_error_during_downloads(self, pip_factory):
+        pip, runner = pip_factory()
         pip.add_return((1, b'SomeNetworkingError: Details here.'))
         with pytest.raises(PackageDownloadError) as einfo:
             runner.download_all_dependencies('requirements.txt', 'directory')
         assert str(einfo.value) == 'SomeNetworkingError: Details here.'
 
-    def test_inject_unknown_error_if_no_stderr(self, pip_runner):
-        pip, runner = pip_runner
+    def test_inject_unknown_error_if_no_stderr(self, pip_factory):
+        pip, runner = pip_factory()
         pip.add_return((1, None))
         with pytest.raises(PackageDownloadError) as einfo:
             runner.download_all_dependencies('requirements.txt', 'directory')