Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions samcli/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@
"""

from samcli.cli.main import cli # pragma: no cover
from samcli.lib.utils.hook_script import run_hook_script_if_requested # pragma: no cover

if __name__ == "__main__": # pragma: no cover
# A bundle runs cookiecutter's Python hooks by re-launching itself, so claim those invocations
# before the CLI treats the script path as a command name.
run_hook_script_if_requested()
# NOTE(TheSriram): prog_name is always set to "sam". This way when the CLI is invoked as a module,
# the help text that is generated still says "sam" instead of "__main__".
cli(prog_name="sam")
17 changes: 10 additions & 7 deletions samcli/lib/cookiecutter/template.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import logging
from typing import Dict, List, Optional

from cookiecutter import hooks as cookiecutter_hooks
from cookiecutter.exceptions import RepositoryNotFound, UnknownRepoType
from cookiecutter.main import cookiecutter

Expand All @@ -20,6 +21,7 @@
from samcli.lib.cookiecutter.plugin import Plugin
from samcli.lib.cookiecutter.processor import Processor
from samcli.lib.init.arbitrary_project import generate_non_cookiecutter_project
from samcli.lib.utils.hook_script import patched_hook_runner

LOG = logging.getLogger(__name__)

Expand Down Expand Up @@ -167,13 +169,14 @@ def generate_project(self, context: Dict, output_dir: str) -> None:

try:
LOG.debug("Baking a new template with cookiecutter with all parameters")
cookiecutter(
template=self._location,
output_dir=output_dir,
no_input=True,
extra_context=context,
overwrite_if_exists=True,
)
with patched_hook_runner(cookiecutter_hooks):
cookiecutter(
template=self._location,
output_dir=output_dir,
no_input=True,
extra_context=context,
overwrite_if_exists=True,
)
except RepositoryNotFound:
# cookiecutter.json is not found in the template. Let's just clone it directly without
# using cookiecutter and call it done.
Expand Down
5 changes: 4 additions & 1 deletion samcli/lib/init/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from pathlib import Path
from typing import Dict, Optional

from cookiecutter import hooks as cookiecutter_hooks
from cookiecutter.exceptions import CookiecutterException, RepositoryNotFound, UnknownRepoType
from cookiecutter.main import cookiecutter

Expand All @@ -22,6 +23,7 @@
from samcli.lib.init.template_modifiers.xray_tracing_template_modifier import XRayTracingTemplateModifier
from samcli.lib.telemetry.event import EventName, EventTracker, UsedFeature
from samcli.lib.utils import osutils
from samcli.lib.utils.hook_script import patched_hook_runner
from samcli.lib.utils.packagetype import ZIP
from samcli.local.common.runtime_template import RUNTIME_DEP_TEMPLATE_MAPPING, is_custom_runtime

Expand Down Expand Up @@ -119,7 +121,8 @@ def generate_project(
LOG.debug("Baking a new template with cookiecutter with all parameters")
# cookiecutter returns the directory it created, which is the only reliable way to know
# where the project landed when the template chooses its own project directory name.
project_directory = cookiecutter(**params)
with patched_hook_runner(cookiecutter_hooks):
Comment thread
roger-zhangg marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] The two call sites this fix depends on have no test guarding them, and the failure mode is silent.

patched_hook_runner() is a no-op unless is_pyinstaller_bundle() is true, so every existing unit test in tests/unit/lib/init/test_init.py and tests/unit/lib/cookiecutter/test_template.py passes identically whether or not the with wrapper is present. The new tests/unit/lib/utils/test_hook_script.py covers the helper thoroughly but never asserts that anything applies it. That leaves the wiring — the part that actually fixes the bug — unverified at both samcli/lib/init/__init__.py:124 and samcli/lib/cookiecutter/template.py:172.

This is not hypothetical: the wrapper was initially applied to only one of the two cookiecutter() calls, and it took a review pass to catch it. A future refactor that drops or reorders the with block reintroduces the original silent bug (hook skipped, project generated wrong, exit 0) with no failing test, and the bundle path is not exercised by CI — validate_pyinstaller.yml only builds the binary.

A cheap regression guard in each existing test module:

@patch("samcli.lib.init.cookiecutter")
@patch("samcli.lib.init.patched_hook_runner")
def test_hook_runner_is_patched_around_cookiecutter(self, patched_hook_runner_mock, cookiecutter_patch):
   generate_project(location="/path", output_dir=".", name="sam-app")
   patched_hook_runner_mock.assert_called_once_with(cookiecutter_hooks)

An equivalent test against samcli.lib.cookiecutter.template.patched_hook_runner covers the sam pipeline init path. If you want the assertion to also prove ordering (that the patch is active while cookiecutter() runs, not merely entered), have the mock's __enter__ record into a list that the cookiecutter mock's side_effect also appends to.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid, fixed in 8a2fb4b. Your framing of the risk was right and the evidence you cite is fair — the missed second call site was caught by review, not by a test.

Added a guard to each existing module: test_hook_runner_is_active_while_cookiecutter_runs in tests/unit/lib/init/test_init.py and in tests/unit/lib/cookiecutter/test_template.py. I took the stronger ordering variant you suggested rather than a bare assert_called_once_with, so it proves the patch is active while cookiecutter() runs: __enter__, __exit__ and the cookiecutter mock all append to one list, asserted as ["enter", "cookiecutter", "exit"].

Confirmed the guards actually guard, by simulating the refactor you describe and deleting the with block at both call sites:

FAILED tests/unit/lib/init/test_init.py::TestInit::test_hook_runner_is_active_while_cookiecutter_runs
FAILED tests/unit/lib/cookiecutter/test_template.py::TestTemplate::test_hook_runner_is_active_while_cookiecutter_runs
2 failed, 20 passed

The 20 passed is your point restated: every other test in those modules is indifferent to the wrapper being present.

You are also right that CI never exercises the bundle path — validate_pyinstaller.yml only builds the binary. I verified the two interpreter paths locally against a simulated bundle instead, but that is not a substitute for CI coverage, and closing that gap is worth its own issue.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to the reply above: the commit is a7b00a0fc, not 8a2fb4b.

project_directory = cookiecutter(**params)
# Fixes gradlew line ending issue caused by Windows git
# gradlew is a shell script which should not have CR LF line endings
# Putting the conversion after cookiecutter as cookiecutter processing will also change the line endings
Expand Down
127 changes: 127 additions & 0 deletions samcli/lib/utils/hook_script.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
"""
Support for running cookiecutter template hooks from a PyInstaller bundle.

