Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-120678: pyrepl: Include globals from modules passed with -i #120904

Merged
merged 9 commits into from
Jul 17, 2024
Merged
3 changes: 3 additions & 0 deletions Lib/_pyrepl/__main__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
# Important: don't add things to this module, as they will end up in the REPL's
# default globals. Use _pyrepl.main instead.

if __name__ == "__main__":
from .main import interactive_console as __pyrepl_interactive_console
__pyrepl_interactive_console()
6 changes: 5 additions & 1 deletion Lib/test/support/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2614,14 +2614,18 @@ def force_not_colorized(func):
def wrapper(*args, **kwargs):
import _colorize
original_fn = _colorize.can_colorize
variables = {"PYTHON_COLORS": None, "FORCE_COLOR": None}
variables: dict[str, str | None] = {
"PYTHON_COLORS": None, "FORCE_COLOR": None, "NO_COLOR": None
}
try:
for key in variables:
variables[key] = os.environ.pop(key, None)
os.environ["NO_COLOR"] = "1"
_colorize.can_colorize = lambda: False
return func(*args, **kwargs)
finally:
_colorize.can_colorize = original_fn
del os.environ["NO_COLOR"]
for key, value in variables.items():
if value is not None:
os.environ[key] = value
Expand Down
13 changes: 12 additions & 1 deletion Lib/test/test_pyrepl/support.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os
from code import InteractiveConsole
from functools import partial
from typing import Iterable
Expand Down Expand Up @@ -100,8 +101,18 @@ def handle_all_events(
)


def make_clean_env() -> dict[str, str]:
clean_env = os.environ.copy()
for k in clean_env.copy():
if k.startswith("PYTHON"):
clean_env.pop(k)
clean_env.pop("FORCE_COLOR", None)
clean_env.pop("NO_COLOR", None)
return clean_env


class FakeConsole(Console):
def __init__(self, events, encoding="utf-8"):
def __init__(self, events, encoding="utf-8") -> None:
self.events = iter(events)
self.encoding = encoding
self.screen = []
Expand Down
114 changes: 106 additions & 8 deletions Lib/test/test_pyrepl/test_pyrepl.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import itertools
import os
import pathlib
import re
import rlcompleter
import select
import subprocess
Expand All @@ -21,7 +22,8 @@
more_lines,
multiline_input,
code_to_events,
clean_screen
clean_screen,
make_clean_env,
)
from _pyrepl.console import Event
from _pyrepl.readline import ReadlineAlikeReader, ReadlineConfig
Expand Down Expand Up @@ -487,6 +489,18 @@ def prepare_reader(self, events):
reader.can_colorize = False
return reader

