From 3eca15d4e9f3423477ecc4da1d466de22b78c89f Mon Sep 17 00:00:00 2001 From: "John T. Wodder II" Date: Sat, 22 Feb 2020 14:32:56 -0500 Subject: [PATCH 1/7] [#1470,#1476] Use subprocess instead of os.system --- click/_compat.py | 13 ++++++++ click/_termui_impl.py | 71 ++++++++++++++++++++++++++++++++++--------- click/termui.py | 5 +-- tests/test_imports.py | 2 +- 4 files changed, 73 insertions(+), 18 deletions(-) diff --git a/click/_compat.py b/click/_compat.py index 5b5c73dd6..79061c022 100644 --- a/click/_compat.py +++ b/click/_compat.py @@ -258,6 +258,17 @@ def filename_to_ui(value): if isinstance(value, bytes): value = value.decode(get_filesystem_encoding(), 'replace') return value + + def which(cmd): + import subprocess + try: + path = subprocess.check_output( + ['which', cmd], + stderr=subprocess.STDOUT, + ) + except subprocess.CalledProcessError: + return None + return path.strip() else: text_type = str raw_input = input @@ -461,6 +472,8 @@ def filename_to_ui(value): .decode('utf-8', 'replace') return value + from shutil import which + def get_streerror(e, default=None): if hasattr(e, 'strerror'): diff --git a/click/_termui_impl.py b/click/_termui_impl.py index 62eb95d21..2087840f6 100644 --- a/click/_termui_impl.py +++ b/click/_termui_impl.py @@ -18,7 +18,7 @@ import contextlib from ._compat import _default_text_stdout, range_type, isatty, \ open_stream, strip_ansi, term_len, get_best_encoding, WIN, int_types, \ - CYGWIN + CYGWIN, which from .utils import echo from .exceptions import ClickException @@ -333,15 +333,15 @@ def pager(generator, color=None): if os.environ.get('TERM') in ('dumb', 'emacs'): return _nullpager(stdout, generator, color) if WIN or sys.platform.startswith('os2'): - return _tempfilepager(generator, 'more <', color) - if hasattr(os, 'system') and os.system('(less) 2>/dev/null') == 0: + return _pipetempfilepager(generator, 'more', color) + if which('less') is not None: return _pipepager(generator, 'less', color) import tempfile fd, filename = tempfile.mkstemp() os.close(fd) try: - if hasattr(os, 'system') and os.system('more "%s"' % filename) == 0: + if which('more') is not None: return _pipepager(generator, 'more', color) return _nullpager(stdout, generator, color) finally: @@ -399,6 +399,31 @@ def _pipepager(generator, cmd, color): def _tempfilepager(generator, cmd, color): """Page through text by invoking a program on a temporary file.""" + import subprocess + import tempfile + filename = tempfile.mktemp() + # TODO: This never terminates if the passed generator never terminates. + text = "".join(generator) + if not color: + text = strip_ansi(text) + encoding = get_best_encoding(sys.stdout) + with open_stream(filename, 'wb')[0] as f: + f.write(text.encode(encoding)) + try: + subprocess.call([cmd, filename]) + except OSError: + # Command not found + pass + finally: + os.unlink(filename) + + +def _pipetempfilepager(generator, cmd, color): + """ + Page through text by invoking a program with a temporary file redirected + as input. + """ + import subprocess import tempfile filename = tempfile.mktemp() # TODO: This never terminates if the passed generator never terminates. @@ -409,7 +434,11 @@ def _tempfilepager(generator, cmd, color): with open_stream(filename, 'wb')[0] as f: f.write(text.encode(encoding)) try: - os.system(cmd + ' "' + filename + '"') + with open_stream(filename, 'rb')[0] as f: + subprocess.call([cmd], stdin=f) + except OSError: + # Command not found + pass finally: os.unlink(filename) @@ -441,7 +470,7 @@ def get_editor(self): if WIN: return 'notepad' for editor in 'sensible-editor', 'vim', 'nano': - if os.system('which %s >/dev/null 2>&1' % editor) == 0: + if which(editor) is not None: return editor return 'vi' @@ -532,20 +561,32 @@ def _unquote_file(url): elif WIN: if locate: url = _unquote_file(url) - args = 'explorer /select,"%s"' % _unquote_file( - url.replace('"', '')) + args = ['explorer', '/select,%s' % _unquote_file(url)] else: - args = 'start %s "" "%s"' % ( - wait and '/WAIT' or '', url.replace('"', '')) - return os.system(args) + args = ['start'] + if wait: + args.append('/WAIT') + args.append('') + args.append(url) + try: + return subprocess.call(args) + except OSError: + # Command not found + return 127 elif CYGWIN: if locate: url = _unquote_file(url) - args = 'cygstart "%s"' % (os.path.dirname(url).replace('"', '')) + args = ['cygstart', os.path.dirname(url)] else: - args = 'cygstart %s "%s"' % ( - wait and '-w' or '', url.replace('"', '')) - return os.system(args) + args = ['cygstart'] + if wait: + args.append('-w') + args.append(url) + try: + return subprocess.call(args) + except OSError: + # Command not found + return 127 try: if locate: diff --git a/click/termui.py b/click/termui.py index 764132309..811fb8e90 100644 --- a/click/termui.py +++ b/click/termui.py @@ -390,10 +390,11 @@ def clear(): if not isatty(sys.stdout): return # If we're on Windows and we don't have colorama available, then we - # clear the screen by shelling out. Otherwise we can use an escape + # clear the screen by invoking `cls`. Otherwise we can use an escape # sequence. if WIN: - os.system('cls') + import subprocess + subprocess.check_call(['cls']) else: sys.stdout.write('\033[2J\033[1;1H') diff --git a/tests/test_imports.py b/tests/test_imports.py index 8e9a97df6..560673aa7 100644 --- a/tests/test_imports.py +++ b/tests/test_imports.py @@ -32,7 +32,7 @@ def tracking_import(module, locals=None, globals=None, fromlist=None, ALLOWED_IMPORTS = set([ 'weakref', 'os', 'struct', 'collections', 'sys', 'contextlib', 'functools', 'stat', 're', 'codecs', 'inspect', 'itertools', 'io', - 'threading', 'colorama', 'errno', 'fcntl', 'datetime' + 'threading', 'colorama', 'errno', 'fcntl', 'datetime', 'shutil' ]) if WIN: From 6c11177e3aa83009077b1b739f7321cf115e3cba Mon Sep 17 00:00:00 2001 From: Andreas Backx Date: Sat, 26 Oct 2024 15:25:37 +0100 Subject: [PATCH 2/7] Merge _pipetempfilepager into _tempfilepager, fix double unquoting. --- src/click/_termui_impl.py | 44 +++++++++++++-------------------------- 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/src/click/_termui_impl.py b/src/click/_termui_impl.py index ff80ced24..c582b4802 100644 --- a/src/click/_termui_impl.py +++ b/src/click/_termui_impl.py @@ -378,7 +378,7 @@ def pager(generator: t.Iterable[str], color: t.Optional[bool] = None) -> None: if os.environ.get("TERM") in ("dumb", "emacs"): return _nullpager(stdout, generator, color) if WIN or sys.platform.startswith("os2"): - return _pipetempfilepager(generator, "more", color) + return _tempfilepager(generator, "more", color, pipe=True) if which("less") is not None: return _pipepager(generator, "less", color) @@ -445,34 +445,15 @@ def _pipepager(generator: t.Iterable[str], cmd: str, color: t.Optional[bool]) -> def _tempfilepager( - generator: t.Iterable[str], cmd: str, color: t.Optional[bool] + generator: t.Iterable[str], + cmd: str, + color: t.Optional[bool], + pipe: bool = False, ) -> None: - """Page through text by invoking a program on a temporary file.""" - import subprocess - import tempfile + """Page through text by invoking a program on a temporary file. - fd, filename = tempfile.mkstemp() - # TODO: This never terminates if the passed generator never terminates. - text = "".join(generator) - if not color: - text = strip_ansi(text) - encoding = get_best_encoding(sys.stdout) - with open_stream(filename, "wb")[0] as f: - f.write(text.encode(encoding)) - try: - subprocess.call([cmd, filename]) - except OSError: - # Command not found - pass - finally: - os.close(fd) - os.unlink(filename) - - -def _pipetempfilepager(generator, cmd, color): - """ - Page through text by invoking a program with a temporary file redirected - as input. + By default executes `cmd` as `cmd tmpfile`. If `pipe` is `True`, it will + instead pipe the text via stdin like `cat tmpfile | cmd`. """ import subprocess import tempfile @@ -486,8 +467,11 @@ def _pipetempfilepager(generator, cmd, color): with open_stream(filename, "wb")[0] as f: f.write(text.encode(encoding)) try: - with open_stream(filename, "rb")[0] as f: - subprocess.call([cmd], stdin=f) + if pipe: + with open_stream(filename, "rb")[0] as f: + subprocess.call([cmd], stdin=f) + else: + subprocess.call([cmd, filename]) except OSError: # Command not found pass @@ -629,7 +613,7 @@ def _unquote_file(url: str) -> str: elif WIN: if locate: url = _unquote_file(url) - args = ["explorer", f"/select,{_unquote_file(url)}"] + args = ["explorer", f"/select,{url}"] else: args = ["start"] if wait: From 976f3042e3a3a90512dc7a0bf1cbb63ed46a79d7 Mon Sep 17 00:00:00 2001 From: Andreas Backx Date: Sat, 30 Nov 2024 18:20:46 +0000 Subject: [PATCH 3/7] Remove _tempfile_pager pipe command. --- src/click/_termui_impl.py | 31 ++++++++++++++----------------- tests/test_utils.py | 9 +++++++-- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/click/_termui_impl.py b/src/click/_termui_impl.py index c582b4802..a38c77179 100644 --- a/src/click/_termui_impl.py +++ b/src/click/_termui_impl.py @@ -378,7 +378,7 @@ def pager(generator: t.Iterable[str], color: t.Optional[bool] = None) -> None: if os.environ.get("TERM") in ("dumb", "emacs"): return _nullpager(stdout, generator, color) if WIN or sys.platform.startswith("os2"): - return _tempfilepager(generator, "more", color, pipe=True) + return _pipepager(generator, "more", color) if which("less") is not None: return _pipepager(generator, "less", color) @@ -413,19 +413,25 @@ def _pipepager(generator: t.Iterable[str], cmd: str, color: t.Optional[bool]) -> elif "r" in less_flags or "R" in less_flags: color = True - c = subprocess.Popen(cmd, shell=True, stdin=subprocess.PIPE, env=env) - stdin = t.cast(t.BinaryIO, c.stdin) - encoding = get_best_encoding(stdin) + c = subprocess.Popen( + cmd, + shell=True, + stdin=subprocess.PIPE, + env=env, + errors="replace", + text=True, + ) + assert c.stdin is not None try: for text in generator: if not color: text = strip_ansi(text) - stdin.write(text.encode(encoding, "replace")) + c.stdin.write(text) except (OSError, KeyboardInterrupt): pass else: - stdin.close() + c.stdin.close() # Less doesn't respect ^C, but catches it for its own UI purposes (aborting # search or other commands inside less). @@ -448,13 +454,8 @@ def _tempfilepager( generator: t.Iterable[str], cmd: str, color: t.Optional[bool], - pipe: bool = False, ) -> None: - """Page through text by invoking a program on a temporary file. - - By default executes `cmd` as `cmd tmpfile`. If `pipe` is `True`, it will - instead pipe the text via stdin like `cat tmpfile | cmd`. - """ + """Page through text by invoking a program on a temporary file.""" import subprocess import tempfile @@ -467,11 +468,7 @@ def _tempfilepager( with open_stream(filename, "wb")[0] as f: f.write(text.encode(encoding)) try: - if pipe: - with open_stream(filename, "rb")[0] as f: - subprocess.call([cmd], stdin=f) - else: - subprocess.call([cmd, filename]) + subprocess.call([cmd, filename]) except OSError: # Command not found pass diff --git a/tests/test_utils.py b/tests/test_utils.py index a1ed559b2..110a9dced 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -179,7 +179,6 @@ def _test_gen_func(): yield "abc" -@pytest.mark.skipif(WIN, reason="Different behavior on windows.") @pytest.mark.parametrize("cat", ["cat", "cat ", "cat "]) @pytest.mark.parametrize( "test", @@ -195,7 +194,8 @@ def _test_gen_func(): ], ) def test_echo_via_pager(monkeypatch, capfd, cat, test): - monkeypatch.setitem(os.environ, "PAGER", cat) + if not WIN: + monkeypatch.setitem(os.environ, "PAGER", cat) monkeypatch.setattr(click._termui_impl, "isatty", lambda x: True) expected_output = test[0] @@ -204,6 +204,11 @@ def test_echo_via_pager(monkeypatch, capfd, cat, test): click.echo_via_pager(test_input) out, err = capfd.readouterr() + if WIN: + # Windows character changes. + expected_output = expected_output.replace("\n", "\r\n") + # more adds this when paging + expected_output += "\r\n" assert out == expected_output From 22fa2d3f44e6668499a077b7b71ac09c8ce6a0c4 Mon Sep 17 00:00:00 2001 From: Andreas Backx Date: Sat, 30 Nov 2024 18:34:00 +0000 Subject: [PATCH 4/7] Undo test changes, use _tempfilepager for more on Windows. --- src/click/_termui_impl.py | 2 +- tests/test_utils.py | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/click/_termui_impl.py b/src/click/_termui_impl.py index a38c77179..2485d8c84 100644 --- a/src/click/_termui_impl.py +++ b/src/click/_termui_impl.py @@ -378,7 +378,7 @@ def pager(generator: t.Iterable[str], color: t.Optional[bool] = None) -> None: if os.environ.get("TERM") in ("dumb", "emacs"): return _nullpager(stdout, generator, color) if WIN or sys.platform.startswith("os2"): - return _pipepager(generator, "more", color) + return _tempfilepager(generator, "more", color) if which("less") is not None: return _pipepager(generator, "less", color) diff --git a/tests/test_utils.py b/tests/test_utils.py index 110a9dced..a1ed559b2 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -179,6 +179,7 @@ def _test_gen_func(): yield "abc" +@pytest.mark.skipif(WIN, reason="Different behavior on windows.") @pytest.mark.parametrize("cat", ["cat", "cat ", "cat "]) @pytest.mark.parametrize( "test", @@ -194,8 +195,7 @@ def _test_gen_func(): ], ) def test_echo_via_pager(monkeypatch, capfd, cat, test): - if not WIN: - monkeypatch.setitem(os.environ, "PAGER", cat) + monkeypatch.setitem(os.environ, "PAGER", cat) monkeypatch.setattr(click._termui_impl, "isatty", lambda x: True) expected_output = test[0] @@ -204,11 +204,6 @@ def test_echo_via_pager(monkeypatch, capfd, cat, test): click.echo_via_pager(test_input) out, err = capfd.readouterr() - if WIN: - # Windows character changes. - expected_output = expected_output.replace("\n", "\r\n") - # more adds this when paging - expected_output += "\r\n" assert out == expected_output From adc0b2264a2db028e4bbd5948c31c9df16727b03 Mon Sep 17 00:00:00 2001 From: Andreas Backx Date: Sat, 30 Nov 2024 19:05:25 +0000 Subject: [PATCH 5/7] Enabled Windows tests again, fixed \r\n suffix with more, and use which. --- examples/termui/termui.py | 3 +++ src/click/_termui_impl.py | 50 +++++++++++++++++++++++++++++---------- tests/test_utils.py | 4 +++- 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/examples/termui/termui.py b/examples/termui/termui.py index e72e65e5f..78d522f97 100644 --- a/examples/termui/termui.py +++ b/examples/termui/termui.py @@ -167,3 +167,6 @@ def menu(): click.echo("Invalid input") elif menu == "quit": return + +if __name__ == "__main__": + pager() diff --git a/src/click/_termui_impl.py b/src/click/_termui_impl.py index 2485d8c84..ad9f8f6c9 100644 --- a/src/click/_termui_impl.py +++ b/src/click/_termui_impl.py @@ -373,31 +373,42 @@ def pager(generator: t.Iterable[str], color: t.Optional[bool] = None) -> None: pager_cmd = (os.environ.get("PAGER", None) or "").strip() if pager_cmd: if WIN: - return _tempfilepager(generator, pager_cmd, color) - return _pipepager(generator, pager_cmd, color) + if _tempfilepager(generator, pager_cmd, color): + return + elif _pipepager(generator, pager_cmd, color): + return if os.environ.get("TERM") in ("dumb", "emacs"): return _nullpager(stdout, generator, color) - if WIN or sys.platform.startswith("os2"): - return _tempfilepager(generator, "more", color) - if which("less") is not None: - return _pipepager(generator, "less", color) + if (WIN or sys.platform.startswith("os2")) and _tempfilepager( + generator, "more", color + ): + return + if _pipepager(generator, "less", color): + return import tempfile fd, filename = tempfile.mkstemp() os.close(fd) try: - if which("more") is not None: - return _pipepager(generator, "more", color) + if _pipepager(generator, "more", color): + return return _nullpager(stdout, generator, color) finally: os.unlink(filename) -def _pipepager(generator: t.Iterable[str], cmd: str, color: t.Optional[bool]) -> None: +def _pipepager(generator: t.Iterable[str], cmd: str, color: t.Optional[bool]) -> bool: """Page through text by feeding it to another program. Invoking a pager through this might support colors. + + Returns True if the command was found, False otherwise and thus another + pager should be attempted. """ + cmd_absolute = which(cmd) + if cmd_absolute is None: + return False + import subprocess env = dict(os.environ) @@ -414,7 +425,7 @@ def _pipepager(generator: t.Iterable[str], cmd: str, color: t.Optional[bool]) -> color = True c = subprocess.Popen( - cmd, + [cmd_absolute], shell=True, stdin=subprocess.PIPE, env=env, @@ -449,13 +460,24 @@ def _pipepager(generator: t.Iterable[str], cmd: str, color: t.Optional[bool]) -> else: break + return True + def _tempfilepager( generator: t.Iterable[str], cmd: str, color: t.Optional[bool], -) -> None: - """Page through text by invoking a program on a temporary file.""" +) -> bool: + """Page through text by invoking a program on a temporary file. + + Returns True if the command was found, False otherwise and thus another + pager should be attempted. + """ + # Which is necessary for Windows, it is also recommended in the Popen docs. + cmd_absolute = which(cmd) + if cmd_absolute is None: + return False + import subprocess import tempfile @@ -468,7 +490,7 @@ def _tempfilepager( with open_stream(filename, "wb")[0] as f: f.write(text.encode(encoding)) try: - subprocess.call([cmd, filename]) + subprocess.call([cmd_absolute, filename]) except OSError: # Command not found pass @@ -476,6 +498,8 @@ def _tempfilepager( os.close(fd) os.unlink(filename) + return True + def _nullpager( stream: t.TextIO, generator: t.Iterable[str], color: t.Optional[bool] diff --git a/tests/test_utils.py b/tests/test_utils.py index a1ed559b2..003315639 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -179,7 +179,6 @@ def _test_gen_func(): yield "abc" -@pytest.mark.skipif(WIN, reason="Different behavior on windows.") @pytest.mark.parametrize("cat", ["cat", "cat ", "cat "]) @pytest.mark.parametrize( "test", @@ -204,6 +203,9 @@ def test_echo_via_pager(monkeypatch, capfd, cat, test): click.echo_via_pager(test_input) out, err = capfd.readouterr() + if WIN: + # Windows character changes. + expected_output = expected_output.replace("\n", "\r\n") assert out == expected_output From 0663bca622022bcfb217c5486962b672f4e2e971 Mon Sep 17 00:00:00 2001 From: Andreas Backx Date: Sat, 30 Nov 2024 19:20:03 +0000 Subject: [PATCH 6/7] Remove Windows test change. --- tests/test_utils.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/test_utils.py b/tests/test_utils.py index 003315639..d80aee716 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -203,9 +203,6 @@ def test_echo_via_pager(monkeypatch, capfd, cat, test): click.echo_via_pager(test_input) out, err = capfd.readouterr() - if WIN: - # Windows character changes. - expected_output = expected_output.replace("\n", "\r\n") assert out == expected_output From 29dc0430f1e5f68b0806c2832446314525e49474 Mon Sep 17 00:00:00 2001 From: Andreas Backx Date: Sat, 30 Nov 2024 19:22:35 +0000 Subject: [PATCH 7/7] Remove added testing in example. --- examples/termui/termui.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/examples/termui/termui.py b/examples/termui/termui.py index 78d522f97..e72e65e5f 100644 --- a/examples/termui/termui.py +++ b/examples/termui/termui.py @@ -167,6 +167,3 @@ def menu(): click.echo("Invalid input") elif menu == "quit": return - -if __name__ == "__main__": - pager()