Repository: aws/chalice
Issue: Environment variables do not get passed to subprocess during package building

When Chalice creates subprocesses to build the package it does not pass down the current set of environment variables down. This was introduced in https://github.com/aws/chalice/commit/d6b22b3e8932dd588bf5110d5e5b084d246681a4 so it is a regression over the previous behavior where the subprocess inherited the environment. This matters for any setup.py file that makes use of an environment variable.

Initial issue #497 

Changed files:
tests/functional/test_package.py, chalice/deploy/packager.py, tests/unit/deploy/test_packager.py, chalice/compat.py, chalice/utils.py

diff --git a/chalice/compat.py b/chalice/compat.py
index 1161027..14221a7 100644
--- a/chalice/compat.py
+++ b/chalice/compat.py
@@ -8,17 +8,6 @@ from six import StringIO
 
 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 @@ if os.name == 'nt':
     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.
diff --git a/chalice/deploy/packager.py b/chalice/deploy/packager.py
index 8a8d9c4..6caac0d 100644
--- a/chalice/deploy/packager.py
+++ b/chalice/deploy/packager.py
@@ -8,9 +8,8 @@ from email.message import Message  # noqa
 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 @@ from chalice import app
 
 StrMap = Dict[str, Any]
 OptStrMap = Optional[StrMap]
+EnvVars = MutableMapping
 OptStr = Optional[str]
 OptBytes = Optional[bytes]
 
@@ -263,7 +263,7 @@ class DependencyBuilder(object):
         # 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 @@ class SDistMetadataFetcher(object):
 
 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 @@ class SubprocessPip(object):
 
 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 @@ class PipRunner(object):
         # 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)
diff --git a/chalice/utils.py b/chalice/utils.py
index 8537aba..fc77b92 100644
--- a/chalice/utils.py
+++ b/chalice/utils.py
@@ -10,6 +10,7 @@ import tarfile
 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)
diff --git a/tests/functional/test_package.py b/tests/functional/test_package.py
index a4b476c..f2da879 100644
--- a/tests/functional/test_package.py
+++ b/tests/functional/test_package.py
@@ -54,6 +54,7 @@ class FakePip(object):
         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
 
 
diff --git a/tests/unit/deploy/test_packager.py b/tests/unit/deploy/test_packager.py
index f3c1689..bc2dcfa 100644
--- a/tests/unit/deploy/test_packager.py
+++ b/tests/unit/deploy/test_packager.py
@@ -39,10 +39,29 @@ class FakePip(object):
 
 
 @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 @@ class TestSubprocessPip(object):
 
 
 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 @@ class TestPipRunner(object):
         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 @@ class TestPipRunner(object):
         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 @@ class TestPipRunner(object):
         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,13 +222,13 @@ class TestPipRunner(object):
             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:
@@ -204,15 +236,15 @@ class TestPipRunner(object):
         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')