def test_stdin_is_tty(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This was helpful to me when debugging weird behavior so I decided to keep it.

# Used during test log analysis to figure out if a TTY was available.
if os.isatty(sys.stdin.fileno()):
return
self.skipTest("stdin is not a tty")

def test_stdout_is_tty(self):
# Used during test log analysis to figure out if a TTY was available.
if os.isatty(sys.stdout.fileno()):
return
self.skipTest("stdout is not a tty")

def test_basic(self):
reader = self.prepare_reader(code_to_events("1+1\n"))

Expand Down Expand Up @@ -888,12 +902,7 @@ def setUp(self):
# Cleanup from PYTHON* variables to isolate from local
# user settings, see #121359. Such variables should be
# added later in test methods to patched os.environ.
clean_env = os.environ.copy()
for k in clean_env.copy():
if k.startswith("PYTHON"):
clean_env.pop(k)

patcher = patch('os.environ', new=clean_env)
patcher = patch('os.environ', new=make_clean_env())
self.addCleanup(patcher.stop)
patcher.start()

Expand All @@ -920,6 +929,84 @@ def test_exposed_globals_in_repl(self):

self.assertTrue(case1 or case2 or case3 or case4, output)

def _assertMatchOK(
self, var: str, expected: str | re.Pattern, actual: str
) -> None:
if isinstance(expected, re.Pattern):
self.assertTrue(
expected.match(actual),
f"{var}={actual} does not match {expected.pattern}",
)
else:
self.assertEqual(
actual,
expected,
f"expected {var}={expected}, got {var}={actual}",
)
ambv marked this conversation as resolved.
Show resolved Hide resolved

@force_not_colorized
def _run_repl_globals_test(self, expectations, *, as_file=False, as_module=False):
clean_env = make_clean_env()
clean_env["NO_COLOR"] = "1" # force_not_colorized doesn't touch subprocesses

with tempfile.TemporaryDirectory() as td:
blue = pathlib.Path(td) / "blue"
blue.mkdir()
mod = blue / "calx.py"
mod.write_text("FOO = 42", encoding="utf-8")
commands = [
"print(f'{" + var + "=}')" for var in expectations
] + ["exit"]
if as_file and as_module:
self.fail("as_file and as_module are mutually exclusive")
elif as_file:
output, exit_code = self.run_repl(
commands,
cmdline_args=[str(mod)],
env=clean_env,
)
elif as_module:
output, exit_code = self.run_repl(
commands,
cmdline_args=["-m", "blue.calx"],
env=clean_env,
cwd=td,
)
else:
self.fail("Choose one of as_file or as_module")

if "can't use pyrepl" in output:
self.skipTest("pyrepl not available")

self.assertEqual(exit_code, 0)
for var, expected in expectations.items():
with self.subTest(var=var, expected=expected):
if m := re.search(rf"[\r\n]{var}=(.+?)[\r\n]", output):
self._assertMatchOK(var, expected, actual=m.group(1))
else:
self.fail(f"{var}= not found in output")

self.assertNotIn("Exception", output)
self.assertNotIn("Traceback", output)

def test_inspect_keeps_globals_from_inspected_file(self):
expectations = {
"FOO": "42",
"__name__": "'__main__'",
"__package__": "None",
# "__file__" is missing in -i, like in the basic REPL
ambv marked this conversation as resolved.
Show resolved Hide resolved
}
self._run_repl_globals_test(expectations, as_file=True)

def test_inspect_keeps_globals_from_inspected_module(self):
expectations = {
"FOO": "42",
"__name__": "'__main__'",
"__package__": "'blue'",
"__file__": re.compile(r"^'.*calx.py'$"),
ambv marked this conversation as resolved.
Show resolved Hide resolved
}
self._run_repl_globals_test(expectations, as_module=True)

def test_dumb_terminal_exits_cleanly(self):
env = os.environ.copy()
env.update({"TERM": "dumb"})
Expand Down Expand Up @@ -981,16 +1068,27 @@ def test_not_wiping_history_file(self):
self.assertIn("spam", output)
self.assertNotEqual(pathlib.Path(hfile.name).stat().st_size, 0)

def run_repl(self, repl_input: str | list[str], env: dict | None = None) -> tuple[str, int]:
def run_repl(
self,
repl_input: str | list[str],
env: dict | None = None,
*,
cmdline_args: list[str] | None = None,
cwd: str | None = None,
) -> tuple[str, int]:
assert pty
master_fd, slave_fd = pty.openpty()
cmd = [sys.executable, "-i", "-u"]
if env is None:
cmd.append("-I")
if cmdline_args is not None:
cmd.extend(cmdline_args)
process = subprocess.Popen(
cmd,
stdin=slave_fd,
stdout=slave_fd,
stderr=slave_fd,
cwd=cwd,
text=True,
close_fds=True,
env=env if env else os.environ,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix regression in the new REPL that meant that globals from files passed
using the ``-i`` argument would not be included in the REPL's global
namespace. Patch by Alex Waygood.
50 changes: 49 additions & 1 deletion Modules/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "pycore_call.h" // _PyObject_CallNoArgs()
#include "pycore_initconfig.h" // _PyArgv
#include "pycore_interp.h" // _PyInterpreterState.sysdict
#include "pycore_long.h" // _PyLong_GetOne()
#include "pycore_pathconfig.h" // _PyPathConfig_ComputeSysPath0()
#include "pycore_pylifecycle.h" // _Py_PreInitializeFromPyArgv()
#include "pycore_pystate.h" // _PyInterpreterState_GET()
Expand Down Expand Up @@ -259,6 +260,53 @@
}


static int
pymain_start_pyrepl_no_main()

Check warning on line 264 in Modules/main.c

View workflow job for this annotation

GitHub Actions / Ubuntu SSL tests with OpenSSL (1.1.1w)

function declaration isn’t a prototype [-Wstrict-prototypes]

Check warning on line 264 in Modules/main.c

View workflow job for this annotation

GitHub Actions / Ubuntu SSL tests with OpenSSL (3.0.13)

function declaration isn’t a prototype [-Wstrict-prototypes]

Check warning on line 264 in Modules/main.c

View workflow job for this annotation

GitHub Actions / Ubuntu SSL tests with OpenSSL (3.2.1)

function declaration isn’t a prototype [-Wstrict-prototypes]

Check warning on line 264 in Modules/main.c

View workflow job for this annotation

GitHub Actions / Ubuntu SSL tests with OpenSSL (3.1.5)

function declaration isn’t a prototype [-Wstrict-prototypes]

Check warning on line 264 in Modules/main.c

View workflow job for this annotation

GitHub Actions / Hypothesis tests on Ubuntu

function declaration isn’t a prototype [-Wstrict-prototypes]

Check warning on line 264 in Modules/main.c

View workflow job for this annotation

GitHub Actions / Ubuntu / build and test

function declaration isn’t a prototype [-Wstrict-prototypes]

Check warning on line 264 in Modules/main.c

View workflow job for this annotation

GitHub Actions / Address sanitizer

function declaration isn’t a prototype [-Wstrict-prototypes]

Check warning on line 264 in Modules/main.c

View workflow job for this annotation

GitHub Actions / Ubuntu (free-threading) / build and test

function declaration isn’t a prototype [-Wstrict-prototypes]
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
{
int res = 0;
PyObject *pyrepl, *console, *empty_tuple, *kwargs, *console_result;
pyrepl = PyImport_ImportModule("_pyrepl.main");
if (pyrepl == NULL) {
fprintf(stderr, "Could not import _pyrepl.main\n");
res = pymain_exit_err_print();
goto done;
}
console = PyObject_GetAttrString(pyrepl, "interactive_console");
if (console == NULL) {
fprintf(stderr, "Could not access _pyrepl.main.interactive_console\n");
res = pymain_exit_err_print();
goto done;
}
empty_tuple = PyTuple_New(0);
if (empty_tuple == NULL) {
res = pymain_exit_err_print();
goto done;
}
kwargs = PyDict_New();
if (kwargs == NULL) {
res = pymain_exit_err_print();
goto done;
}
if (!PyDict_SetItemString(kwargs, "pythonstartup", _PyLong_GetOne())) {
_PyRuntime.signals.unhandled_keyboard_interrupt = 0;
console_result = PyObject_Call(console, empty_tuple, kwargs);
if (!console_result && PyErr_Occurred() == PyExc_KeyboardInterrupt) {
_PyRuntime.signals.unhandled_keyboard_interrupt = 1;
}
if (console_result == NULL) {
res = pymain_exit_err_print();
}
}
done:
Py_XDECREF(console_result);
Py_XDECREF(kwargs);
Py_XDECREF(empty_tuple);
Py_XDECREF(console);
Py_XDECREF(pyrepl);
return res;
}


static int
pymain_run_module(const wchar_t *modname, int set_argv0)
{
Expand Down Expand Up @@ -549,7 +597,7 @@
*exitcode = (run != 0);
return;
}
int run = pymain_run_module(L"_pyrepl", 0);
int run = pymain_start_pyrepl_no_main();
Copy link
Contributor

Choose a reason for hiding this comment

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

My first intuition was to simply PyImport_ImportModule("_pyrepl.__main__") but then the entire shell session is a side-effect of an on-going import, which was ugly and caused some funny edge cases (a non-zero exit was technically an ImportError).

So instead we do the full dance of running _pyrepl.main.interactive_console(). While doing that I'm smuggling PYTHONSTARTUP support because:

  • you can always remove PYTHONSTARTUP from your environment to not have it;
  • but previously you could not get PYTHONSTARTUP to be executed even if you wanted.

*exitcode = (run != 0);
return;
}
Expand Down
Loading