Fix: Pass a copy of environment variables when building layers

samcli/lib/build/build_graph.py

@@ -3,6 +3,7 @@
 """
 
 import logging
+from copy import deepcopy
 from pathlib import Path
 from typing import Tuple, List, Any, Optional, Dict, cast
 from uuid import uuid4
@@ -323,9 +324,14 @@ class AbstractBuildDefinition:
     Build definition holds information about each unique build
     """
 
-    def __init__(self, source_md5: str) -> None:
+    def __init__(self, source_md5: str, env_vars: Optional[Dict] = None) -> None:
         self.uuid = str(uuid4())
         self.source_md5 = source_md5
+        self._env_vars = env_vars if env_vars else {}
+
+    @property
+    def env_vars(self) -> Dict:
+        return deepcopy(self._env_vars)
 
 
 class LayerBuildDefinition(AbstractBuildDefinition):
@@ -342,12 +348,11 @@ def __init__(
         source_md5: str = "",
         env_vars: Optional[Dict] = None,
     ):
-        super().__init__(source_md5)
+        super().__init__(source_md5, env_vars)
         self.name = name
         self.codeuri = codeuri
         self.build_method = build_method
         self.compatible_runtimes = compatible_runtimes
-        self.env_vars = env_vars if env_vars else {}
         # Note(xinhol): In our code, we assume "layer" is never None. We should refactor
         # this and move "layer" out of LayerBuildDefinition to take advantage of type check.
         self.layer: LayerVersion = None  # type: ignore
@@ -398,12 +403,11 @@ def __init__(
         source_md5: str = "",
         env_vars: Optional[Dict] = None,
     ) -> None:
-        super().__init__(source_md5)
+        super().__init__(source_md5, env_vars)
         self.runtime = runtime
         self.codeuri = codeuri
         self.packagetype = packagetype
         self.metadata = metadata if metadata else {}
-        self.env_vars = env_vars if env_vars else {}
         self.functions: List[Function] = []
 
     def add_function(self, function: Function) -> None:

samcli/lib/build/build_strategy.py

@@ -5,7 +5,6 @@
 import pathlib
 import shutil
 from abc import abstractmethod, ABC
-from copy import deepcopy
 from typing import Callable, Dict, List, Any, Optional, cast
 
 from samcli.commands.build.exceptions import MissingBuildMethodException
@@ -115,10 +114,6 @@ def build_single_function_definition(self, build_definition: FunctionBuildDefini
 
         LOG.debug("Building to following folder %s", single_build_dir)
 
-        # we should create a copy and pass it down, otherwise additional env vars like LAMBDA_BUILDERS_LOG_LEVEL
-        # will make cache invalid all the time
-        container_env_vars = deepcopy(build_definition.env_vars)
-
         # when a function is passed here, it is ZIP function, codeuri and runtime are not None
         result = self._build_function(
             build_definition.get_function_name(),
@@ -128,7 +123,7 @@ def build_single_function_definition(self, build_definition: FunctionBuildDefini
             build_definition.get_handler_name(),
             single_build_dir,
             build_definition.metadata,
-            container_env_vars,
+            build_definition.env_vars,
         )
         function_build_results[single_full_path] = result
 

tests/integration/buildcmd/test_build_cmd.py

@@ -1349,35 +1349,50 @@ def test_cache_build(self, use_container, code_uri, function1_handler, function2
                 expected_messages, command_result, self._make_parameter_override_arg(overrides)
             )
 
+
+@skipIf(
+    ((IS_WINDOWS and RUNNING_ON_CI) and not CI_OVERRIDE),
+    "Skip build tests on windows when running in CI unless overridden",
+)
+class TestRepeatedBuildHitsCache(BuildIntegBase):
+    # Use template containing both functions and layers
+    template = "layers-functions-template.yaml"
+
+    @parameterized.expand([(True,), (False,)])
     @skipIf(SKIP_DOCKER_TESTS, SKIP_DOCKER_MESSAGE)
-    def test_cached_build_with_env_vars(self):
+    def test_repeated_cached_build_hits_cache(self, use_container):
         """
         Build 2 times to verify that second time hits the cached build
         """
-        overrides = {
-            "FunctionCodeUri": "Python",
-            "Function1Handler": "main.first_function_handler",
-            "Function2Handler": "main.second_function_handler",
-            "FunctionRuntime": "python3.8",
+
+        parameter_overrides = {
+            "LayerContentUri": "PyLayer",
+            "LayerBuildMethod": "python3.7",
+            "CodeUri": "Python",
+            "Handler": "main.handler",
+            "Runtime": "python3.7",
+            "LayerMakeContentUri": "PyLayerMake",
         }
+
         cmdlist = self.get_command_list(
-            use_container=True, parameter_overrides=overrides, cached=True, container_env_var="FOO=BAR"
+            use_container=use_container,
+            parameter_overrides=parameter_overrides,
+            cached=True,
+            container_env_var="FOO=BAR" if use_container else None,
         )
 
+        cache_invalid_output = "Cache is invalid, running build and copying resources to "
+        cache_valid_output = "Valid cache found, copying previously built resources from "
+
         LOG.info("Running Command (cache should be invalid): %s", cmdlist)
-        command_result = run_command(cmdlist, cwd=self.working_dir)
-        self.assertTrue(
-            "Cache is invalid, running build and copying resources to function build definition"
-            in command_result.stderr.decode("utf-8")
-        )
+        command_result = run_command(cmdlist, cwd=self.working_dir).stderr.decode("utf-8")
+        self.assertTrue(cache_invalid_output in command_result)
+        self.assertFalse(cache_valid_output in command_result)
 
         LOG.info("Re-Running Command (valid cache should exist): %s", cmdlist)
-        command_result_with_cache = run_command(cmdlist, cwd=self.working_dir)
-
-        self.assertTrue(
-            "Valid cache found, copying previously built resources from function build definition"
-            in command_result_with_cache.stderr.decode("utf-8")
-        )
+        command_result = run_command(cmdlist, cwd=self.working_dir).stderr.decode("utf-8")
+        self.assertFalse(cache_invalid_output in command_result)
+        self.assertTrue(cache_valid_output in command_result)
 
 
 @skipIf(

tests/unit/lib/build_module/test_build_strategy.py

@@ -1,4 +1,3 @@
-from copy import deepcopy
 from unittest import TestCase
 from unittest.mock import Mock, patch, MagicMock, call, ANY
 
@@ -223,11 +222,7 @@ def test_build_single_function_definition_image_functions_with_same_metadata(sel
         # since they have the same metadata, they are put into the same build_definition.
         build_definition.functions = [function1, function2]
 
-        with patch("samcli.lib.build.build_strategy.deepcopy", wraps=deepcopy) as patched_deepcopy:
-            result = default_build_strategy.build_single_function_definition(build_definition)
-
-            patched_deepcopy.assert_called_with(build_definition.env_vars)
-
+        result = default_build_strategy.build_single_function_definition(build_definition)
         # both of the function name should show up in results
         self.assertEqual(result, {"Function": built_image, "Function2": built_image})