diff --git a/.github/workflows/publish.yaml b/.github/workflows/publish.yaml index c9f19eb..7e4d555 100644 --- a/.github/workflows/publish.yaml +++ b/.github/workflows/publish.yaml @@ -29,6 +29,10 @@ jobs: echo "::error::release tag '$GITHUB_REF_NAME' does not match pyproject version '$version'" exit 1 fi + - name: Check that the distributions ship the type hints marker + run: | + unzip -l dist/*.whl | grep -q ' cliffy/py\.typed$' + tar -tzf dist/*.tar.gz | grep -q '/cliffy/py\.typed$' - name: Smoke test the wheel run: uv run --isolated --no-project --with dist/*.whl cli --version - uses: actions/upload-artifact@v7 @@ -53,5 +57,9 @@ jobs: path: dist/ - name: Install uv uses: astral-sh/setup-uv@v10.3.0 + # --check-url: a file that is on PyPI already, with the same content, is skipped. An upload that + # stopped half way (the sdist is up, the wheel is not) can then be finished with "Re-run failed + # jobs". Without it the files that made it are rejected as duplicates and the retry fails. + # A file with the same name and different content still fails: a release is never replaced. - name: Publish - run: uv publish --trusted-publishing always dist/* + run: uv publish --trusted-publishing always --check-url https://pypi.org/simple/ dist/* diff --git a/SOUL.md b/SOUL.md index f3f8545..1e7c28e 100644 --- a/SOUL.md +++ b/SOUL.md @@ -39,7 +39,11 @@ write and a developer can review. That overlap is the project. Everything about a tool lives in its manifest: commands, parameters, help, dependencies, examples and tests. There is no second place to look and no hidden state that changes what a manifest means. -The same manifest produces the same CLI on every machine, every time. +A manifest names its inputs itself: the files it lists under `includes`, its blueprint, +and the environment variables it reads with `{{ env[...] }}`. +The same manifest with the same inputs, read by the same version of cliffy, produces the same CLI +on every machine, every time. +Cliffy adds nothing of its own: not the time, not the machine, not the order something happened to be read in. ### 2. Nothing is hidden diff --git a/cliffy/builder.py b/cliffy/builder.py index 928a873..dd1847e 100644 --- a/cliffy/builder.py +++ b/cliffy/builder.py @@ -13,7 +13,7 @@ from importlib import metadata from io import TextIOWrapper from pathlib import Path -from tempfile import NamedTemporaryFile, TemporaryDirectory +from tempfile import TemporaryDirectory from typing import ClassVar import pybash @@ -24,7 +24,7 @@ from shiv import cli as shiv_cli from cliffy.commander import CLI, SHELL_RUNTIME_IMPORT -from cliffy.helper import TEMP_FILES, delete_temp_files, import_module_from_path, make_executable +from cliffy.helper import import_generated_code, make_executable, option_like_requirement from cliffy.transformer import Transformer DEFAULT_TARGET = "zipapp" @@ -115,7 +115,10 @@ def build(self, job: BuildJob) -> BuildResult: with TemporaryDirectory() as staging: pip_install = [sys.executable, "-m", "pip", "--disable-pip-version-check", "install", "--no-compile"] - pip = subprocess.run([*pip_install, "--target", staging, *job.requirements], capture_output=True, text=True) + # `--` ends the options of pip: whatever follows is a requirement, also if it starts with `-` + pip = subprocess.run( + [*pip_install, "--target", staging, "--", *job.requirements], capture_output=True, text=True + ) if pip.returncode != 0: return self.failed(f"could not install {', '.join(job.requirements)}:\n{last_lines(pip)}") @@ -348,6 +351,9 @@ def build_cli( """ if target not in TARGETS: return BuildResult(1, f"unknown build target `{target}`: choose one of {', '.join(TARGETS)}", target=target) + if problem := option_like_requirement(deps or []): + # a manifest is checked when it is read. The requirements of a loaded CLI come from its metadata record + return BuildResult(1, f"`requires` of {cli_name}: {problem}", target=target) try: code = Path(script_path).read_text(encoding="utf-8") except OSError as e: @@ -540,18 +546,6 @@ def get_python_guard(cli_name: str, requires_python: str) -> str: def run_cli(cli_name: str, script_code: str, args: tuple) -> None: - with NamedTemporaryFile( - mode="w", prefix=f"{cli_name}_", suffix=".py", delete=False, encoding="utf-8" - ) as runner_file: - TEMP_FILES.append(runner_file) - try: - runner_file.write(script_code) - runner_file.flush() - module = import_module_from_path(runner_file.name) - finally: - # also when the generated code fails to import - delete_temp_files() - - runner_argvs = [runner_file.name, *list(args)] - sys.argv = runner_argvs + module, module_path = import_generated_code(script_code, f"{cli_name.replace('-', '_')}_cli") + sys.argv = [module_path, *list(args)] module.cli() diff --git a/cliffy/cli.py b/cliffy/cli.py index d6dff7a..db9003a 100644 --- a/cliffy/cli.py +++ b/cliffy/cli.py @@ -23,6 +23,7 @@ CLI_NAME_PATTERN, ManifestOrCLI, age_datetime, + cli_name_problem, cliffy_version, exit_err, indent_block, @@ -31,11 +32,11 @@ write_to_file, ) from cliffy.homer import get_clis, get_metadata, get_metadata_path, remove_metadata, save_metadata -from cliffy.loader import Loader +from cliffy.loader import Loader, LoaderError from cliffy.manifest import CLIManifest from cliffy.reloader import Reloader from cliffy.rich import Console, click, print_rich_table # type: ignore -from cliffy.tester import ShellScript, Tester +from cliffy.tester import ASSERTION_FILE, GeneratedImportError, ShellScript, Tester from cliffy.transformer import Transformer CONTEXT_SETTINGS = {"help_option_names": ["-h", "--help"]} @@ -58,6 +59,23 @@ def show_aliases_callback(ctx: Any, param: Any, val: bool) -> None: ctx.exit() +def stdout_is_terminal() -> bool: + return sys.stdout.isatty() + + +def print_source(text: str) -> None: + """Print generated code or a manifest to stdout as it is: no line is broken and nothing is added. + + Redirected or piped, the output is the text itself, character for character. On a terminal the + text is highlighted and long lines are left to the terminal to wrap. + """ + if stdout_is_terminal(): + Console().print(text, soft_wrap=True, emoji=False, markup=False, end="" if text.endswith("\n") else "\n") + else: + # color=True: nothing that looks like an escape sequence is removed from the text + click.echo(text, nl=not text.endswith("\n"), color=True) + + blueprint_option = click.option( "--blueprint", "-b", @@ -94,7 +112,10 @@ def update(cli_names: list[str]) -> None: """Reloads CLI by name""" failed = False for cli_name in cli_names: - if cli_metadata := get_metadata(cli_name): + if problem := cli_name_problem(cli_name): + out_err(f"~ {problem}") + failed = True + elif cli_metadata := get_metadata(cli_name): with open(cli_metadata.runner_path, encoding="utf-8") as manifest: T = Transformer(manifest, blueprint=cli_metadata.blueprint) Loader.load_from_cli(T.cli) @@ -115,9 +136,9 @@ def update(cli_names: list[str]) -> None: def render(manifest: TextIO, blueprint: str | None = None) -> None: """Render the CLI manifest generation as code""" T = Transformer(manifest, blueprint=blueprint) - console = Console() - console.print(T.cli.code, overflow="fold", emoji=False, markup=False) - out(f"# Rendered {T.cli.name} CLI v{T.cli.version} ~", fg="green") + print_source(T.cli.code) + # not on stdout: `cli render x.yaml > x.py` writes the module and nothing else + out(f"# Rendered {T.cli.name} CLI v{T.cli.version} ~", fg="green", err=True) @click.argument("manifest", type=click.File("rb")) @@ -162,8 +183,7 @@ def init(cli_name: str, render: bool, raw: bool, json_schema: bool) -> None: exit_err(f"~ {e}") if render: - console = Console() - console.print(template, overflow="fold", emoji=False, markup=False) + print_source(template) else: try: write_to_file(f"{cli_name}.yaml", text=template) @@ -193,24 +213,40 @@ def remove(cli_names: list[str]) -> None: """Remove a loaded CLI by name""" failed = False for cli_name in cli_names: - if get_metadata_path(cli_name): - remove_metadata(cli_name) - Loader.unload_cli(cli_name) - out(f"~ {cli_name} removed šŸ’„", fg="green") - else: + if problem := cli_name_problem(cli_name): + out_err(f"~ {problem}") + failed = True + elif not get_metadata_path(cli_name): out_err(f"~ {cli_name} not loaded") failed = True + elif not remove_loaded_cli(cli_name): + failed = True if failed: raise SystemExit(1) +def remove_loaded_cli(cli_name: str) -> bool: + """Remove the launcher, the module and the metadata record of a CLI. All of it, or none of it. + + The record goes last: as long as something of the CLI is left, it is listed and can be removed again. + """ + try: + Loader.unload_cli(cli_name) + except (LoaderError, ValueError) as e: + out_err(f"~ {cli_name} not removed: {e}") + return False + remove_metadata(cli_name) + out(f"~ {cli_name} removed šŸ’„", fg="green") + return True + + def remove_all() -> None: """Remove all loaded CLIs""" - for metadata in get_clis(): - remove_metadata(metadata.cli_name) - Loader.unload_cli(metadata.cli_name) - out(f"~ {metadata.cli_name} removed šŸ’„", fg="green") + # all of them are tried. The command fails when one could not be removed + removed = [remove_loaded_cli(metadata.cli_name) for metadata in list(get_clis())] + if not all(removed): + raise SystemExit(1) @click.argument("cli_or_manifests", type=ManifestOrCLI(), nargs=-1) @@ -252,6 +288,10 @@ def build( cli_name = cli.name else: cli_name = cli_or_manifest + if problem := cli_name_problem(cli_name): + out_err(f"~ {problem}. A manifest is given as a path that ends in `yaml`") + failed = True + continue if not (metadata := get_metadata(cli_name)): out_err(f"~ {cli_name} not loaded") failed = True @@ -288,6 +328,8 @@ def build( @click.argument("cli_name", type=str) def info(cli_name: str) -> None: """Display CLI info""" + if problem := cli_name_problem(cli_name): + exit_err(f"~ {problem}") metadata = get_metadata(cli_name) or exit_err(f"~ {cli_name} not loaded") out(f"{click.style('name:', fg='blue')} {metadata.cli_name}") out(f"{click.style('version:', fg='blue')} {metadata.version}") @@ -334,10 +376,20 @@ def dev(manifest: str, run_cli: bool, run_cli_args: tuple[str], blueprint: str | default=False, help="exit instantly on first error or failed test.", ) +@click.option( + "--traceback", + "show_traceback", + is_flag=True, + default=False, + help="Also print the full Python traceback of a command that raises.", +) @blueprint_option -def test(manifest: str, exitfirst: bool, blueprint: str | None = None) -> None: +def test(manifest: str, exitfirst: bool, show_traceback: bool = False, blueprint: str | None = None) -> None: """Run tests defined in a manifest""" - tester = Tester(manifest, blueprint=blueprint) + try: + tester = Tester(manifest, blueprint=blueprint) + except GeneratedImportError as e: + exit_err(f"~ error in {manifest}: the generated code does not work\n {e}") if not tester.test_pipeline: exit_err("Missing tests section in manifest") out("āœØ Invoking tests āœØ", nl=False) @@ -351,36 +403,49 @@ def test(manifest: str, exitfirst: bool, blueprint: str | None = None) -> None: tester.invoke_shell(case) except SyntaxError as e: exit_err(f"~ error in {manifest}: test step `{case.command}`, column {e.offset}: {e.msg}") + except Exception as e: + # i.e. a program that does not exist + exit_err(f"~ error in {manifest}: test step `{case.command}` could not be run: {type(e).__name__}: {e}") continue case_number += 1 out(f"\n\nšŸŖ„ > {tester.T.cli.name} {case.command}\n") + test = tester.invoke_test(case.command, case.assert_script) + result = next(test) + out("āš—ļø > \n" + result.output) + if raised := tester.describe_exception(case.command, result): + # the exception of the command, without frames of cliffy or of the generated module + out(raised) + if show_traceback and result.exc_info: + out("".join(traceback.format_exception(*result.exc_info))) + elif isinstance(result.exception, SystemExit) and result.exit_code: + out(f"exit code {result.exit_code}") + try: - test = tester.invoke_test(case.command, case.assert_script) - result = next(test) - out("āš—ļø > \n" + result.output) - if result.exception: - out(str(result)) + # the assertions of the test case next(test, "") out(f"\nāœ… {case_number} of {total}") passed += 1 - except AssertionError: - _, _, tb = sys.exc_info() - tb_info = traceback.extract_tb(tb) - _, line_no, _, _ = tb_info[-1] - expr = case.assert_script.split("\n")[line_no - 1] - out(f"šŸ’” AssertionError: (line {line_no}) > {expr}") + except SyntaxError as e: + # the assertion itself is not Python + lines = case.assert_script.split("\n") + line = lines[min(max(e.lineno or 1, 1), len(lines)) - 1].strip() + out(f"šŸ’” the assertion is not valid Python, line {e.lineno}, column {e.offset}: {e.msg}\n {line}") if exitfirst: raise SystemExit(1) from None - except SyntaxError: - out("šŸ’” Syntax error") - traceback.print_exc() - if exitfirst: - raise SystemExit(1) from None - except Exception: - out("šŸ’” Exception") - traceback.print_exc() + except Exception as e: + # a failed assertion, or an assertion that raises: reported with its line of the test case + frames = [frame for frame in traceback.extract_tb(e.__traceback__) if frame.filename == ASSERTION_FILE] + line_no = frames[0].lineno if frames else None + lines = case.assert_script.split("\n") + expr = lines[line_no - 1] if line_no and line_no <= len(lines) else case.assert_script.strip() + where = f"(line {line_no}) > {expr}" + out( + f"šŸ’” AssertionError: {where}" + if isinstance(e, AssertionError) + else f"šŸ’” {type(e).__name__}: {e} {where}" + ) if exitfirst: raise SystemExit(1) from None @@ -422,6 +487,8 @@ def docs(cli_or_manifest: str, format: str, output_dir: str, blueprint: str | No doc_generator.generate(format, output_dir) out(f"+ {T.cli.name}.{format}") else: + if problem := cli_name_problem(cli_or_manifest): + exit_err(f"~ {problem}. A manifest is given as a path that ends in `yaml`") metadata = get_metadata(cli_or_manifest) or exit_err(f"~ {cli_or_manifest} not loaded") # metadata.manifest is the raw manifest text. It still has to be rendered and merged with its # includes, so it is transformed from its source like `cli update` does. diff --git a/cliffy/commander.py b/cliffy/commander.py index 353b5bd..890f7aa 100644 --- a/cliffy/commander.py +++ b/cliffy/commander.py @@ -6,6 +6,7 @@ from collections import defaultdict from collections.abc import ItemsView, Iterator, ValuesView from contextlib import contextmanager +from typing import Any import pybash from pydantic import BaseModel @@ -38,14 +39,23 @@ def callback_label(callback: Callback) -> str: return f"callback of group `{callback.group}`, run" if callback.group else "callback of the CLI, run" +def script_text(script: RunBlock | RunBlockList) -> str: + """A run block as the one script it is generated from""" + return script.to_script() if isinstance(script, RunBlockList) else str(script.root) + + class CLI(BaseModel): name: str version: str code: str requires: list[str] = [] blueprint: str | None = None - # generated function name -> where it comes from in the manifest, i.e. {"db_create": "commands.db.create"} + # generated function name -> where it comes from in the manifest, i.e. {"db_create": "command `db.create`"} functions: dict[str, str] = {} + # generated function name -> the scripts its body is generated from, in order, as (key in the manifest, + # script): {"db_create": [("pre_run", "..."), ("run", "...")]}. Lets a line of the generated code be + # reported as a line of the manifest. + blocks: dict[str, list[tuple[str, str]]] = {} class BaseGroup(BaseModel): @@ -116,6 +126,7 @@ class Commander(ABC): __slots__ = ( "aliases_by_commands", "base_imports", + "blocks", "callbacks_by_group", "cli", "command_functions", @@ -161,6 +172,7 @@ def __init__(self, manifest: CLIManifest) -> None: # nothing else uses: two manifest names can map to the same identifier (`db-app` and the group `db`). self.taken_names: set[str] = set(self.manifest.vars) | self.user_defined_names() self.functions: dict[str, str] = {} + self.blocks: dict[str, list[tuple[str, str]]] = {} self.command_functions: dict[str, str] = {} for group in self.groups.values(): group.var = self.unique_name(to_identifier(group.name + "_app")) @@ -206,8 +218,18 @@ def command_func_name(self, command: Command) -> str: self.command_functions[command.name] = self.unique_name( self.parser.get_command_func_name(command), source=f"command `{command.name}`" ) + self.blocks[self.command_functions[command.name]] = self.command_blocks(command) return self.command_functions[command.name] + @staticmethod + def command_blocks(command: Command) -> list[tuple[str, str]]: + """The scripts of a command in the order `Parser.parse_command_run` writes them.""" + scripts = [("pre_run", command.pre_run), ("run", command.run), ("post_run", command.post_run)] + return [(key, script_text(script)) for key, script in scripts if script or key == "run"] + + def record_callback_blocks(self, function_name: str, callbacks: list[Callback]) -> None: + self.blocks[function_name] = [("run", script_text(callback.run)) for callback in callbacks] + def _merge_command_template(self, command: Command) -> None: """Merge command with its template if specified.""" if not command.template: @@ -305,6 +327,46 @@ def generate_cli(self) -> None: if SHELL_RUNTIME_REFERENCE.search(self.cli, base_imports_end): self.cli = self.cli[:base_imports_end] + SHELL_RUNTIME_IMPORT + self.cli[base_imports_end:] + # A rich markup tag, as rich.markup finds it: `[bold]`, `[/]`, `[#ff0000]`, `[@click]` + MARKUP_TAG = re.compile(r"(\\*)(\[[a-z#/@][^[]*?])") + + def examples_epilog(self, escape_markup: bool = False) -> str: + """Text for the end of the CLI's `--help`: the manifest's `examples`, one paragraph each. + + `$ notes add "buy milk" # Add a note.` A paragraph per example, because the help renderers + (Click, rich-click and Typer) agree on nothing else: they join or re-wrap the lines within one. + + Args: + escape_markup: Typer reads `[...]` in help text as rich markup and would drop `[home]` from + `--tag [home]`. An example is a command line to copy: it is shown as written. + """ + lines = [] + for example in self.manifest.examples: + command, description = ( + (example, None) if isinstance(example, str) else (example.command, example.description) + ) + line = "$ " + " ".join(command.splitlines()).strip() + if description and description.strip(): + line += " # " + " ".join(description.split()) + lines.append(line) + if not lines: + return "" + + epilog = "\n\n".join(["Examples:", *lines]) + if escape_markup: + epilog = self.MARKUP_TAG.sub(lambda tag: f"{tag.group(1) * 2}\\{tag.group(2)}", epilog) + return epilog + + def cli_options_with_examples(self, escape_markup: bool = False) -> dict[str, Any]: + """`cli_options` with the examples in `epilog`, after an epilog the manifest sets itself.""" + options = dict(self.manifest.cli_options) + if examples := self.examples_epilog(escape_markup): + own_epilog = options.get("epilog") + options["epilog"] = ( + f"{own_epilog}\n\n{examples}" if isinstance(own_epilog, str) and own_epilog else examples + ) + return options + def header(self) -> str: """First line of the generated code. It holds nothing that changes between two runs, such as the time: the same manifest gives the same code.""" @@ -492,4 +554,5 @@ def generate_cli(manifest: CLIManifest, commander_cls: type[Commander] = Command code=commander.cli, requires=manifest.requires, functions=commander.functions, + blocks=commander.blocks, ) diff --git a/cliffy/commanders/click.py b/cliffy/commanders/click.py index b9c91e1..2787580 100644 --- a/cliffy/commanders/click.py +++ b/cliffy/commanders/click.py @@ -67,12 +67,13 @@ def show_aliases(ctx, param, value): """ callbacks = self.callbacks_for() self.functions["cli"] = "callback of the CLI" + self.record_callback_blocks("cli", callbacks) self.cli += """ @click.group(context_settings=CONTEXT_SETTINGS""" if "invoke_without_command" not in self.manifest.cli_options: self.cli += self.parse_callback_options(callbacks, leading=", ") - if self.manifest.cli_options: - self.cli += f",{self.click_parser.to_args(self.manifest.cli_options)}" + if cli_options := self.cli_options_with_examples(): + self.cli += f",{self.click_parser.to_args(cli_options)}" self.cli += f""") @click.version_option({self.manifest.version!r})""" @@ -117,6 +118,7 @@ def define_groups(self) -> None: parsed_help = clean_help(group.help) callbacks = self.callbacks_for(group) self.functions[group.var_name] = f"callback of group `{group.name}`" + self.record_callback_blocks(group.var_name, callbacks) self.cli += f""" @click.group({self.parse_callback_options(callbacks)}){self.parse_callback_decorators(callbacks)} def {group.var_name}({self.parse_callback_param_names(callbacks)}): @@ -182,10 +184,12 @@ def {self.alias_func_name(command, alias)}({self.click_parser.get_param_names(co def alias_func_name(self, command: Command, alias: str) -> str: """Name of the function that is registered under an alias: Click needs one function per command name.""" - return self.unique_name( + name = self.unique_name( self.click_parser.get_alias_func_name(self.command_func_name(command), alias), source=f"command `{command.name}`", ) + self.blocks[name] = self.command_blocks(command) + return name def add_main_block(self) -> None: self.cli += """ diff --git a/cliffy/commanders/typer.py b/cliffy/commanders/typer.py index 1a15f2a..fc1e41a 100644 --- a/cliffy/commanders/typer.py +++ b/cliffy/commanders/typer.py @@ -22,8 +22,10 @@ def add_base_cli(self) -> None: self.cli += """ CONTEXT_SETTINGS = dict(help_option_names=['-h', '--help']) cli = typer.Typer(context_settings=CONTEXT_SETTINGS""" - if self.manifest.cli_options: - self.cli += f",{self.parser.to_args(self.manifest.cli_options)}" + # Typer shows help text as rich markup unless the manifest chooses another mode + rich_markup = self.manifest.cli_options.get("rich_markup_mode", "rich") == "rich" + if cli_options := self.cli_options_with_examples(escape_markup=rich_markup): + self.cli += f",{self.parser.to_args(cli_options)}" if self.manifest.help: self.cli += f", help={py_str(self.manifest.help)}" self.cli += f""") @@ -48,6 +50,7 @@ def aliases_callback(value: bool): """ callbacks = self.callbacks_for() self.functions["main"] = "callback of the CLI" + self.record_callback_blocks("main", callbacks) self.cli += f""" @cli.callback({self.parse_callback_options(callbacks)}) def main({self.parse_callback_params(callbacks)}""" @@ -118,6 +121,7 @@ def define_groups(self) -> None: callback_name = self.unique_name( f"{group.var_name}_callback", source=f"callback of group `{group.name}`" ) + self.record_callback_blocks(callback_name, callbacks) self.cli += f""" @{group.var_name}.callback({self.parse_callback_options(callbacks)}) def {callback_name}({self.parse_callback_params(callbacks)} diff --git a/cliffy/helper.py b/cliffy/helper.py index b69a98e..6ef9a6b 100644 --- a/cliffy/helper.py +++ b/cliffy/helper.py @@ -5,13 +5,14 @@ import os import platform import re +import secrets import sys from datetime import datetime from importlib.metadata import PackageNotFoundError, distributions, version from importlib.resources import files from importlib.util import module_from_spec, spec_from_file_location from pathlib import Path -from tempfile import _TemporaryFileWrapper +from tempfile import TemporaryDirectory from types import ModuleType from typing import IO, Any, NoReturn @@ -47,7 +48,6 @@ } ) CLI_NAME_PATTERN = re.compile(r"[A-Za-z_][A-Za-z0-9_-]*") -TEMP_FILES: list[_TemporaryFileWrapper] = [] class ManifestOrCLI(File): @@ -65,6 +65,27 @@ def convert( # type: ignore[override] return value +def cli_name_problem(name: object) -> str | None: + """Why `name` cannot be the name of a CLI, or None when it can. + + A name becomes a file name in three directories (launcher, module, metadata record). Whatever a + command is given as the name of a loaded CLI goes through this first, so that `..` or a path + separator cannot make it point at a file outside of them. + """ + if isinstance(name, str) and CLI_NAME_PATTERN.fullmatch(name): + return None + return ( + f"{name!r} is not a valid CLI name: a name has letters, digits, `_` and `-` only " + "and starts with a letter or `_`" + ) + + +def validate_cli_name(name: str) -> str: + if problem := cli_name_problem(name): + raise ValueError(problem) + return name + + def write_to_file(path: str, text: str, executable: bool = False) -> None: output_file = Path(path) output_file.parent.mkdir(exist_ok=True, parents=True) @@ -75,6 +96,47 @@ def write_to_file(path: str, text: str, executable: bool = False) -> None: make_executable(path) +def write_atomically(path: str, text: str, executable: bool = False, replace: bool = True) -> None: + """Write a file so that `path` is never seen half-written and a symbolic link at `path` is never followed. + + The text goes to a new file in the same directory, which is then moved onto `path`: whatever is + at `path`, a link included, is replaced as a directory entry, not written through. + + Args: + replace: With False the file is only created. FileExistsError is raised when something is at + `path` already, also when it appeared a moment ago, and that file is left alone. + """ + directory = os.path.dirname(path) or "." + os.makedirs(directory, exist_ok=True) + temp_path = os.path.join(directory, f".{os.path.basename(path)}.{os.getpid()}.{secrets.token_hex(4)}.tmp") + no_follow = getattr(os, "O_NOFOLLOW", 0) + # the mode is reduced by the umask, like that of any new file + mode = 0o777 if executable else 0o666 + + def create(target: str) -> None: + descriptor = os.open(target, os.O_WRONLY | os.O_CREAT | os.O_EXCL | no_follow, mode) + # generated code and manifests are always UTF-8, whatever the locale encoding is + with os.fdopen(descriptor, "w", encoding="utf-8") as file: + file.write(text) + + try: + create(temp_path) + if replace: + os.replace(temp_path, path) + return + try: + # a hard link is only made when nothing has the name + os.link(temp_path, path) + except FileExistsError: + raise + except (OSError, NotImplementedError): + # a file system without hard links: created exclusively instead + create(path) + finally: + with contextlib.suppress(FileNotFoundError): + os.unlink(temp_path) + + def import_module_from_path(filepath: str) -> ModuleType: try: path = Path(filepath) @@ -96,12 +158,19 @@ def make_executable(path: str) -> None: os.chmod(path, mode) -def delete_temp_files() -> None: - for file in TEMP_FILES: - with contextlib.suppress(Exception): - file.close() - os.unlink(file.name) - TEMP_FILES.clear() +def import_generated_code(code: str, module_name: str) -> tuple[ModuleType, str]: + """Import generated code from a file, as the module it is when it is loaded. + + The file is in a temporary directory that belongs to this call and is removed before it returns, + also when the import fails. Runs on other threads have their own and cannot remove this one. + + Returns: + The module, and the path it was imported from: the file name in its tracebacks. + """ + with TemporaryDirectory(prefix="cliffy_") as directory: + path = os.path.join(directory, f"{module_name}.py") + Path(path).write_text(code, encoding="utf-8") + return import_module_from_path(path), path def indent_block(block: str, spaces: int = 4) -> str: @@ -172,6 +241,22 @@ def get_installed_package_versions() -> dict[str, str]: return installed_packages +def option_like_requirement(requirements: list[str]) -> str | None: + """Why `requirements` cannot be handed to an installer, or None when they can. + + `cli build` passes the manifest's `requires` to pip. An entry that starts with `-` would be read + as an option of pip (`--index-url ...`, `-r file`) and change where and what it installs from, + so `requires` holds requirements and nothing else. + """ + for requirement in requirements: + if str(requirement).strip().startswith("-"): + return ( + f"`{str(requirement).strip()}` is an option, not a requirement. `requires` lists packages, " + "such as `rich` or `tabulate>=0.9.0`" + ) + return None + + def check_requirement(requirement: str, installed_packages: dict[str, str]) -> str | None: """Checks a PEP 508 requirement against the installed packages. diff --git a/cliffy/homer.py b/cliffy/homer.py index e2a5acf..466de2e 100644 --- a/cliffy/homer.py +++ b/cliffy/homer.py @@ -8,7 +8,7 @@ from pathlib import Path from cliffy.commander import CLI -from cliffy.helper import CLIFFY_METADATA_DIR, write_to_file +from cliffy.helper import CLIFFY_METADATA_DIR, cli_name_problem, write_to_file from cliffy.manifest import CLIMetadata @@ -46,6 +46,9 @@ def remove_metadata(cli_name: str) -> None: Args: cli_name (str): CLI name """ + if cli_name_problem(cli_name): + # i.e. `../x`: not the name of a CLI, and it would select files outside of the metadata directory + return # the name is matched literally: `*` or `?` in it must not select other CLIs metadata_paths = glob.glob(f"{glob.escape(str(CLIFFY_METADATA_DIR))}/*/{glob.escape(cli_name)}.*") for path in metadata_paths: @@ -102,6 +105,9 @@ def get_metadata_path(cli_name: str) -> str | None: Returns: Optional[str]: CLI metadata path """ + if cli_name_problem(cli_name): + # i.e. `../x`: not the name of a CLI, and it would select files outside of the metadata directory + return None if cli_metadata_path := glob.glob(f"{glob.escape(str(CLIFFY_METADATA_DIR))}/*/{glob.escape(cli_name)}.json"): return cli_metadata_path[0] return None @@ -115,4 +121,11 @@ def get_clis() -> Iterator[CLIMetadata]: """ metadata_paths = Path(str(CLIFFY_METADATA_DIR)).glob("*/*.json") for metadata_path in metadata_paths: - yield get_metadata_bypath(metadata_path) + metadata = get_metadata_bypath(metadata_path) + # A CLI is known by the name of its record file. The name inside the record is not used to + # find or remove files: an edited record cannot point `cli rm-all` at another CLI. + yield ( + metadata + if metadata.cli_name == metadata_path.stem + else metadata.model_copy(update={"cli_name": metadata_path.stem}) + ) diff --git a/cliffy/loader.py b/cliffy/loader.py index dcebd62..43f61d1 100644 --- a/cliffy/loader.py +++ b/cliffy/loader.py @@ -1,8 +1,24 @@ import contextlib import os +import stat +from typing import Literal from cliffy.commander import CLI -from cliffy.helper import CLIFFY_CLI_DIR, PYTHON_BIN, PYTHON_EXECUTABLE, exit_err, write_to_file +from cliffy.helper import ( + CLIFFY_CLI_DIR, + PYTHON_BIN, + PYTHON_EXECUTABLE, + exit_err, + validate_cli_name, + write_atomically, +) + +ScriptState = Literal["missing", "owned", "foreign"] +NOT_OURS = "already exists and was not created by cliffy, or was changed since" + + +class LoaderError(Exception): + """A loaded CLI could not be removed. The message says what is in the way; nothing was removed.""" class Loader: @@ -13,18 +29,29 @@ def __init__(self, cli: CLI) -> None: def deploy_cli(self) -> str: cli_path = Loader.get_cli_path(self.cli.name) - write_to_file(cli_path, self.cli.code) + write_atomically(cli_path, self.cli.code) return cli_path def deploy_script(self) -> str: + """Write the launcher into the directory of the interpreter's executables. + + A file that is already there is only replaced when it is the launcher cliffy wrote for this + CLI. Anything else stops the load: a manifest named `python` or `pip` would replace that executable. + """ script_path = Loader.get_cli_script_path(self.cli.name) - if os.path.lexists(script_path) and not Loader.is_cli_script(script_path): - # i.e. a manifest named `python` or `pip`: the script would replace that executable - exit_err( - f"~ cannot load {self.cli.name}: {script_path} already exists and was not created by cliffy. " - "Rename the CLI in the manifest" + refused = f"~ cannot load {self.cli.name}: {script_path} {NOT_OURS}. Rename the CLI in the manifest" + state = Loader.script_state(script_path, self.cli.name) + if state == "foreign": + exit_err(refused) + + try: + # A new launcher is created exclusively: a file that appeared since the check is kept. + # An own launcher is replaced by a rename, which never writes through a link. + write_atomically( + script_path, Loader.get_cli_script(self.cli.name), executable=True, replace=state == "owned" ) - write_to_file(script_path, Loader.get_cli_script(self.cli.name), executable=True) + except FileExistsError: + exit_err(refused) return script_path @classmethod @@ -35,29 +62,84 @@ def load_from_cli(cls, cli: CLI) -> None: @classmethod def unload_cli(cls, cli_name: str) -> None: + """Remove the launcher and the module of a loaded CLI: both, or neither. + + Raises: + ValueError: `cli_name` is not a CLI name. + LoaderError: the launcher is not the one cliffy wrote (edited, replaced, a link, unreadable), + or a file could not be deleted. The CLI is left as it was and still runs. + """ script_path = cls.get_cli_script_path(cli_name) - if cls.is_cli_script(script_path): + module_path = cls.get_cli_path(cli_name) + + # checked right before the file is deleted + state = cls.script_state(script_path, cli_name) + if state == "foreign": + raise LoaderError( + f"{script_path} was not created by cliffy, or was changed since. Nothing was removed and " + f"{cli_name} is still loaded. Move or delete that file, then remove {cli_name} again" + ) + try: + if state == "owned": + os.unlink(script_path) + except FileNotFoundError: + state = "missing" + except OSError as e: + raise LoaderError(f"could not delete {script_path}: {e}. Nothing was removed") from e + + try: with contextlib.suppress(FileNotFoundError): - os.remove(script_path) - with contextlib.suppress(FileNotFoundError): - os.remove(cls.get_cli_path(cli_name)) + os.unlink(module_path) + except OSError as e: + if state == "owned": + # put back, so that the CLI is not left with a module and nothing to start it + with contextlib.suppress(OSError): + write_atomically(script_path, cls.get_cli_script(cli_name), executable=True, replace=False) + raise LoaderError(f"could not delete {module_path}: {e}. {cli_name} is still loaded") from e @staticmethod - def is_cli_script(script_path: str) -> bool: - """Whether the file is an entry script written by `deploy_script`""" + def script_state(script_path: str, cli_name: str) -> ScriptState: + """Whether the launcher of a CLI is missing, is the file `deploy_script` writes for it, or is anything else. + + "owned" is a regular file, not a link to one, whose text is the launcher of exactly this CLI. + Only its `#!` line may differ: one environment can be reached through several interpreter paths. + """ try: - with open(script_path, encoding="utf-8") as script: - return "from cliffy.clis." in script.read(4096) + if not stat.S_ISREG(os.lstat(script_path).st_mode): + return "foreign" + # O_NOFOLLOW: a link put there after the lstat fails to open instead of being followed + descriptor = os.open(script_path, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)) + except FileNotFoundError: + return "missing" + except OSError: + return "foreign" + + expected = Loader.get_cli_script(cli_name) + try: + with os.fdopen(descriptor, "rb") as script: + if not stat.S_ISREG(os.fstat(script.fileno()).st_mode): + return "foreign" + # more than a launcher holds is not a launcher: the rest does not need to be read + content = script.read(len(expected.encode("utf-8")) + 4096).decode("utf-8") except (OSError, UnicodeDecodeError): - return False + return "foreign" + + interpreter_line, _, body = content.replace("\r\n", "\n").partition("\n") + owned = interpreter_line.startswith("#!") and body == expected.partition("\n")[2] + return "owned" if owned else "foreign" + + @staticmethod + def is_cli_script(script_path: str, cli_name: str) -> bool: + """Whether the file is the launcher written by `deploy_script` for this CLI""" + return Loader.script_state(script_path, cli_name) == "owned" @staticmethod def get_cli_path(cli_name: str) -> str: - return f"{CLIFFY_CLI_DIR}/{cli_name.replace('-', '_')}.py" + return f"{CLIFFY_CLI_DIR}/{validate_cli_name(cli_name).replace('-', '_')}.py" @staticmethod def get_cli_script_path(cli_name: str) -> str: - return f"{PYTHON_BIN}/{cli_name}" + return f"{PYTHON_BIN}/{validate_cli_name(cli_name)}" @staticmethod def get_cli_script(cli_name: str) -> str: diff --git a/cliffy/manifest.py b/cliffy/manifest.py index adb9f46..5dc5ec1 100644 --- a/cliffy/manifest.py +++ b/cliffy/manifest.py @@ -8,7 +8,7 @@ from pydantic import BaseModel, Field, RootModel, ValidationInfo, field_validator, model_validator -from .helper import CLI_NAME_PATTERN, GENERATED_NAMES, wrap_as_comment +from .helper import CLI_NAME_PATTERN, GENERATED_NAMES, option_like_requirement, wrap_as_comment LATEST_SCHEMA_VERSION = "v3" MIGRATION_GUIDE = "https://jaykv.github.io/cliffy/project/migration/" @@ -370,9 +370,18 @@ class CLIManifest(BaseModel): tests: list[str | dict[str, str]] = Field(default=[], description="Test cases for commands") examples: list[str | Example] = Field( - default=[], description="Example command usages to display in generated CLI docs." + default=[], + description="Example command lines of the CLI, with an optional description each. " + "Shown at the end of the `--help` of the generated CLI and in the pages written by `cli docs`.", ) + @field_validator("requires", mode="after") + @classmethod + def requires_are_requirements(cls, value: list[str]) -> list[str]: + if problem := option_like_requirement(value): + raise ValueError(problem) + return value + @field_validator("name", mode="after") @classmethod def is_safe_name(cls, value: str) -> str: @@ -644,6 +653,13 @@ class IncludeManifest(BaseModel): cli_options: dict[str, str] = {} tests: list[str | dict[str, str]] = [] + @field_validator("requires", mode="after") + @classmethod + def requires_are_requirements(cls, value: list[str]) -> list[str]: + if problem := option_like_requirement(value): + raise ValueError(problem) + return value + class CLIMetadata(BaseModel): """Metadata model""" diff --git a/cliffy/reloader.py b/cliffy/reloader.py index d1ee31b..82b27b9 100644 --- a/cliffy/reloader.py +++ b/cliffy/reloader.py @@ -1,3 +1,4 @@ +import itertools import os import threading import time @@ -13,6 +14,15 @@ from cliffy.loader import Loader from cliffy.transformer import Transformer, source_files +# One reload at a time, for all manifests: a reload reads the manifest and writes the launcher, the module +# and the metadata record, and two of them must not interleave. +RELOAD_LOCK = threading.Lock() +GENERATIONS = itertools.count(1) +# manifest path -> generation of the newest change a reload was started for +NEWEST_GENERATION: dict[str, int] = {} + +FileSignature = tuple[int, int] | None + class Reloader(FileSystemEventHandler): def __init__(self, manifest_path: str, run_cli: bool, run_cli_args: tuple, blueprint: str | None = None) -> None: @@ -26,6 +36,8 @@ def __init__(self, manifest_path: str, run_cli: bool, run_cli_args: tuple, bluep self.scheduled_directories: set[str] = set() # the manifest, the manifests it includes and the Python file of its blueprint self.watched_files: set[str] = set() + # what the watched files looked like at the last change a reload was started for + self.reloaded_signature: dict[str, FileSignature] | None = None self.refresh_watched_files() super().__init__() @@ -49,16 +61,21 @@ def on_modified(self, event: FileSystemEvent) -> None: if not isinstance(event, FileModifiedEvent): return - if datetime.now() - self.last_modified < timedelta(seconds=1): + # A save is reported more than once. Within a second of the last reload an event is only a new + # change when a watched file differs from what that reload was started for. + signature = self.watched_signature() + if datetime.now() - self.last_modified < timedelta(seconds=1) and signature == self.reloaded_signature: return self.last_modified = datetime.now() self.refresh_watched_files() + self.reloaded_signature = self.watched_signature() + generation = next(GENERATIONS) t = threading.Thread( target=self.reload, args=(self.manifest_path, self.run_cli, self.run_cli_args), - kwargs={"blueprint": self.blueprint}, + kwargs={"blueprint": self.blueprint, "generation": generation}, daemon=True, ) try: @@ -66,6 +83,20 @@ def on_modified(self, event: FileSystemEvent) -> None: except RuntimeError as e: # Log the error or handle it as needed out_err(f"Failed to start thread: {e}") + return + # only once its reload runs: reloads of older changes that are still waiting can then be skipped + NEWEST_GENERATION[self.manifest_realpath] = max(generation, NEWEST_GENERATION.get(self.manifest_realpath, 0)) + + def watched_signature(self) -> dict[str, FileSignature]: + """Modification time and size of every watched file, None for one that cannot be read.""" + signature: dict[str, FileSignature] = {} + for path in sorted(self.watched_files): + try: + status = os.stat(path) + signature[path] = (status.st_mtime_ns, status.st_size) + except OSError: + signature[path] = None + return signature @classmethod def watch(cls, manifest_path: str, run_cli: bool, run_cli_args: tuple, blueprint: str | None = None) -> None: @@ -86,12 +117,36 @@ def watch(cls, manifest_path: str, run_cli: bool, run_cli_args: tuple, blueprint observer.join() @staticmethod - def reload(manifest_path: str, run_cli: bool, run_cli_args: tuple, blueprint: str | None = None) -> None: - with open(manifest_path, encoding="utf-8") as manifest_io: - T = Transformer(manifest_io, blueprint=blueprint) if blueprint else Transformer(manifest_io) - Loader.load_from_cli(T.cli) - save_metadata(manifest_path, T.cli) - out(f"āœØ Reloaded {T.cli.name} CLI v{T.cli.version} āœØ", fg="green") - - if run_cli: + def reload( + manifest_path: str, + run_cli: bool, + run_cli_args: tuple, + blueprint: str | None = None, + generation: int | None = None, + ) -> None: + """Generate the CLI from the manifest as it is now and load it. + + Reloads run one after the other, and each reads the manifest only once it is its turn: the one + that runs last loads the newest text, whatever order the threads were started in. + + Args: + generation: Number of the change this reload was started for. A reload is skipped when a + reload for a newer change of the same manifest has been started: that one does the work. + """ + + def superseded() -> bool: + newest = NEWEST_GENERATION.get(os.path.realpath(manifest_path), 0) + return generation is not None and generation < newest + + with RELOAD_LOCK: + if superseded(): + return + with open(manifest_path, encoding="utf-8") as manifest_io: + T = Transformer(manifest_io, blueprint=blueprint) if blueprint else Transformer(manifest_io) + Loader.load_from_cli(T.cli) + save_metadata(manifest_path, T.cli) + out(f"āœØ Reloaded {T.cli.name} CLI v{T.cli.version} āœØ", fg="green") + + # not under the lock: a CLI that keeps running must not hold up the next reload + if run_cli and not superseded(): cli_runner(T.cli.name, T.cli.code, run_cli_args) diff --git a/cliffy/rich.py b/cliffy/rich.py index 5d41f91..82c4787 100644 --- a/cliffy/rich.py +++ b/cliffy/rich.py @@ -19,7 +19,7 @@ def print(self, text: Any, **kwargs: Any) -> None: if isinstance(text, PlainTable): click.echo(text) else: - print(text) + print(text, end=kwargs.get("end", "\n")) class PlainTable: diff --git a/cliffy/tester.py b/cliffy/tester.py index 7b7d5ee..8851fe1 100644 --- a/cliffy/tester.py +++ b/cliffy/tester.py @@ -1,6 +1,9 @@ +import ast import inspect +import linecache +import os +import traceback from collections.abc import Generator -from tempfile import NamedTemporaryFile from typing import cast import pybash @@ -10,11 +13,22 @@ from pydantic import BaseModel, field_validator from typer.testing import CliRunner -from cliffy.helper import TEMP_FILES, delete_temp_files, import_module_from_path +from cliffy.checker import describe +from cliffy.helper import import_generated_code from cliffy.manifest import CLIManifest, Command, RunBlock -from cliffy.parser import Parser +from cliffy.parser import Parser, transform_script from cliffy.transformer import Transformer +# File name of the assertions of a test case in tracebacks +ASSERTION_FILE = "" + + +class GeneratedImportError(ImportError): + """The code generated for the manifest fails while it is imported. + + The message names the part of the manifest the failing code was generated from. + """ + class TestCase(BaseModel): command: str @@ -42,17 +56,15 @@ def __init__(self, manifest_path: str, blueprint: str | None = None) -> None: with open(manifest_path, encoding="utf-8") as manifest_io: self.T = Transformer(manifest_io, blueprint=blueprint) - with NamedTemporaryFile( - mode="w", prefix=f"{self.T.cli.name}_test_", suffix=".py", delete=False, encoding="utf-8" - ) as runner_file: - TEMP_FILES.append(runner_file) - try: - runner_file.write(self.T.cli.code) - runner_file.flush() - self.module = import_module_from_path(runner_file.name) - finally: - # also when the generated code fails to import - delete_temp_files() + # module_path is the file name of the generated code in tracebacks. The file itself is gone + module_name = f"{self.T.cli.name.replace('-', '_')}_test" + try: + self.module, self.module_path = import_generated_code(self.T.cli.code, module_name) + except ImportError as e: + raise GeneratedImportError(self.describe_import_error(e, f"{module_name}.py")) from e + # the lines of a traceback are read from here (`cli test --traceback`) + code_lines = self.T.cli.code.splitlines(keepends=True) + linecache.cache[self.module_path] = (len(self.T.cli.code), None, code_lines, self.module_path) self.module_funcs = inspect.getmembers(self.module, inspect.isfunction) @@ -89,9 +101,95 @@ def invoke_test(self, command: str, script: str) -> Generator[Result, None, None result, stream, unstyle(captured.decode(self.runner.charset, "replace")).encode(self.runner.charset) ) yield result - code = compile(script, f"test_{self.T.cli.name}.py", "exec") + code = compile(script, ASSERTION_FILE, "exec") exec(code, {}, {"result": result, "result_text": result.output.strip()}) + def describe_import_error(self, error: ImportError, file_name: str) -> str: + """Why the generated code does not import, against the part of the manifest it comes from.""" + cause = error.__cause__ or error + if isinstance(cause, SyntaxError): + line_number = cause.lineno + else: + frames = [ + frame + for frame in traceback.extract_tb(cause.__traceback__) + if os.path.basename(frame.filename) == file_name + ] + line_number = frames[-1].lineno if frames else None + return describe(self.T.cli, cause, line_number) + + def describe_exception(self, command: str, result: Result) -> str | None: + """An exception a command ended with, in the manifest's terms. None when it did not raise. + + `command `db.create` raised ValueError: no such table`, then the line of the script it was + raised on as it is written in the manifest. No frame of cliffy, of the framework or of the + generated module is shown. + """ + error = result.exception + if error is None or isinstance(error, SystemExit): + # an exit code is a result of the command, not a failure of it + return None + + trace = result.exc_info[2] if result.exc_info else error.__traceback__ + frames = [frame for frame in traceback.extract_tb(trace) if frame.filename == self.module_path and frame.lineno] + functions = self.T.cli.functions + owner = next((frame for frame in reversed(frames) if frame.name in functions), None) + who = functions[owner.name] if owner else f"`{self.T.cli.name} {command}`" + message = f"šŸ’„ {who} raised {type(error).__name__}: {error}" + + code_lines = self.T.cli.code.splitlines() + if owner and owner.lineno: + message += f"\n {self.locate_line(owner.name, owner.lineno)}" + if frames and frames[-1] is not owner and (line_number := frames[-1].lineno): + # raised further in: in a function of the `functions` section, or in a function defined in the script + located = self.locate_line(owner.name, line_number, or_none=True) if owner else None + source = code_lines[line_number - 1].strip() if line_number <= len(code_lines) else "" + message += f"\n {located or f'in `{frames[-1].name}` (functions): {source}'}" + return message + + def locate_line(self, function_name: str, line_number: int, or_none: bool = False) -> str | None: + """Line `line_number` of the generated code as `run, line 2: `. + + Falls back to the generated line itself when it is not part of a script of the function, i.e. + when a blueprint changed the code after it was generated. + """ + code_lines = self.T.cli.code.splitlines() + fallback = None if or_none else code_lines[line_number - 1].strip() + function = next( + ( + node + for node in ast.parse(self.T.cli.code).body + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef) and node.name == function_name + ), + None, + ) + if function is None or not function.lineno <= line_number <= (function.end_lineno or 0): + return fallback + + # the scripts follow each other in the body, each as `Parser.parse_run_block` wrote it + blocks = [] + for key, script in self.T.cli.blocks.get(function_name, []): + transformed = transform_script(script, key) + skipped = (len(transformed) - len(transformed.lstrip())) and transformed[ + : len(transformed) - len(transformed.lstrip()) + ].count("\n") + blocks.append((key, script, skipped, [" " * 4 + line for line in transformed.strip().split("\n")])) + + body = [line for _, _, _, generated in blocks for line in generated] + last = function.end_lineno or function.lineno + start = next( + (first for first in range(function.lineno + 1, last + 1) if code_lines[first - 1 : last] == body), None + ) + if start is None: + return fallback + + for key, script, skipped, generated in blocks: + if line_number < start + len(generated): + script_line = line_number - start + skipped + 1 + return f"{key}, line {script_line}: {script.split(chr(10))[script_line - 1].strip()}" + start += len(generated) + return fallback + def is_valid_command(self, command: str) -> bool: command_name = command.split(" ")[0] cmd = Command(name=command_name, run=RunBlock("")) diff --git a/docs/guides/dev-loop.md b/docs/guides/dev-loop.md index a47e934..fec3a28 100644 --- a/docs/guides/dev-loop.md +++ b/docs/guides/dev-loop.md @@ -71,5 +71,14 @@ $ cli remove-all # uninstall everything cliffy loaded `cli ls`, `cli reload`, `cli rm` and `cli rm-all` are aliases for these. Details are in the [CLI reference](../reference/cli.md). +Loading writes a small launcher script named after the CLI into the `bin` directory of the +environment. cliffy only replaces or deletes that file while it is exactly the launcher it wrote. If +a file with the name of your CLI is already there (`pip`, say), `cli load` stops and asks you to +rename the CLI. If you edited the launcher, `cli remove` removes nothing, tells you which file is in +the way and leaves the CLI loaded and working; move or delete that file and remove the CLI again. + +While `cli dev` runs, reloads happen one at a time and each one reads the manifest when it starts, +so after a burst of saves the loaded CLI is the one of the last save. + Loaded CLIs belong to the Python environment cliffy is installed in. They are available whenever that environment is active and disappear with it. diff --git a/docs/guides/generating-docs.md b/docs/guides/generating-docs.md index 17b1962..07d016c 100644 --- a/docs/guides/generating-docs.md +++ b/docs/guides/generating-docs.md @@ -5,7 +5,20 @@ with its params and aliases, and the examples you provide. ## Add examples -`examples` is a list of command lines with a description each. It is only used for documentation. +`examples` is a list of command lines with a description each. They are part of the documentation +`cli docs` writes, and the generated CLI shows them at the end of its own `--help`: + +```text + Examples: + + $ notes add "buy milk" --tag home # Add a note tagged home. + + $ notes ls --tag home # Show only the notes tagged home. +``` + +Write an example as you would type it, starting with the name of the CLI. It is shown as written: +quotes, brackets and backslashes are not interpreted. The help of a single command does not repeat +the examples. ```yaml --8<-- "docs/snippets/notes.yaml:examples" diff --git a/docs/guides/manifest-basics.md b/docs/guides/manifest-basics.md index 989c628..cc4ad6d 100644 --- a/docs/guides/manifest-basics.md +++ b/docs/guides/manifest-basics.md @@ -39,7 +39,7 @@ hello world | `cli_options` | Options for the CLI application object. | [below](#cli-options) | | `use_click` | Generate Click code instead of Typer. | [Click and Typer backends](backends.md) | | `tests` | Commands to run and assertions to check. | [Testing manifests](testing.md) | -| `examples` | Example command lines for generated docs. | [Generating docs](generating-docs.md) | +| `examples` | Example command lines, shown in `--help` and in generated docs. | [Generating docs](generating-docs.md) | Keys that cliffy does not know are ignored. The [manifest reference](../reference/manifest.md) lists every key with its type and default. @@ -64,6 +64,9 @@ To read the generated module for any manifest: $ cli render hello.yaml ``` +The output is the module itself, so `cli render hello.yaml > hello.py` writes a file you can run. The +closing `# Rendered ...` line goes to the error stream and does not end up in that file. + ## Imports and functions ```yaml diff --git a/docs/guides/testing.md b/docs/guides/testing.md index edd7f36..1f97986 100644 --- a/docs/guides/testing.md +++ b/docs/guides/testing.md @@ -70,6 +70,27 @@ A failing assertion is reported with its line: šŸ’” AssertionError: (line 1) > assert result.output == "1\n" ``` +So is an assertion that raises something else, or that is not valid Python: + +```text +šŸ’” NameError: name 'reslt' is not defined (line 1) > assert reslt.exit_code == 0 +``` + +When the command itself raises, the exception is reported against the manifest: the command, the +exception, and the line of the `run`, `pre_run` or `post_run` block it was raised on. + +```text +šŸ’„ command `add` raised KeyError: 'notes' + run, line 2: data["notes"].append(text) +``` + +No Python traceback is printed. The assertions still run, so a test can expect the exception with +`assert isinstance(result.exception, KeyError)`. To see the full traceback of the command as well: + +```console +$ cli test testing.yaml --traceback +``` + The run ends with one of three messages: | Last line | Meaning | Exit code | @@ -101,7 +122,8 @@ step of its own. - **The counter counts test cases.** `āœ… 2 of 5` is the second test case of five. Shell steps are run but not counted, and a failed case keeps its number. - **Shell steps are not checked.** A step that exits with an error does not fail the run. Assert on - the state it should have produced in the next test case. + the state it should have produced in the next test case. A step whose program cannot be started at + all (it does not exist) ends the run with an error. ## The complete example diff --git a/docs/project/changelog.md b/docs/project/changelog.md index 323d53d..4c1feea 100644 --- a/docs/project/changelog.md +++ b/docs/project/changelog.md @@ -69,6 +69,21 @@ dependency versions. Read the [migration notes](migration.md#from-06x-to-070) be removed with `cli rm`. - `make generate-clis` renders `examples/generated/` with `scripts/examples.py` and no longer installs packages. The `generate-cleanup` target is gone. +- **`cli render` prints exactly the generated module.** Long lines are no longer broken, so + `cli render x.yaml > x.py` is the code that runs. The `# Rendered ...` line goes to stderr. The same + holds for `cli init --render`. +- **`cli test` reports errors against the manifest instead of printing tracebacks.** A command that + raises is shown as `command raised : ` with the line of its run block; an + assertion that raises or is not valid Python is shown with its line; a manifest that does not import + and a shell step that cannot be started end the run with an error message. +- **`cli remove` removes a CLI completely or not at all.** When the launcher is not the file cliffy + wrote, nothing is removed, the CLI stays loaded and the command exits with 1. `cli remove-all` exits + with 1 when a CLI could not be removed. +- **`cli load` only replaces a launcher it wrote itself**: a regular file with exactly the launcher + text for that CLI. A file that merely contained `from cliffy.clis.` used to count as cliffy's own. +- A `requires` entry that starts with `-` is rejected. `requires` lists packages. +- `cli dev` runs one reload at a time. A second save within a second of the first is no longer + dropped. ### Added @@ -84,11 +99,20 @@ dependency versions. Read the [migration notes](migration.md#from-06x-to-070) be CLI or of a group, for both backends. - `vars` values can be numbers and booleans, and a var can be built from other vars in any order. - `cli dev` also watches included manifests and the Python file of a file blueprint. +- The manifest's `examples` are shown at the end of the `--help` of the generated CLI, for both + backends. See [Generating docs](../guides/generating-docs.md#add-examples). +- `cli test --traceback` prints the full Python traceback of a command that raises. ### Security - Text from a manifest (help, names, defaults, versions) is escaped in the generated code and names are validated, so manifest data cannot change the code that is generated. +- The name given to `cli remove`, `update`, `info`, `build` and `docs` is validated. A name such as + `../x` could select and delete files outside of cliffy's own directories. +- Launchers and generated modules are written to a temporary file and renamed into place, a new + launcher is only created when nothing has its name, and symbolic links are never followed. +- `cli build` never passes a `requires` entry to pip as an option. An entry such as + `--index-url=...` used to change where pip installs from. ### Removed @@ -108,6 +132,13 @@ dependency versions. Read the [migration notes](migration.md#from-06x-to-070) be - A Jinja error in a manifest is reported with file, line and the text of the line instead of a traceback. - The field descriptions in the JSON schema and the `cli init` template were wrong in places. +- `cli dev`: a reload that started earlier can no longer finish last and load an older version of the + manifest over a newer one. +- `cli dev --run-cli` and `cli test` on several threads no longer delete each other's temporary files. + Each run has a temporary directory of its own. +- The MCP example blueprint answers a JSON-RPC message that is not an object, or a batch, with an + error instead of exiting. +- Releases: the upload to PyPI can be re-run after it stopped half way. - cliffy type-checks and its test suite passes without the optional extras. The test suite no longer extracts zipapps into `~/.shiv`. - Compatibility with Click 8.2 and later, rich-click 1.9 and Python 3.12 and later (deprecated APIs diff --git a/docs/project/known-issues.md b/docs/project/known-issues.md index 7835c7a..219b563 100644 --- a/docs/project/known-issues.md +++ b/docs/project/known-issues.md @@ -18,8 +18,31 @@ problem against the command it comes from. ### `cli test` does not check shell steps -A `$` or `>` step that exits with an error does not fail the run, and a step whose program does not -exist ends the run with a traceback. See [Testing manifests](../guides/testing.md#things-to-know). +A `$` or `>` step that exits with an error does not fail the run. A step whose program does not +exist ends the run with an error message. See [Testing manifests](../guides/testing.md#things-to-know). + +### Examples are shown for the CLI, not per command + +`examples` belongs to the manifest. The generated CLI shows all of them at the end of its own +`--help`; the help of a single command has no examples section, because a command has no `examples` +key. With the Typer backend an example that contains an emoji code such as `:rocket:` is shown as +the emoji, like the rest of the help text. + +### `cli remove` refuses when the launcher was changed + +The launcher of a loaded CLI is only deleted while it is exactly the file cliffy wrote. After an +edit, or when another program replaced it, `cli remove` and `cli remove-all` remove nothing for that +CLI and exit with 1. + +**Do this instead:** move or delete the file named in the message, then run `cli remove` again. + +### A file can still appear between cliffy's check and its write + +cliffy checks a launcher right before it replaces or deletes it, never follows a symbolic link and +creates a new launcher only when nothing has the name. Replacing or deleting an existing launcher +is a check followed by a rename or an unlink: a file that another process puts at exactly that path +in between is replaced or removed. This needs write access to the environment's `bin` directory at +that moment. ### `cli init` without `--raw` rejects names with a hyphen diff --git a/docs/quickstart.md b/docs/quickstart.md index 358fac5..7292731 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -223,7 +223,8 @@ not need cliffy. With `--target script` you get a single Python script instead, --8<-- "docs/snippets/notes.yaml" ``` -The `examples` section at the end is used by [`cli docs`](guides/generating-docs.md). +The `examples` section at the end is shown at the end of `notes --help` and is used by +[`cli docs`](guides/generating-docs.md). ## Next steps diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 49ba08d..014604f 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -450,6 +450,7 @@ cli test [OPTIONS] MANIFEST |---|---|---|---|---| | `MANIFEST` | file | yes | | Manifest file whose `tests` section should run. | | `-x`, `--exitfirst` | flag | no | | exit instantly on first error or failed test. | +| `--traceback` | flag | no | | Also print the full Python traceback of a command that raises. | | `-b`, `--blueprint` | text | no | | Blueprint that maps a custom YAML spec onto a CLI: a .py file, module:Class or an installed blueprint name. Overrides the `blueprint:` key of the file. | Runs the `tests` section of a manifest and prints each command, its output and the result. The exit diff --git a/docs/reference/examples.md b/docs/reference/examples.md index 6f9dc1f..3516510 100644 --- a/docs/reference/examples.md +++ b/docs/reference/examples.md @@ -30,7 +30,7 @@ described in [Blueprints](../blueprints.md#examples). | [todo.yaml](#todo) | Typer | `tabulate`, `rich` | no | | [requires.yaml](#requires) | Typer | `requests >= 2.30.0`, `six < 1.0.0` | no | | [callbacks.yaml](#callbacks) | Typer | nothing | yes | -| [click_hello.yaml](#clickhello) | Click | nothing | no | +| [click_hello.yaml](#clickhello) | Click | nothing | yes | | [aiprompt.yaml](#aiprompt) | Typer | `openai>=1.0.0`, `rich>=13.0.0`, `keyring>=24.0.0` | no | ## hello diff --git a/docs/reference/manifest.md b/docs/reference/manifest.md index 4025ae8..2353356 100644 --- a/docs/reference/manifest.md +++ b/docs/reference/manifest.md @@ -33,7 +33,7 @@ The top level of a manifest file. | `callbacks` | list of [Callback](#callback) | `[]` | Code that runs before any command of the CLI, or of one command group. Callbacks for the same target run in the order given and their params are combined. The built-in --version and --aliases options are kept. | | `cli_options` | mapping | `{}` | Additional CLI configuration options | | `tests` | list of (string or mapping of name to string) | `[]` | Test cases for commands | -| `examples` | list of (string or [Example](#example)) | `[]` | Example command usages to display in generated CLI docs. | +| `examples` | list of (string or [Example](#example)) | `[]` | Example command lines of the CLI, with an optional description each. Shown at the end of the `--help` of the generated CLI and in the pages written by `cli docs`. | - `name` and `version` are the only required keys. `version` must be a string, so quote values such as `"1.0"`. `commands` can be left out for a CLI that only has `callbacks`. @@ -181,8 +181,9 @@ an existing command group. See [Callbacks](../blueprints.md#callbacks). | `command` | string | **required** | The command line to show. | | `description` | string or null | `null` | A sentence describing what the command does. | -`examples` entries are only used by [`cli docs`](../guides/generating-docs.md). An entry can also be a -plain string, which is shown without a description. +`examples` entries are shown at the end of the `--help` of the generated CLI, as +`$ # `, and in the pages written by [`cli docs`](../guides/generating-docs.md). +An entry can also be a plain string, which is shown without a description. ## Include files diff --git a/examples/blueprints/mcp_tools.py b/examples/blueprints/mcp_tools.py index f106a20..a623827 100644 --- a/examples/blueprints/mcp_tools.py +++ b/examples/blueprints/mcp_tools.py @@ -48,11 +48,30 @@ def _call_tool(name, arguments): return _render_result(TOOLS[name]["function"](**arguments)) +def _error(message_id, code, text): + return {"jsonrpc": "2.0", "id": message_id, "error": {"code": code, "message": text}} + + def _handle_message(message): """Answer one JSON-RPC message. Returns None for notifications, which get no reply.""" - method, params, message_id = message.get("method"), message.get("params") or {}, message.get("id") + if isinstance(message, list): + # MCP has no batches since revision 2025-06-18: one message per line + return _error(None, -32600, "Invalid Request: batch messages are not supported, send one message per line") + if not isinstance(message, dict): + return _error(None, -32600, "Invalid Request: a message must be a JSON object") + + method, params, message_id = message.get("method"), message.get("params"), message.get("id") + if not isinstance(method, str): + # a response to a request is not answered: this server never sends one + if "result" in message or "error" in message: + return None + return _error(message_id, -32600, "Invalid Request: `method` must be a string") if message_id is None: return None + if params is None: + params = {} + if not isinstance(params, dict): + return _error(message_id, -32602, "Invalid params: `params` must be an object") if method == "initialize": result = { @@ -65,13 +84,18 @@ def _handle_message(message): elif method == "tools/list": result = {"tools": _describe_tools()} elif method == "tools/call": + arguments = params.get("arguments") + if arguments is None: + arguments = {} + if not isinstance(arguments, dict): + return _error(message_id, -32602, "Invalid params: `arguments` must be an object") try: - text, failed = _call_tool(params.get("name"), params.get("arguments") or {}), False + text, failed = _call_tool(params.get("name"), arguments), False except Exception as e: text, failed = f"{type(e).__name__}: {e}", True result = {"content": [{"type": "text", "text": text}], "isError": failed} else: - return {"jsonrpc": "2.0", "id": message_id, "error": {"code": -32601, "message": f"Method not found: {method}"}} + return _error(message_id, -32601, f"Method not found: {method}") return {"jsonrpc": "2.0", "id": message_id, "result": result} @@ -80,9 +104,11 @@ def _serve(): if not line.strip(): continue try: - reply = _handle_message(json.loads(line)) + message = json.loads(line) except ValueError: - reply = {"jsonrpc": "2.0", "id": None, "error": {"code": -32700, "message": "Parse error"}} + reply = _error(None, -32700, "Parse error") + else: + reply = _handle_message(message) if reply is not None: sys.stdout.write(json.dumps(reply) + "\\n") sys.stdout.flush() diff --git a/examples/click_hello.yaml b/examples/click_hello.yaml index 507bc78..d8a56dd 100644 --- a/examples/click_hello.yaml +++ b/examples/click_hello.yaml @@ -6,4 +6,11 @@ help: Hello World with Click! commands: hello | hl: $ echo "world" +examples: + - command: clickhello hello + description: Print "world". + - clickhello hl +tests: + - hello: assert result.exit_code == 0 + - --help: assert "Examples:" in result.output and "$ clickhello hl" in result.output diff --git a/examples/cliffy_schema.json b/examples/cliffy_schema.json index 34e3572..9678390 100644 --- a/examples/cliffy_schema.json +++ b/examples/cliffy_schema.json @@ -549,7 +549,7 @@ }, "examples": { "default": [], - "description": "Example command usages to display in generated CLI docs.", + "description": "Example command lines of the CLI, with an optional description each. Shown at the end of the `--help` of the generated CLI and in the pages written by `cli docs`.", "items": { "anyOf": [ { diff --git a/examples/generated/clickhello.py b/examples/generated/clickhello.py index 760c3b3..15a7640 100644 --- a/examples/generated/clickhello.py +++ b/examples/generated/clickhello.py @@ -14,7 +14,7 @@ def show_aliases(ctx, param, value): print("\nCommand Aliases\n-------- --------\nhello hl\n") ctx.exit() -@click.group(context_settings=CONTEXT_SETTINGS) +@click.group(context_settings=CONTEXT_SETTINGS, epilog="Examples:\n\n$ clickhello hello # Print \"world\".\n\n$ clickhello hl") @click.version_option('0.1.0') @click.option('--aliases', is_flag=True, callback=show_aliases, expose_value=False, is_eager=True, help='Show command aliases.') diff --git a/examples/generated/hello.py b/examples/generated/hello.py index 2e4cb4f..5f52c5c 100644 --- a/examples/generated/hello.py +++ b/examples/generated/hello.py @@ -4,7 +4,7 @@ import typer CONTEXT_SETTINGS = dict(help_option_names=['-h', '--help']) -cli = typer.Typer(context_settings=CONTEXT_SETTINGS, help="Hello world!") +cli = typer.Typer(context_settings=CONTEXT_SETTINGS, epilog="Examples:\n\n$ hello shell # Say hello from a shell command.\n\n$ hello python # Say hello from Python.", help="Hello world!") __version__ = "0.1.0" __cli_name__ = "hello" diff --git a/examples/hello.yaml b/examples/hello.yaml index c381403..4ad250e 100644 --- a/examples/hello.yaml +++ b/examples/hello.yaml @@ -8,8 +8,15 @@ commands: shell: $echo "hello from shell" python: print("hello from python") +examples: + - command: hello shell + description: Say hello from a shell command. + - command: hello python + description: Say hello from Python. + tests: - shell: assert result.exit_code == 0 - python: assert result.output == "hello from python\n" - shell --help: assert "Show this message and exit" in result.output - - -h: assert "Hello world!" in result.output \ No newline at end of file + - -h: assert "Hello world!" in result.output + - -h: assert "Examples:" in result.output and "$ hello shell" in result.output diff --git a/scripts/docs.py b/scripts/docs.py index 0f1a317..b0f82eb 100644 --- a/scripts/docs.py +++ b/scripts/docs.py @@ -530,8 +530,9 @@ def render_cli_reference() -> str: Click. With the Click backend `rich_help_panel` needs rich-click. """, "Example": """ -`examples` entries are only used by [`cli docs`](../guides/generating-docs.md). An entry can also be a -plain string, which is shown without a description. +`examples` entries are shown at the end of the `--help` of the generated CLI, as +`$ # `, and in the pages written by [`cli docs`](../guides/generating-docs.md). +An entry can also be a plain string, which is shown without a description. """, "IncludeManifest": """ Files listed under `includes` are validated against this smaller model. A relative path is looked up diff --git a/tests/test_blueprint.py b/tests/test_blueprint.py index 1a44dd9..0330cf4 100644 --- a/tests/test_blueprint.py +++ b/tests/test_blueprint.py @@ -503,3 +503,41 @@ def test_example_mcp_blueprint_rejects_bad_specs(): required_flag = {**tool, "parameters": [{"name": "flag", "type": "boolean", "required": True}]} with pytest.raises(BlueprintError, match="a boolean parameter is a flag and cannot be required"): build_manifest(blueprint_cls, {"name": "x", "tools": {"ok": required_flag}}) + + +def test_example_mcp_server_keeps_serving_after_invalid_messages(): + # valid JSON that is not an object raised AttributeError in the server, which then exited + lines = [ + "[]", + '[{"jsonrpc": "2.0", "id": 1, "method": "ping"}]', + '"text"', + "42", + "null", + '{"id": 2}', + '{"jsonrpc": "2.0", "id": 3, "method": "tools/list", "params": [1]}', + '{"jsonrpc": "2.0", "id": 4, "method": "tools/call", "params": {"name": "repeat", "arguments": "ab"}}', + "{not json", + # no reply: a notification, and a response to a request the server never made + '{"jsonrpc": "2.0", "method": "notifications/initialized"}', + '{"jsonrpc": "2.0", "id": 5, "result": {}}', + '{"jsonrpc": "2.0", "id": 6, "method": "ping"}', + ] + + result = CliRunner().invoke(cli, ["run", str(EXAMPLES / "text-tools.yaml"), "--", "serve"], input="\n".join(lines)) + + assert result.exit_code == 0, result.output + replies = [json.loads(line) for line in result.stdout.splitlines()] + assert [(reply["id"], reply.get("error", {}).get("code")) for reply in replies] == [ + (None, -32600), + (None, -32600), + (None, -32600), + (None, -32600), + (None, -32600), + (2, -32600), + (3, -32602), + (4, -32602), + (None, -32700), + (6, None), + ] + assert "batch" in replies[1]["error"]["message"] + assert replies[-1] == {"jsonrpc": "2.0", "id": 6, "result": {}} diff --git a/tests/test_builder.py b/tests/test_builder.py index 840024e..2656b6c 100644 --- a/tests/test_builder.py +++ b/tests/test_builder.py @@ -4,6 +4,7 @@ import subprocess import sys import tempfile +import threading import zipfile from importlib import metadata from pathlib import Path @@ -12,6 +13,7 @@ import pytest from click.testing import CliRunner +import cliffy.helper as IMPORT_OWNER from cliffy.builder import ( build_cli, build_cli_from_manifest, @@ -21,7 +23,6 @@ run_cli, ) from cliffy.cli import cli -from cliffy.helper import TEMP_FILES from cliffy.transformer import Transformer EXAMPLES = Path(__file__).parent.parent / "examples" / "blueprints" @@ -51,7 +52,9 @@ def pip_requirements(mock_pip_run: MagicMock) -> list[str]: pip_args = mock_pip_run.call_args.args[0] assert pip_args[:3] == [sys.executable, "-m", "pip"] assert "install" in pip_args - return pip_args[pip_args.index("--target") + 2 :] + # the requirements follow `--`, which ends the options of pip + assert pip_args[pip_args.index("--target") + 2] == "--" + return pip_args[pip_args.index("--") + 1 :] def run_artifact( @@ -310,7 +313,41 @@ def test_run_cli_removes_temp_file_when_import_fails(temp_dir): run_cli("broken", "import no_such_module_for_cliffy", ()) assert glob.glob(f"{temp_dir}/broken_*") == [] - assert TEMP_FILES == [] + # the run has a temporary directory of its own: neither the file nor the directory is left behind + assert list(temp_dir.iterdir()) == [] + + +def test_concurrent_runs_keep_their_own_temp_files(temp_dir, monkeypatch): + # every run cleaned up a process-wide list of temp files: a run that finished deleted the file + # another run, i.e. a reload of `cli dev --run-cli` on another thread, was about to import + first_is_importing, second_is_done = threading.Event(), threading.Event() + real_import = IMPORT_OWNER.import_module_from_path + + def slow_import(path): + if os.path.basename(path).startswith("first"): + first_is_importing.set() + assert second_is_done.wait(10) + return real_import(path) + + monkeypatch.setattr(IMPORT_OWNER, "import_module_from_path", slow_import) + monkeypatch.setattr(sys, "argv", list(sys.argv)) + errors: list[BaseException] = [] + + def first_run(): + try: + run_cli("first", "cli = lambda: None", ()) + except BaseException as e: + errors.append(e) + + thread = threading.Thread(target=first_run) + thread.start() + assert first_is_importing.wait(10) + run_cli("second", "cli = lambda: None", ()) + second_is_done.set() + thread.join(10) + + assert errors == [] + assert list(temp_dir.iterdir()) == [] def test_build_removes_temp_file_when_build_fails(temp_dir, tmp_path): @@ -327,7 +364,6 @@ def test_build_removes_temp_file_when_build_fails(temp_dir, tmp_path): assert glob.glob(f"{temp_dir}/buildme_*") == [] # the script is written into a temporary directory: neither it nor the directory is left behind assert list(temp_dir.iterdir()) == [] - assert TEMP_FILES == [] @pytest.mark.parametrize( @@ -372,3 +408,64 @@ def test_build_creates_nested_output_dir(tmp_path): assert result.exit_code == 0, result.output assert (output_dir / "script").exists() + + +OPTION_LIKE = ["--index-url=https://packages.invalid/simple", "-r/etc/hosts", " -e .", "--no-deps"] + + +@pytest.mark.parametrize("requirement", OPTION_LIKE) +def test_requirement_that_is_an_option_is_rejected_by_the_manifest(tmp_path, requirement): + # `cli build` does not check `requires` against the installed packages, so the entry went to pip + # as it was: an option, which changed where pip installs from + manifest = tmp_path / "optioned.yaml" + manifest.write_text(f"name: optioned\nversion: '1'\nrequires: [{requirement!r}, rich]\ncommands:\n a: print(1)\n") + + with patch("cliffy.builder.subprocess.run") as run: + result = CliRunner().invoke(cli, ["build", str(manifest), "-o", str(tmp_path / "out")]) + + assert result.exit_code == 1 + run.assert_not_called() + assert f"`{requirement.strip()}` is an option, not a requirement" in result.output + assert "requires" in result.output + assert not (tmp_path / "out").exists() + + +@pytest.mark.parametrize("requirement", OPTION_LIKE) +def test_requirement_that_is_an_option_is_rejected_in_an_include(tmp_path, requirement): + (tmp_path / "shared.yaml").write_text(f"requires: [{requirement!r}]\n") + manifest = tmp_path / "optioned.yaml" + manifest.write_text("name: optioned\nversion: '1'\nincludes: [shared.yaml]\ncommands:\n a: print(1)\n") + + result = CliRunner().invoke(cli, ["render", str(manifest)]) + + assert result.exit_code == 1 + assert f"`{requirement.strip()}` is an option, not a requirement" in result.output + + +@pytest.mark.parametrize("requirement", OPTION_LIKE) +def test_build_does_not_pass_an_option_to_pip(tmp_path, requirement): + # the requirements of a loaded CLI come from its metadata record, not from a validated manifest + script = tmp_path / "demo.py" + script.write_text(TYPER_CODE) + + with patch("cliffy.builder.subprocess.run") as run: + result = build_cli( + "demo", script_path=str(script), deps=["rich", requirement], output_dir=str(tmp_path / "out") + ) + + run.assert_not_called() + assert result.exit_code == 1 + assert f"`{requirement.strip()}` is an option, not a requirement" in result.output + + +def test_requirements_follow_the_end_of_pip_options(tmp_path): + script = tmp_path / "demo.py" + script.write_text(TYPER_CODE) + + with skip_pip() as run: + build_cli("demo", script_path=str(script), deps=["rich>=13"], output_dir=str(tmp_path / "out")) + + command = run.call_args.args[0] + assert command[command.index("--") + 1] == "rich>=13" + assert all(not argument.startswith("-") for argument in command[command.index("--") + 1 :]) + assert "shell" not in run.call_args.kwargs diff --git a/tests/test_cli.py b/tests/test_cli.py index dc4353b..83a2c8e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -7,6 +7,7 @@ from pathlib import Path from unittest.mock import MagicMock, patch +import click import pytest from click.testing import CliRunner @@ -28,6 +29,7 @@ from cliffy.homer import get_metadata from cliffy.loader import Loader from cliffy.manifest import CLIMetadata +from cliffy.transformer import Transformer ANSI_RE = re.compile(r"(?:\x1B[@-_]|[\x80-\x9F])[0-?]*[ -/]*[@-~]") @@ -471,3 +473,68 @@ def test_init_template_loads_runs_and_passes_its_own_tests(isolated_fs, flags): tested = runner.invoke(manifest_test_command, ["mycli.yaml"]) assert "All tests passed" in tested.output assert tested.exit_code == 0 + + +LONG_LINE = "print('" + "long line " * 30 + "')" +WIDE_MANIFEST = f"name: wide\nversion: '1'\nhelp: {'wide help ' * 20}\ncommands:\n a: {LONG_LINE}\n" + + +def generated_code(manifest_path) -> str: + with open(manifest_path, encoding="utf-8") as manifest_io: + return Transformer(manifest_io).cli.code + + +def test_render_prints_exactly_the_generated_code(tmp_path): + # the code was printed through rich with overflow="fold": lines longer than the terminal (80 columns + # when redirected) were cut in two, so `cli render x.yaml > x.py` was not the code that runs + manifest_path = tmp_path / "wide.yaml" + manifest_path.write_text(WIDE_MANIFEST) + + result = CliRunner().invoke(render_command, [str(manifest_path)]) + + assert result.exit_code == 0, result.output + assert result.stdout == generated_code(manifest_path) + assert f" {LONG_LINE}" in result.stdout.splitlines() + compile(result.stdout, "rendered", "exec") + # the status line is not part of the code + assert "Rendered wide CLI v1" in result.stderr + assert "Rendered" not in result.stdout + + +def test_render_redirected_to_a_file_is_the_module(tmp_path): + manifest_path = tmp_path / "wide.yaml" + manifest_path.write_text(WIDE_MANIFEST) + rendered_path = tmp_path / "rendered.py" + + with open(rendered_path, "wb") as rendered: + subprocess.run( + [sys.executable, "-m", "cliffy", "render", str(manifest_path)], + stdout=rendered, + stderr=subprocess.PIPE, + check=True, + env={**os.environ, "PYTHONIOENCODING": "utf-8", "COLUMNS": "40", "FORCE_COLOR": "1"}, + ) + + assert rendered_path.read_bytes() == generated_code(manifest_path).encode("utf-8") + ran = subprocess.run([sys.executable, str(rendered_path), "a"], capture_output=True, text=True) + assert ran.returncode == 0, ran.stderr + assert ran.stdout.strip() == ("long line " * 30).strip() + + +def test_render_on_a_terminal_does_not_break_lines(tmp_path, monkeypatch): + manifest_path = tmp_path / "wide.yaml" + manifest_path.write_text(WIDE_MANIFEST) + monkeypatch.setattr("cliffy.cli.stdout_is_terminal", lambda: True) + + result = CliRunner().invoke(render_command, [str(manifest_path)], env={"COLUMNS": "40"}) + + assert result.exit_code == 0, result.output + assert click.unstyle(result.stdout) == generated_code(manifest_path) + + +def test_init_render_prints_the_template_as_it_is_written(isolated_fs): + rendered = CliRunner().invoke(init_command, ["wide", "--render"], env={"COLUMNS": "40"}) + written = CliRunner().invoke(init_command, ["wide"]) + + assert rendered.exit_code == 0 and written.exit_code == 0 + assert rendered.stdout == (isolated_fs / "wide.yaml").read_text() diff --git a/tests/test_codegen.py b/tests/test_codegen.py index 53a2323..827bf51 100644 --- a/tests/test_codegen.py +++ b/tests/test_codegen.py @@ -8,6 +8,7 @@ import textwrap from importlib import metadata +import click import pytest from click.testing import CliRunner as ClickCliRunner from typer.testing import CliRunner as TyperCliRunner @@ -214,7 +215,11 @@ def test_manifest_text_is_not_executed(run, monkeypatch, manifest): "['a', 'b']", id="builtin_command", ), - pytest.param(BASE + "commands:\n print: print('printed')\n", ["--version"], "t, 1", id="print_command"), + pytest.param(BASE + "commands:\n print: print('printed')\n", ["print"], "printed", id="print_command"), + # --version is implemented with print(): a command named `print` must not replace it + pytest.param( + BASE + "commands:\n print: print('printed')\n", ["--version"], "t, 1", id="print_command_version" + ), pytest.param(BASE + "commands:\n my-group.sub: print(1)\n", ["my-group", "sub"], "1", id="dash_group"), pytest.param(BASE + "commands:\n 2fa.setup: print(1)\n", ["2fa", "setup"], "1", id="digit_group"), pytest.param( @@ -554,3 +559,114 @@ def test_generated_code_is_reproducible(generate, base): assert first == second assert first.splitlines()[0] == f"## Generated t with cliffy {metadata.version('cliffy')}" assert not re.search(r"\d{4}-\d{2}-\d{2}", first) + + +EXAMPLES = """\ +examples: + - command: t greet "Ada Lovelace" --shout + description: Greet someone, loudly. + - t list --tag [home] --pattern '*.txt' + - command: 't say "it''s \\\\n" ''''''x''''''' + description: | + Quotes and backslashes + are shown as written. +commands: + greet: print(1) + sub.show: print(2) +""" +# as `help_text` returns them: with single spaces +EXAMPLE_LINES = [ + '$ t greet "Ada Lovelace" --shout # Greet someone, loudly.', + "$ t list --tag [home] --pattern '*.txt'", + "$ t say \"it's \\\\n\" '''x''' # Quotes and backslashes are shown as written.", +] + + +def help_text(result) -> str: + """Help output as plain text on one line per paragraph: styles and line wrapping differ between renderers""" + return " ".join(click.unstyle(result.output).split()) + + +@pytest.mark.parametrize("base", [BASE, CLICK], ids=["typer", "click"]) +def test_examples_are_shown_in_the_help_of_the_cli(run, base): + # SOUL principle 7: "examples in the help". They were only used by `cli docs`. + result = run(base + EXAMPLES, ["--help"]) + + assert result.exit_code == 0, result.output + text = help_text(result) + assert "Examples:" in text + for line in EXAMPLE_LINES: + assert line in text + # after the commands, in the order of the manifest + positions = [text.index(marker) for marker in ("greet", "Examples:", *EXAMPLE_LINES)] + assert positions == sorted(positions) + + +@pytest.mark.parametrize("base", [BASE, CLICK], ids=["typer", "click"]) +def test_examples_are_not_repeated_in_the_help_of_commands(run, base): + for args in (["greet", "--help"], ["sub", "--help"], ["sub", "show", "--help"]): + result = run(base + EXAMPLES, args) + assert result.exit_code == 0, result.output + assert "Examples:" not in help_text(result) + + +@pytest.mark.parametrize("base", [BASE, CLICK], ids=["typer", "click"]) +def test_help_has_no_examples_section_without_examples(generate, run, base): + manifest = base + "commands:\n greet: print(1)\n" + + assert "Examples" not in help_text(run(manifest, ["--help"])) + assert "epilog" not in generate(manifest).cli.code + assert "epilog" not in generate(manifest + "examples: []\n").cli.code + + +@pytest.mark.parametrize("base", [BASE, CLICK], ids=["typer", "click"]) +def test_examples_follow_an_epilog_from_cli_options(run, base): + manifest = base + "cli_options:\n epilog: Made with cliffy.\n" + EXAMPLES + + text = help_text(run(manifest, ["--help"])) + + assert text.index("Made with cliffy.") < text.index("Examples:") < text.index(EXAMPLE_LINES[0]) + + +@pytest.mark.parametrize("base", [BASE, CLICK], ids=["typer", "click"]) +@pytest.mark.parametrize( + "example", + [ + '\'"+__import__("os").environ.__setitem__("CLIFFY_PWNED","1")+"\'', + '\'"""+__import__("os").environ.__setitem__("CLIFFY_PWNED","1")+"""\'', + '"\\")\\n__import__(\\"os\\").environ.__setitem__(\\"CLIFFY_PWNED\\",\\"1\\")\\n(\\""', + ], + ids=["quote", "triple_quote", "newline"], +) +@pytest.mark.parametrize("field", ["command", "description", "string"]) +def test_example_text_is_not_executed(run, monkeypatch, base, example, field): + monkeypatch.delenv("CLIFFY_PWNED", raising=False) + if field == "string": + examples = f"examples:\n - {example}\n" + elif field == "command": + examples = f"examples:\n - command: {example}\n" + else: + examples = f"examples:\n - command: t a\n description: {example}\n" + + for args in (["a"], ["--help"]): + result = run(base + examples + "commands:\n a: print(1)\n", args) + assert result.exit_code == 0, result.output + + assert "CLIFFY_PWNED" not in os.environ + assert "CLIFFY_PWNED" in help_text(result) + + +@pytest.mark.parametrize("base", [BASE, CLICK], ids=["typer", "click"]) +def test_example_is_generated_as_one_paragraph_with_its_description(generate, base): + code = generate(base + EXAMPLES).cli.code + + assert r"$ t greet \"Ada Lovelace\" --shout # Greet someone, loudly.\n\n$ t list" in code + + +@pytest.mark.parametrize("mode", ["null", "rich"]) +def test_typer_example_brackets_are_shown_whatever_the_markup_mode(run, mode): + # rich markup reads `[home]` as a tag and drops it. Without markup an escaped `\[home]` would show the backslash. + result = run(BASE + f"cli_options:\n rich_markup_mode: {mode}\n" + EXAMPLES, ["--help"]) + + assert result.exit_code == 0, result.output + assert EXAMPLE_LINES[1] in help_text(result) diff --git a/tests/test_helper.py b/tests/test_helper.py index 53ce9ec..49ac7e6 100644 --- a/tests/test_helper.py +++ b/tests/test_helper.py @@ -2,7 +2,6 @@ import platform from datetime import datetime, timedelta from pathlib import Path -from tempfile import NamedTemporaryFile from unittest.mock import MagicMock, patch import pytest @@ -12,12 +11,12 @@ CLIFFY_METADATA_DIR, PYTHON_BIN, PYTHON_EXECUTABLE, - TEMP_FILES, age_datetime, check_requirement, - delete_temp_files, + cli_name_problem, exit_err, get_installed_package_versions, + import_generated_code, import_module_from_path, indent_block, make_executable, @@ -25,6 +24,7 @@ out_err, wrap_as_comment, wrap_as_var, + write_atomically, write_to_file, ) @@ -96,18 +96,24 @@ def test_make_executable(tmpdir): assert os.access(str(temp_file), os.X_OK) -# Test for delete_temp_files -def test_delete_temp_files(): - # Arrange - temp_file = NamedTemporaryFile(delete=False) # noqa: SIM115 - TEMP_FILES.append(temp_file) +def test_import_generated_code_removes_its_file(tmp_path, monkeypatch): + monkeypatch.setattr("tempfile.tempdir", str(tmp_path)) - # Act - delete_temp_files() + module, module_path = import_generated_code("answer = 42", "generated_cli") - # Assert - assert not Path(temp_file.name).exists() - assert TEMP_FILES == [] + assert module.answer == 42 + assert module.__name__ == "generated_cli" and os.path.basename(module_path) == "generated_cli.py" + assert not os.path.exists(module_path) + assert list(tmp_path.iterdir()) == [] + + +def test_import_generated_code_removes_its_file_when_the_import_fails(tmp_path, monkeypatch): + monkeypatch.setattr("tempfile.tempdir", str(tmp_path)) + + with pytest.raises(ImportError): + import_generated_code("import no_such_module_for_cliffy", "generated_cli") + + assert list(tmp_path.iterdir()) == [] # Parametrized tests for indent_block @@ -284,9 +290,49 @@ def test_constants(): assert isinstance(CLIFFY_METADATA_DIR, Path) assert isinstance(PYTHON_BIN, str) assert isinstance(PYTHON_EXECUTABLE, str) - assert isinstance(TEMP_FILES, list) if platform.system() == "Windows": assert "Scripts" in PYTHON_BIN else: assert "bin" in PYTHON_BIN + + +def test_write_atomically_replaces_a_link_instead_of_writing_through_it(tmp_path): + target = tmp_path / "target" + target.write_text("untouched") + link = tmp_path / "link" + try: + link.symlink_to(target) + except (OSError, NotImplementedError) as e: + pytest.skip(f"symbolic links cannot be created here: {e}") + + write_atomically(str(link), "new") + + assert target.read_text() == "untouched" + assert not link.is_symlink() and link.read_text() == "new" + assert sorted(path.name for path in tmp_path.iterdir()) == ["link", "target"] + + +def test_write_atomically_without_replace_keeps_an_existing_file(tmp_path): + existing = tmp_path / "existing" + existing.write_text("kept") + + with pytest.raises(FileExistsError): + write_atomically(str(existing), "new", replace=False) + + assert existing.read_text() == "kept" + assert [path.name for path in tmp_path.iterdir()] == ["existing"] + + write_atomically(str(tmp_path / "sub" / "created"), "new", executable=True, replace=False) + assert (tmp_path / "sub" / "created").read_text() == "new" + assert os.access(tmp_path / "sub" / "created", os.X_OK) + + +@pytest.mark.parametrize("name", ["hello", "my-cli", "_x1"]) +def test_cli_name_problem_accepts_names(name): + assert cli_name_problem(name) is None + + +@pytest.mark.parametrize("name", ["", ".", "..", "../x", "a/b", "a\\b", "-x", "a b", "a.b", "*", "x\n", None]) +def test_cli_name_problem_rejects_paths_and_patterns(name): + assert "not a valid CLI name" in (cli_name_problem(name) or "") diff --git a/tests/test_homer.py b/tests/test_homer.py index f320fce..4d35dd6 100644 --- a/tests/test_homer.py +++ b/tests/test_homer.py @@ -258,3 +258,29 @@ def test_corrupt_metadata_can_be_removed(metadata_dir: Path, command: str): assert result.exit_code == 0, result.output assert not Path(metadata_path).exists() assert list(get_clis()) == [] + + +@pytest.mark.parametrize("name", ["../first", "*/../first", "first/../first", "..", "a/b"]) +def test_metadata_is_not_looked_up_or_removed_by_a_path(metadata_dir: Path, name: str): + # `cli rm ../x` used to select /x.json and delete x.* next to it + save_cli(metadata_dir, "first") + stray = metadata_dir / "first.json" + stray.write_text(Path(str(get_metadata_path("first"))).read_text()) + + assert get_metadata_path(name) is None + assert get_metadata(name) is None + remove_metadata(name) + + assert stray.exists() + assert [cli.cli_name for cli in get_clis()] == ["first"] + + +def test_listed_name_is_the_name_of_the_record_file(metadata_dir: Path): + # the name inside an edited record must not decide which CLI `cli rm-all` removes + save_cli(metadata_dir, "first") + metadata_path = Path(str(get_metadata_path("first"))) + record = json.loads(metadata_path.read_text()) + record["cli_name"] = "../../somewhere-else" + metadata_path.write_text(json.dumps(record)) + + assert [cli.cli_name for cli in get_clis()] == ["first"] diff --git a/tests/test_loader.py b/tests/test_loader.py index 345101a..2b15005 100644 --- a/tests/test_loader.py +++ b/tests/test_loader.py @@ -2,28 +2,51 @@ from pathlib import Path import pytest +from click.testing import CliRunner +from cliffy.cli import cli as cliffy_cli from cliffy.commander import CLI -from cliffy.loader import Loader +from cliffy.homer import get_clis, get_metadata, save_metadata +from cliffy.loader import Loader, LoaderError @pytest.fixture def loader_dirs(monkeypatch, tmp_path): bin_dir = tmp_path / "bin" clis_dir = tmp_path / "clis" + metadata_dir = tmp_path / "metadata" bin_dir.mkdir() clis_dir.mkdir() + metadata_dir.mkdir() monkeypatch.setattr("cliffy.loader.PYTHON_BIN", str(bin_dir)) monkeypatch.setattr("cliffy.loader.CLIFFY_CLI_DIR", clis_dir) + monkeypatch.setattr("cliffy.homer.CLIFFY_METADATA_DIR", metadata_dir) return bin_dir, clis_dir +def load(tmp_path: Path, name: str, code: str = "cli = print") -> None: + """What `cli load` does for a manifest: the launcher, the module and the metadata record.""" + manifest_path = tmp_path / f"{name}.yaml" + manifest_path.write_text(f"name: {name}\n") + cli = CLI(name=name, version="1", code=code) + Loader.load_from_cli(cli) + save_metadata(str(manifest_path), cli) + + +def symlink_or_skip(link: Path, target: Path) -> None: + try: + link.symlink_to(target) + except (OSError, NotImplementedError) as e: + # i.e. Windows without the privilege to create symbolic links + pytest.skip(f"symbolic links cannot be created here: {e}") + + def test_load_does_not_overwrite_foreign_executable(loader_dirs, capsys): bin_dir, clis_dir = loader_dirs # a manifest named `python` or `pip` would land on the interpreter's own executables real_python = bin_dir / "python3.x" real_python.write_bytes(b"\xcf\xfa\xed\xfe real interpreter") - (bin_dir / "python").symlink_to(real_python) + symlink_or_skip(bin_dir / "python", real_python) with pytest.raises(SystemExit): Loader.load_from_cli(CLI(name="python", version="1", code="cli = print")) @@ -34,6 +57,56 @@ def test_load_does_not_overwrite_foreign_executable(loader_dirs, capsys): assert "was not created by cliffy" in capsys.readouterr().err +@pytest.mark.parametrize( + "content", + [ + # a script of someone else that happens to import a loaded CLI + "#!/usr/bin/env python3\nfrom cliffy.clis.other import cli\ncli(['--version'])\n", + # the launcher of another CLI, i.e. a copy + Loader.get_cli_script("other"), + # cliffy's own launcher, edited by hand + Loader.get_cli_script("tool") + "\nprint('my change')\n", + ], + ids=["imports_a_loaded_cli", "launcher_of_another_cli", "edited_launcher"], +) +def test_load_does_not_overwrite_script_it_did_not_write(loader_dirs, capsys, content): + bin_dir, clis_dir = loader_dirs + (bin_dir / "tool").write_text(content) + + with pytest.raises(SystemExit): + Loader.load_from_cli(CLI(name="tool", version="1", code="cli = print")) + + assert (bin_dir / "tool").read_text() == content + assert not (clis_dir / "tool.py").exists() + assert "was not created by cliffy" in capsys.readouterr().err + + +def test_load_does_not_follow_link_to_a_launcher(loader_dirs, tmp_path): + bin_dir, _ = loader_dirs + elsewhere = tmp_path / "elsewhere" + elsewhere.write_text(Loader.get_cli_script("tool")) + symlink_or_skip(bin_dir / "tool", elsewhere) + + with pytest.raises(SystemExit): + Loader.load_from_cli(CLI(name="tool", version="1", code="cli = print")) + + assert (bin_dir / "tool").is_symlink() + assert elsewhere.read_text() == Loader.get_cli_script("tool") + + +def test_load_does_not_replace_file_that_appears_after_the_check(loader_dirs, monkeypatch): + bin_dir, clis_dir = loader_dirs + (bin_dir / "tool").write_text("#!/bin/sh\necho created by someone else, after the check\n") + # the check ran before that file existed + monkeypatch.setattr(Loader, "script_state", staticmethod(lambda script_path, cli_name: "missing")) + + with pytest.raises(SystemExit): + Loader.load_from_cli(CLI(name="tool", version="1", code="cli = print")) + + assert "created by someone else" in (bin_dir / "tool").read_text() + assert not (clis_dir / "tool.py").exists() + + def test_load_replaces_its_own_script(loader_dirs): bin_dir, clis_dir = loader_dirs @@ -43,6 +116,30 @@ def test_load_replaces_its_own_script(loader_dirs): assert "from cliffy.clis.my_cli import cli" in (bin_dir / "my-cli").read_text() assert os.access(bin_dir / "my-cli", os.X_OK) assert (clis_dir / "my_cli.py").read_text() == "cli = repr" + # written to a temporary file next to the target and moved into place + assert sorted(path.name for path in bin_dir.iterdir()) == ["my-cli"] + assert sorted(path.name for path in clis_dir.iterdir()) == ["my_cli.py"] + + +def test_load_replaces_script_written_with_another_interpreter_path(loader_dirs): + bin_dir, _ = loader_dirs + # the same environment reached through another path, i.e. bin/python3 instead of bin/python + body = Loader.get_cli_script("my-cli").split("\n", 1)[1] + (bin_dir / "my-cli").write_text(f"#!/some/venv/bin/python3\n{body}") + + Loader.load_from_cli(CLI(name="my-cli", version="1", code="cli = print")) + + assert (bin_dir / "my-cli").read_text() == Loader.get_cli_script("my-cli") + + +def test_unload_removes_script_and_module(loader_dirs): + bin_dir, clis_dir = loader_dirs + Loader.load_from_cli(CLI(name="my-cli", version="1", code="cli = print")) + + Loader.unload_cli("my-cli") + + assert list(bin_dir.iterdir()) == [] + assert list(clis_dir.iterdir()) == [] def test_unload_removes_module_when_script_is_missing(loader_dirs): @@ -60,6 +157,102 @@ def test_unload_keeps_foreign_executable(loader_dirs): foreign: Path = bin_dir / "pip" foreign.write_text("#!/bin/sh\necho pip\n") - Loader.unload_cli("pip") + with pytest.raises(LoaderError, match="was not created by cliffy"): + Loader.unload_cli("pip") assert foreign.exists() + + +def test_unload_keeps_the_cli_runnable_when_its_script_was_edited(loader_dirs): + bin_dir, clis_dir = loader_dirs + Loader.load_from_cli(CLI(name="my-cli", version="1", code="cli = print")) + edited = (bin_dir / "my-cli").read_text() + "\nprint('my change')\n" + (bin_dir / "my-cli").write_text(edited) + + with pytest.raises(LoaderError): + Loader.unload_cli("my-cli") + + # the module used to be deleted, which left a script that failed with ModuleNotFoundError + assert (bin_dir / "my-cli").read_text() == edited + assert (clis_dir / "my_cli.py").read_text() == "cli = print" + + +def test_unload_puts_the_script_back_when_the_module_cannot_be_removed(loader_dirs, monkeypatch): + bin_dir, clis_dir = loader_dirs + Loader.load_from_cli(CLI(name="my-cli", version="1", code="cli = print")) + real_unlink = os.unlink + + def unlink(path, *args, **kwargs): + if str(path).endswith("my_cli.py"): + raise PermissionError(13, "Permission denied", str(path)) + real_unlink(path, *args, **kwargs) + + monkeypatch.setattr("cliffy.loader.os.unlink", unlink) + + with pytest.raises(LoaderError, match="Permission denied"): + Loader.unload_cli("my-cli") + + assert (bin_dir / "my-cli").read_text() == Loader.get_cli_script("my-cli") + assert os.access(bin_dir / "my-cli", os.X_OK) + assert (clis_dir / "my_cli.py").exists() + + +@pytest.mark.parametrize("command", ["rm", "rm-all"]) +def test_rm_leaves_nothing_half_removed_when_the_script_was_edited(loader_dirs, tmp_path, command): + bin_dir, clis_dir = loader_dirs + load(tmp_path, "kept") + (bin_dir / "kept").write_text("#!/bin/sh\necho mine now\n") + + result = CliRunner().invoke(cliffy_cli, [command] if command == "rm-all" else [command, "kept"]) + + assert result.exit_code == 1, result.output + assert "was not created by cliffy" in result.output + # still listed, and its module is still there: the metadata used to be removed first + assert [cli.cli_name for cli in get_clis()] == ["kept"] + assert (clis_dir / "kept.py").exists() + assert (bin_dir / "kept").read_text() == "#!/bin/sh\necho mine now\n" + + +def test_rm_removes_script_module_and_metadata(loader_dirs, tmp_path): + bin_dir, clis_dir = loader_dirs + load(tmp_path, "gone") + + result = CliRunner().invoke(cliffy_cli, ["rm", "gone"]) + + assert result.exit_code == 0, result.output + assert list(bin_dir.iterdir()) == [] and list(clis_dir.iterdir()) == [] + assert get_metadata("gone") is None + + +@pytest.mark.parametrize("name", ["../outside", "../../outside", "sub/outside", "..", ".", "a\\b", "", "-x", "a b"]) +def test_names_that_are_paths_are_not_cli_names(loader_dirs, name): + with pytest.raises(ValueError, match="not a valid CLI name"): + Loader.get_cli_path(name) + with pytest.raises(ValueError, match="not a valid CLI name"): + Loader.get_cli_script_path(name) + with pytest.raises(ValueError, match="not a valid CLI name"): + Loader.unload_cli(name) + + +@pytest.mark.parametrize("command", ["rm", "update", "info", "build", "docs"]) +@pytest.mark.parametrize("name", ["../outside", "../../outside"]) +def test_commands_do_not_resolve_a_name_outside_of_cliffys_directories(loader_dirs, tmp_path, command, name): + load(tmp_path, "inside") + # what a name with `..` in it would have reached: files one and two levels above the directories + # that hold the launchers, the modules and the metadata records + record = next((tmp_path / "metadata").glob("*/inside.json")).read_text() + outside = [tmp_path / "outside", tmp_path / "outside.py", tmp_path / "outside.json"] + outside += [tmp_path / "metadata" / "outside.json", tmp_path.parent / "outside.json"] + for path in outside: + path.write_text(Loader.get_cli_script("outside") if path.suffix == "" else record) + + try: + result = CliRunner().invoke(cliffy_cli, [command, name]) + + assert result.exit_code == 1, result.output + assert "not a valid CLI name" in result.output + for path in outside: + assert path.exists(), path + finally: + (tmp_path.parent / "outside.json").unlink(missing_ok=True) + assert [cli.cli_name for cli in get_clis()] == ["inside"] diff --git a/tests/test_packaging.py b/tests/test_packaging.py new file mode 100644 index 0000000..a8393b5 --- /dev/null +++ b/tests/test_packaging.py @@ -0,0 +1,10 @@ +"""What the installed package has to carry besides its modules.""" + +from importlib import metadata +from importlib.resources import files + + +def test_package_is_marked_as_typed(): + # without py.typed a type checker ignores the annotations of an installed cliffy (PEP 561) + assert files("cliffy").joinpath("py.typed").is_file() + assert "Typing :: Typed" in (metadata.metadata("cliffy").get_all("Classifier") or []) diff --git a/tests/test_reloader.py b/tests/test_reloader.py index 4f107c0..5b6fbbb 100644 --- a/tests/test_reloader.py +++ b/tests/test_reloader.py @@ -1,4 +1,5 @@ import io +import threading import time from datetime import datetime, timedelta from unittest.mock import MagicMock, patch @@ -481,3 +482,101 @@ def test_watch_schedules_every_watched_directory(mock_sleep, MockObserver, tmp_p scheduled = {call.kwargs["path"] for call in MockObserver.return_value.schedule.call_args_list} assert scheduled == {str(tmp_path.resolve()), str((tmp_path / "bp").resolve())} + + +class FakeTransformer: + """Stands in for the Transformer: the "CLI" is the text of the manifest at the time it was read.""" + + reading = threading.Event() + release_first = threading.Event() + calls = 0 + + def __init__(self, manifest_io, **kwargs): + type(self).calls += 1 + call = type(self).calls + content = manifest_io.read().strip() + self.cli = MagicMock(code=content, version=content) + self.cli.name = "raced" + if call == 1: + # the first reload has read the manifest and is still busy generating + type(self).reading.set() + assert type(self).release_first.wait(10) + + +@pytest.fixture +def fake_transformer(): + FakeTransformer.reading, FakeTransformer.release_first = threading.Event(), threading.Event() + FakeTransformer.calls = 0 + return FakeTransformer + + +@patch("cliffy.reloader.out") +@patch("cliffy.reloader.save_metadata") +@patch("cliffy.reloader.Loader.load_from_cli") +def test_older_reload_does_not_overwrite_a_newer_one(mock_load, mock_save, mock_out, fake_transformer, tmp_path): + # every change started a reload of its own: one that had read the old manifest could load its CLI last + manifest_path = tmp_path / "raced.yaml" + manifest_path.write_text("old") + + with patch("cliffy.reloader.Transformer", fake_transformer): + older = threading.Thread(target=Reloader.reload, args=(str(manifest_path), False, ())) + older.start() + assert fake_transformer.reading.wait(10) + + manifest_path.write_text("new") + newer = threading.Thread(target=Reloader.reload, args=(str(manifest_path), False, ())) + newer.start() + # the newer reload has to wait for the older one. Without that it is done well within this time + newer.join(0.5) + fake_transformer.release_first.set() + older.join(10) + newer.join(10) + + assert not older.is_alive() and not newer.is_alive() + assert [call.args[0].code for call in mock_load.call_args_list][-1] == "new" + assert [call.args[1].code for call in mock_save.call_args_list][-1] == "new" + + +@patch("cliffy.reloader.out") +@patch("cliffy.reloader.save_metadata") +@patch("cliffy.reloader.Loader.load_from_cli") +def test_reloads_queued_behind_a_newer_change_are_skipped(mock_load, mock_save, mock_out, tmp_path): + manifest_path = tmp_path / "queued.yaml" + manifest_path.write_text("name: queued\nversion: '1'\ncommands:\n a: print(1)\n") + reloader = Reloader(str(manifest_path), False, ()) + started: list[dict] = [] + + with patch("threading.Thread") as mock_thread: + for _ in range(3): + reloader.last_modified = datetime.now() - timedelta(seconds=2) + reloader.on_modified(modified(manifest_path)) + started = [call.kwargs for call in mock_thread.call_args_list] + + # three changes were accepted before any of their reloads got to run + assert len(started) == 3 + for thread in started: + thread["target"](*thread["args"], **thread["kwargs"]) + + # the last one reads the manifest as it is now: the two before it have nothing left to do + assert mock_load.call_count == 1 + assert mock_save.call_count == 1 + + +def test_a_second_save_within_the_debounce_time_is_not_lost(tmp_path): + # an event within a second of the last one was dropped as a duplicate, also when the file had + # changed again: the CLI then stayed on the older text until the next save + manifest_path = tmp_path / "quick.yaml" + manifest_path.write_text("name: quick\nversion: '1'\ncommands:\n a: print(1)\n") + reloader = Reloader(str(manifest_path), False, ()) + reloader.last_modified = datetime.now() - timedelta(seconds=2) + + with patch("threading.Thread") as mock_thread: + reloader.on_modified(modified(manifest_path)) + # the same save reported twice, as file system watchers do + reloader.on_modified(modified(manifest_path)) + assert mock_thread.call_count == 1 + + manifest_path.write_text("name: quick\nversion: '2'\ncommands:\n a: print(1)\n b: print(2)\n") + reloader.on_modified(modified(manifest_path)) + reloader.on_modified(modified(manifest_path)) + assert mock_thread.call_count == 2 diff --git a/tests/test_tester.py b/tests/test_tester.py index 0e8fb97..5011449 100644 --- a/tests/test_tester.py +++ b/tests/test_tester.py @@ -7,7 +7,6 @@ from pybash.transformer import transform as transform_bash from cliffy.cli import test_command as manifest_test_command -from cliffy.helper import TEMP_FILES from cliffy.tester import ShellScript from cliffy.tester import Tester as ManifestTester @@ -35,7 +34,8 @@ def test_tester_removes_temp_file_when_import_fails(monkeypatch, tmp_path): ManifestTester(str(manifest_path)) assert glob.glob(f"{temp_dir}/broken_*") == [] - assert TEMP_FILES == [] + # the run has a temporary directory of its own: neither the file nor the directory is left behind + assert list(temp_dir.iterdir()) == [] def test_manifest_tests_run_for_click_cli(tmp_path): @@ -135,3 +135,148 @@ def test_assertions_see_output_without_ansi_styles(tmp_path): assert "All tests passed" in result.output, result.output assert result.exit_code == 0 + + +RAISING_MANIFEST = """\ +name: raising +version: '1' +use_click: {use_click} +functions: + - | + def parse(text): + import json + return json.loads(text) +commands: + boom: + help: Raises on its second line + pre_run: prepared = True + run: | + + value = 1 + raise RuntimeError("the user's own problem") + deep: + run: | + print("before") + parse("{{") + quit: raise SystemExit(3) +tests: +{tests} +""" + + +def run_manifest_tests(tmp_path, tests: str, *args: str, use_click: bool = False, manifest: str = RAISING_MANIFEST): + manifest_path = tmp_path / "raising.yaml" + manifest_path.write_text(manifest.format(use_click=str(use_click).lower(), tests=tests)) + return CliRunner().invoke(manifest_test_command, [str(manifest_path), *args]) + + +def assert_no_traceback(output: str) -> None: + """Nothing of cliffy itself and nothing of the generated module is shown: SOUL principle 3""" + assert "Traceback" not in output, output + assert 'File "' not in output, output + for internal in ("tester.py", "cli.py", "helper.py", "_test.py", "site-packages", "`: nothing said where in the manifest it came from + result = run_manifest_tests(tmp_path, " - boom: assert result.exit_code == 0\n", use_click=use_click) + + assert result.exit_code == 1 + assert "command `boom` raised RuntimeError: the user's own problem" in result.output + # line 3 of the run block as it is written in the manifest: the blank first line counts + assert 'run, line 3: raise RuntimeError("the user\'s own problem")' in result.output + assert "AssertionError: (line 1) > assert result.exit_code == 0" in result.output + assert_no_traceback(result.output) + + +@pytest.mark.parametrize("use_click", [False, True], ids=["typer", "click"]) +def test_exception_from_a_helper_names_the_command_and_the_helper(tmp_path, use_click): + result = run_manifest_tests(tmp_path, " - deep: assert result.exit_code == 0\n", use_click=use_click) + + assert result.exit_code == 1 + assert "command `deep` raised JSONDecodeError: " in result.output + assert 'run, line 2: parse("{")' in result.output + assert "in `parse` (functions): return json.loads(text)" in result.output + assert_no_traceback(result.output) + + +def test_expected_exception_can_still_be_asserted_on(tmp_path): + tests = " - boom: assert isinstance(result.exception, RuntimeError)\n - quit: assert result.exit_code == 3\n" + + result = run_manifest_tests(tmp_path, tests) + + assert result.exit_code == 0, result.output + assert "All tests passed" in result.output + assert "command `boom` raised RuntimeError" in result.output + # an exit code is not an exception of the command + assert "command `quit` raised" not in result.output + assert_no_traceback(result.output) + + +def test_traceback_flag_shows_the_traceback_of_the_command(tmp_path): + result = run_manifest_tests(tmp_path, " - boom: assert result.exit_code == 0\n", "--traceback") + + assert result.exit_code == 1 + assert "command `boom` raised RuntimeError: the user's own problem" in result.output + assert "Traceback (most recent call last)" in result.output + # with the lines of the generated code, although its file is gone + assert 'raise RuntimeError("the user\'s own problem")' in result.output.split("Traceback")[1] + + +@pytest.mark.parametrize( + "assertion, problem", + [ + ("assert undefined_name", "NameError: name 'undefined_name' is not defined (line 1) > assert undefined_name"), + ("assert result.output.nope()", "AttributeError: 'str' object has no attribute 'nope' (line 1) > assert"), + ("assert (", "the assertion is not valid Python, line 1, column 8: '(' was never closed"), + ], + ids=["name_error", "attribute_error", "syntax_error"], +) +def test_broken_assertion_is_reported_without_a_traceback(tmp_path, assertion, problem): + # these printed a traceback through cliffy/cli.py and cliffy/tester.py + tests = f" - quit: {assertion}\n - quit: assert result.exit_code == 3\n" + + result = run_manifest_tests(tmp_path, tests) + + assert result.exit_code == 1 + assert problem in result.output + assert "2 of 2" in result.output + assert_no_traceback(result.output) + + +def test_assertion_error_on_a_later_line_is_located(tmp_path): + tests = " - quit: |\n assert result.exit_code == 3\n assert 'nothing' in result.output\n" + + result = run_manifest_tests(tmp_path, tests) + + assert result.exit_code == 1 + assert "AssertionError: (line 2) > assert 'nothing' in result.output" in result.output + assert_no_traceback(result.output) + + +def test_manifest_that_does_not_import_is_reported_against_the_manifest(tmp_path): + # `cli test` ended with a traceback through cliffy/helper.py that named a deleted temp file + manifest_path = tmp_path / "broken.yaml" + manifest_path.write_text(BROKEN_MANIFEST + "tests:\n - a: assert result.exit_code == 0\n") + + result = CliRunner().invoke(manifest_test_command, [str(manifest_path)]) + + assert result.exit_code == 1 + assert isinstance(result.exception, SystemExit) + assert "module level code (`imports`, `vars`, `types` or `functions`)" in result.output + assert "ModuleNotFoundError: No module named 'no_such_module_for_cliffy'" in result.output + assert "import no_such_module_for_cliffy" in result.output + assert_no_traceback(result.output) + + +def test_shell_step_that_cannot_start_ends_the_run_without_a_traceback(tmp_path): + tests = " - $ no-such-program-for-cliffy --flag\n - quit: assert result.exit_code == 3\n" + + result = run_manifest_tests(tmp_path, tests) + + assert result.exit_code == 1 + assert isinstance(result.exception, SystemExit) + assert "test step `$ no-such-program-for-cliffy --flag` could not be run: FileNotFoundError" in result.output + assert_no_traceback(result.output)