Cookiecutter runs a Python hook as ``[sys.executable, script]``. In a bundle ``sys.executable`` is
the sam executable itself, so the hook never runs. This module supplies a real interpreter: a system
python3 when one is available, otherwise this executable re-launched in hook mode.
"""

import logging
import os
import runpy
import shutil
import subprocess
import sys
from contextlib import contextmanager
from types import ModuleType
from typing import Iterator, Optional

from samcli.lib.utils.subprocess_utils import is_pyinstaller_bundle, isolate_library_paths_for_subprocess

LOG = logging.getLogger(__name__)

# Set on the hook subprocess so a re-launched bundle runs the script instead of parsing a command name.
HOOK_SCRIPT_ENV_VAR = "SAM_CLI_RUN_HOOK_SCRIPT"

# A bundle ships no interpreter of its own, so a system one is preferred: it gives hooks the same

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] The rationale for preferring a system interpreter is inverted, and the resulting order is the one that diverges most from a pip install.

The comment says a system python "gives hooks the same environment they get from a pip install, rather than this bundle's Python and dependencies." Under a pip install, sys.executable is the interpreter that has SAM CLI's dependencies importable — PyYAML~=6.0 is a direct dependency (pyproject.toml:41), and jinja2 comes in with cookiecutter. So a hook containing import yaml or import jinja2 succeeds today for pip users, and would also succeed on the re-launch fallback (which runs under the bundle's sys.path), but fails against a bare /usr/bin/python3 — the path this code prefers.

Net effect: the preferred branch is the only one of the three that cannot import SAM's dependencies, and the failure surfaces as a ModuleNotFoundError inside a FailedHookException for custom --location templates. Either make the re-launch path the primary and the system interpreter the fallback, or keep the current order and correct the comment to say the system interpreter is preferred for isolation, not fidelity, so the next reader does not rely on a guarantee that is not there.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, and the comment was mine to get wrong — fixed in c137500.

Verified the premise: PyYAML~=6.0 is a direct dependency (pyproject.toml:41) and jinja2 arrives with cookiecutter, so under a pip install sys.executable is an interpreter that can import them, and the re-launch path can too. A bare /usr/bin/python3 cannot. So the branch I called the faithful one is the only one of the three that is not.

I took your second option and kept the order, correcting the rationale to say the system interpreter is preferred for isolation — so SAM's bundled dependency set does not quietly become an implicit contract for template authors — rather than for fidelity.

I did not flip the order, for two reasons. Making the bundle primary would promise that hooks can import SAM's dependencies, which is incidental under pip rather than a contract, and would make a PyYAML major bump able to break someone's template. It would also promote the arbitrary-file execution path from rarely-used fallback to the common case. That said, the ordering was chosen by the PR author on the strength of the rationale you just corrected, so I have flagged the trade-off to them; if they would rather have fidelity than isolation, flipping is a one-line change.

# environment they get from a pip install, rather than this bundle's Python and dependencies.
_INTERPRETER_CANDIDATES = ("python3", "python")
Comment thread
roger-zhangg marked this conversation as resolved.
Outdated
_PROBE_TIMEOUT = 10


def run_hook_script_if_requested() -> None:
"""Run the script named in argv and exit, when re-launched by the patched hook runner."""
# Popped rather than read so a hook that shells out to sam again gets the normal CLI.
requested = os.environ.pop(HOOK_SCRIPT_ENV_VAR, None) == "1"
arguments = sys.argv[1:]
if not requested or not arguments:
return

LOG.debug("Running template hook script %s through this executable", arguments[0])
# The bootloader re-points library paths into the bundle for this process, and the CLI callback
# that normally undoes that is never reached here. Hooks routinely shell out to git, npm and pip.
isolate_library_paths_for_subprocess()
runpy.run_path(arguments[0], run_name="__main__")
Comment thread
roger-zhangg marked this conversation as resolved.
Outdated
Comment thread
roger-zhangg marked this conversation as resolved.
Outdated
sys.exit(0)


def find_system_interpreter() -> Optional[str]:
"""Return the path to a usable system Python 3, or None if there isn't one."""
for candidate in _INTERPRETER_CANDIDATES:
path = shutil.which(candidate)
if not path or os.path.realpath(path) == os.path.realpath(sys.executable):
continue
# Executed rather than trusted because Windows ships a "python" App Execution Alias that
# resolves on PATH without being an interpreter.
try:
completed = subprocess.run(
[path, "-c", "import sys; sys.exit(0 if sys.version_info[0] == 3 else 1)"],
Comment thread
roger-zhangg marked this conversation as resolved.
Outdated
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
timeout=_PROBE_TIMEOUT,
check=False,
)
except (OSError, subprocess.SubprocessError):
continue
if completed.returncode == 0:
return path
return None


@contextmanager
def _replaced_attribute(target: object, name: str, value: object) -> Iterator[None]:
"""Set an attribute for the duration of the block, restoring whatever was there before."""
original = getattr(target, name)
setattr(target, name, value)
try:
yield
finally:
setattr(target, name, original)


@contextmanager
def _hook_script_env() -> Iterator[None]:
"""Mark the environment so the re-launched executable runs the hook script."""
original = os.environ.get(HOOK_SCRIPT_ENV_VAR)
os.environ[HOOK_SCRIPT_ENV_VAR] = "1"
try:
yield
finally:
if original is None:
os.environ.pop(HOOK_SCRIPT_ENV_VAR, None)
else:
os.environ[HOOK_SCRIPT_ENV_VAR] = original


@contextmanager
def patched_hook_runner(hooks_module: ModuleType) -> Iterator[None]:
"""Make cookiecutter's Python hooks runnable while frozen; a no-op when not frozen.

The module is passed in so this stays importable without pulling in cookiecutter, which every
sam invocation would otherwise pay for at startup.
"""
if not is_pyinstaller_bundle():
yield
return

original_run_script = hooks_module.run_script

def run_script(script_path: str, cwd: str = ".") -> None:
# Only .py hooks go through an interpreter; anything else already runs on its own.
if not script_path.endswith(".py"):
original_run_script(script_path, cwd)
return

interpreter = find_system_interpreter()
if interpreter:
LOG.debug("Running template hook with system interpreter %s", interpreter)
with _replaced_attribute(sys, "executable", interpreter):
original_run_script(script_path, cwd)
return

LOG.debug("No system interpreter found, re-launching this executable to run the template hook")
with _hook_script_env():
original_run_script(script_path, cwd)

with _replaced_attribute(hooks_module, "run_script", run_script):
yield
2 changes: 1 addition & 1 deletion tests/integration/init/test_init_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,7 @@ def _assert_template_with_cfn_lint(self, cwd):
You can run 'sam init' without any options for an interactive initialization flow, or you can provide one of the following required parameter combinations:
\t--name, --location, or
\t--name, --package-type, --base-image, or
\t--name, --runtime, --app-template, --dependency-manager
\t--name, --runtime, --dependency-manager, --app-template
"""


Expand Down
172 changes: 172 additions & 0 deletions tests/unit/lib/utils/test_hook_script.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
import inspect
import os
import sys
from types import SimpleNamespace
from unittest import TestCase
from unittest.mock import Mock, patch

from cookiecutter import hooks as cookiecutter_hooks

from samcli.lib.utils.hook_script import (
HOOK_SCRIPT_ENV_VAR,
find_system_interpreter,
patched_hook_runner,
run_hook_script_if_requested,
)


def _hooks_module(recorder):
"""A stand-in for cookiecutter.hooks whose run_script records the interpreter it would use."""

def run_script(script_path, cwd="."):
recorder.append((sys.executable, os.environ.get(HOOK_SCRIPT_ENV_VAR)))

return SimpleNamespace(run_script=run_script)


class TestPatchedHookRunner(TestCase):
@patch("samcli.lib.utils.hook_script.is_pyinstaller_bundle", return_value=False)
def test_not_patched_when_not_frozen(self, patched_bundle):
module = _hooks_module([])
original = module.run_script
with patched_hook_runner(module):
self.assertIs(module.run_script, original)

@patch("samcli.lib.utils.hook_script.is_pyinstaller_bundle", return_value=True)
def test_restores_run_script_afterwards(self, patched_bundle):
module = _hooks_module([])
original = module.run_script
with patched_hook_runner(module):
self.assertIsNot(module.run_script, original)
self.assertIs(module.run_script, original)

@patch("samcli.lib.utils.hook_script.is_pyinstaller_bundle", return_value=True)
def test_restores_run_script_when_body_raises(self, patched_bundle):
module = _hooks_module([])
original = module.run_script
with self.assertRaises(ValueError):
with patched_hook_runner(module):
raise ValueError("boom")
self.assertIs(module.run_script, original)

@patch("samcli.lib.utils.hook_script.find_system_interpreter", return_value="/usr/bin/python3")
@patch("samcli.lib.utils.hook_script.is_pyinstaller_bundle", return_value=True)
def test_system_interpreter_used_for_python_hook(self, patched_bundle, patched_find):
calls = []
module = _hooks_module(calls)
original_executable = sys.executable
with patched_hook_runner(module):
module.run_script("post_gen_project.py")
self.assertEqual(calls, [("/usr/bin/python3", None)])
self.assertEqual(sys.executable, original_executable)

@patch("samcli.lib.utils.hook_script.find_system_interpreter", return_value=None)
@patch("samcli.lib.utils.hook_script.is_pyinstaller_bundle", return_value=True)
def test_falls_back_to_relaunching_this_executable(self, patched_bundle, patched_find):
calls = []
module = _hooks_module(calls)
with patched_hook_runner(module):
module.run_script("post_gen_project.py")
# The executable is left alone; the env var tells the re-launched bundle to run the script.
self.assertEqual(calls, [(sys.executable, "1")])
self.assertNotIn(HOOK_SCRIPT_ENV_VAR, os.environ)

@patch("samcli.lib.utils.hook_script.find_system_interpreter")
@patch("samcli.lib.utils.hook_script.is_pyinstaller_bundle", return_value=True)
def test_non_python_hook_is_delegated_untouched(self, patched_bundle, patched_find):
calls = []
module = _hooks_module(calls)
with patched_hook_runner(module):
module.run_script("post_gen_project.sh")
self.assertEqual(calls, [(sys.executable, None)])
patched_find.assert_not_called()


class TestFindSystemInterpreter(TestCase):
@patch("samcli.lib.utils.hook_script.shutil.which", return_value=None)
def test_returns_none_when_nothing_on_path(self, patched_which):
self.assertIsNone(find_system_interpreter())

@patch("samcli.lib.utils.hook_script.subprocess.run")
@patch("samcli.lib.utils.hook_script.shutil.which")
def test_skips_candidate_that_is_this_executable(self, patched_which, patched_run):
patched_which.return_value = sys.executable
self.assertIsNone(find_system_interpreter())
patched_run.assert_not_called()

@patch("samcli.lib.utils.hook_script.subprocess.run", return_value=Mock(returncode=1))
@patch("samcli.lib.utils.hook_script.shutil.which", return_value="/fake/python3")
def test_skips_candidate_that_is_not_an_interpreter(self, patched_which, patched_run):
self.assertIsNone(find_system_interpreter())

@patch("samcli.lib.utils.hook_script.subprocess.run", return_value=Mock(returncode=0))
@patch("samcli.lib.utils.hook_script.shutil.which", return_value="/fake/python3")
def test_returns_usable_interpreter(self, patched_which, patched_run):
self.assertEqual(find_system_interpreter(), "/fake/python3")

@patch("samcli.lib.utils.hook_script.subprocess.run", side_effect=OSError("nope"))
@patch("samcli.lib.utils.hook_script.shutil.which", return_value="/fake/python3")
def test_skips_candidate_that_cannot_be_executed(self, patched_which, patched_run):
self.assertIsNone(find_system_interpreter())


class TestRunHookScriptIfRequested(TestCase):
@patch("samcli.lib.utils.hook_script.runpy.run_path")
def test_no_op_without_env_var(self, patched_run_path):
os.environ.pop(HOOK_SCRIPT_ENV_VAR, None)
run_hook_script_if_requested()
patched_run_path.assert_not_called()

@patch("samcli.lib.utils.hook_script.runpy.run_path")
def test_runs_script_and_exits(self, patched_run_path):
with patch.dict(os.environ, {HOOK_SCRIPT_ENV_VAR: "1"}):
with patch.object(sys, "argv", ["sam", "/tmp/hook.py"]):
with self.assertRaises(SystemExit) as context:
run_hook_script_if_requested()
self.assertEqual(context.exception.code, 0)
patched_run_path.assert_called_once_with("/tmp/hook.py", run_name="__main__")

@patch("samcli.lib.utils.hook_script.runpy.run_path")
def test_env_var_removed_so_nested_sam_calls_get_the_cli(self, patched_run_path):
with patch.dict(os.environ, {HOOK_SCRIPT_ENV_VAR: "1"}):
with patch.object(sys, "argv", ["sam", "/tmp/hook.py"]):
with self.assertRaises(SystemExit):
run_hook_script_if_requested()
self.assertNotIn(HOOK_SCRIPT_ENV_VAR, os.environ)

def test_library_paths_isolated_before_the_hook_runs(self):
# The bootloader re-points library paths into the bundle for this process, and the CLI
# callback that normally undoes it never runs on this path.
order = []
with patch(
"samcli.lib.utils.hook_script.isolate_library_paths_for_subprocess",
side_effect=lambda: order.append("isolate"),
):
with patch("samcli.lib.utils.hook_script.runpy.run_path", side_effect=lambda *a, **k: order.append("run")):
with patch.dict(os.environ, {HOOK_SCRIPT_ENV_VAR: "1"}):
with patch.object(sys, "argv", ["sam", "/tmp/hook.py"]):
with self.assertRaises(SystemExit):
run_hook_script_if_requested()
self.assertEqual(order, ["isolate", "run"])

@patch("samcli.lib.utils.hook_script.isolate_library_paths_for_subprocess")
@patch("samcli.lib.utils.hook_script.runpy.run_path")
def test_no_op_without_a_script_argument(self, patched_run_path, patched_isolate):
with patch.dict(os.environ, {HOOK_SCRIPT_ENV_VAR: "1"}):
with patch.object(sys, "argv", ["sam"]):
run_hook_script_if_requested()
patched_run_path.assert_not_called()


class TestCookiecutterPatchTarget(TestCase):
"""Guards the seam: a cookiecutter bump inside the pin must not silently un-patch us."""

def test_run_script_exists_with_expected_signature(self):
self.assertTrue(callable(cookiecutter_hooks.run_script))
parameters = list(inspect.signature(cookiecutter_hooks.run_script).parameters)
self.assertEqual(parameters, ["script_path", "cwd"])

def test_run_script_still_uses_sys_executable(self):
# The whole fix rests on run_script reading sys.executable at call time.
source = inspect.getsource(cookiecutter_hooks.run_script)
self.assertIn("sys.executable", source)