From 98a9a656f8cc6cc2dabf1d58ac796238f730ef93 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 00:17:25 +0100 Subject: [PATCH 01/28] Add coverage env propagation sequence diagram Document how `run_rust.py` derives and forwards Cargo environment overrides during Rust coverage runs, including the optional cucumber.rs follow-up path. Add a plain-language caption so the Mermaid diagram remains accessible when the rendered graphic is unavailable. --- .github/actions/generate-coverage/README.md | 38 +++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/.github/actions/generate-coverage/README.md b/.github/actions/generate-coverage/README.md index 02dcf711..e6f0998c 100644 --- a/.github/actions/generate-coverage/README.md +++ b/.github/actions/generate-coverage/README.md @@ -41,6 +41,44 @@ flowchart TD K --> L[End] ``` + +## Rust coverage environment propagation + +Figure: sequence diagram showing how `run_rust.py` derives coverage-specific +Cargo environment overrides for Cranelift-configured projects and passes them +into `_run_cargo`, including the optional cucumber.rs follow-up run. + +```mermaid +sequenceDiagram + actor GitHubActions + participant run_rust_py as run_rust.py + participant get_cargo_coverage_env + participant _run_cargo + participant cargo + + GitHubActions->>run_rust_py: main(manifest_path, fmt, use_nextest, ...) + run_rust_py->>get_cargo_coverage_env: get_cargo_coverage_env(manifest_path) + get_cargo_coverage_env-->>run_rust_py: cargo_env + run_rust_py->>_run_cargo: _run_cargo(args, env_overrides=cargo_env) + alt env_overrides is not None + _run_cargo->>_run_cargo: merge os_environ with env_overrides + else + _run_cargo->>_run_cargo: use os_environ unchanged + end + _run_cargo->>cargo: invoke cargo llvm-cov with env + cargo-->>_run_cargo: stdout + _run_cargo-->>run_rust_py: stdout + + opt with_cucumber_rs + run_rust_py->>get_cargo_coverage_env: get_cargo_coverage_env(manifest_path) + get_cargo_coverage_env-->>run_rust_py: cargo_env + run_rust_py->>_run_cargo: _run_cargo(cucumber_args, env_overrides=cargo_env) + _run_cargo->>cargo: invoke cargo test with env + cargo-->>_run_cargo: stdout + _run_cargo-->>run_rust_py: stdout + end +``` + ## Inputs | Name | Description | Required | Default | From a32012c44d578988623d71cd9f363400ed737ee5 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 00:19:23 +0100 Subject: [PATCH 02/28] Document Rust coverage env override design Expand the generate-coverage design notes to explain why `get_cargo_coverage_env` and `_run_cargo(..., env_overrides=...)` exist, how the Cranelift-to-LLVM override works, and which limits the current detection strategy still has. Link the action README to the internal design note so reviewers and maintainers can find the architectural rationale from the main action documentation. --- .github/actions/generate-coverage/README.md | 4 + docs/generate-coverage-design.md | 138 ++++++++++++++++++++ 2 files changed, 142 insertions(+) diff --git a/.github/actions/generate-coverage/README.md b/.github/actions/generate-coverage/README.md index e6f0998c..44e661e5 100644 --- a/.github/actions/generate-coverage/README.md +++ b/.github/actions/generate-coverage/README.md @@ -194,4 +194,8 @@ Coverage reports are archived as workflow artefacts named `-` segment. This prevents collisions across matrix jobs and distinguishes runs on different platforms. +Developer-facing design notes, including the rationale for Cranelift coverage +environment overrides, are available in +[`docs/generate-coverage-design.md`](../../../docs/generate-coverage-design.md). + Release history is available in [CHANGELOG](CHANGELOG.md). diff --git a/docs/generate-coverage-design.md b/docs/generate-coverage-design.md index 0b2fbf1b..2aa28b0a 100644 --- a/docs/generate-coverage-design.md +++ b/docs/generate-coverage-design.md @@ -11,8 +11,146 @@ action and the evolution of its supporting scripts. `plumbum`-driven Python subprocess and exposes the composed name to the workflow. The script migrated to `cyclopts` for CLI parsing so additional inputs can be mapped declaratively from the GitHub Actions environment. +- *2026-04-16* — Rust coverage runs now force LLVM via subprocess environment + overrides instead of outer `cargo --config ...` flags when a repository + configures the Cranelift backend. This keeps the action compatible with + `cargo-llvm-cov`, which spawns nested Cargo commands that inherit environment + variables but do not inherit the wrapper process's ad hoc `--config` flags. + +## Rust Coverage Environment Overrides + +### Problem Statement + +Some Rust repositories set: + +```toml +[profile.dev] +codegen-backend = "cranelift" +``` + +to speed up normal local builds. That is a valid repository-level choice, but +it breaks source-based coverage because `-Cinstrument-coverage` only works with +LLVM. Earlier versions of `run_rust.py` tried to work around this by launching +the outer coverage command with: + +```text +cargo --config 'profile.dev.codegen-backend="llvm"' \ + --config 'profile.test.codegen-backend="llvm"' \ + llvm-cov ... +``` + +That looked correct at first glance, but it only affected the wrapper Cargo +process. `cargo-llvm-cov` then spawned nested `cargo test` or `cargo nextest` +processes, and those child processes still read the repository's +`.cargo/config.toml` with `codegen-backend = "cranelift"`. The result was the +same failure the action was supposed to prevent: + +```text +error: `-Cinstrument-coverage` is LLVM specific and not supported by Cranelift +``` + +### Current Design + +`run_rust.py` now splits the behaviour into two explicit pieces: + +1. `get_cargo_coverage_env(manifest_path)` decides whether coverage-specific + Cargo environment overrides are needed. +2. `_run_cargo(args, env_overrides=...)` merges those overrides into the + subprocess environment before invoking Cargo. + +When the target repository does not use Cranelift, `get_cargo_coverage_env()` +returns an empty mapping and the action behaves as before. + +When Cranelift is detected, the helper returns: + +```text +CARGO_PROFILE_DEV_CODEGEN_BACKEND=llvm +CARGO_PROFILE_TEST_CODEGEN_BACKEND=llvm +``` + +These variables are passed to the `cargo llvm-cov` subprocess and therefore +propagate into the nested Cargo commands that perform the actual compilation. + +### Why Environment Variables Instead of `--config` + +The environment-variable approach is intentional rather than incidental: + +- Cargo child processes inherit environment variables by default. +- `cargo-llvm-cov` launches nested Cargo commands internally. +- Those nested commands do not inherit wrapper-only `--config` CLI arguments. +- Coverage therefore needs a transport mechanism that survives process + boundaries. + +Using `CARGO_PROFILE_*_CODEGEN_BACKEND` matches Cargo's configuration model +while keeping the override tightly scoped to the coverage subprocess. The +repository's checked-in configuration stays unchanged, and non-coverage flows +continue to use Cranelift if the repository asked for it. + +### Where the Overrides Apply + +The overrides are applied in both Rust coverage entry points: + +- the main `cargo llvm-cov` run +- the optional cucumber.rs follow-up run when `with-cucumber-rs` is enabled + +Both paths call the same `get_cargo_coverage_env()` helper so the logic does +not drift between the primary and follow-up coverage invocations. + +### Behaviour of `env_overrides` + +`_run_cargo(..., env_overrides=...)` accepts an optional mapping: + +- `None` means "use the inherited process environment unchanged" +- a mapping means "merge the current environment with these extra keys" + +The helper does not construct a clean-room environment. It starts from +`os.environ` and overlays the requested keys, preserving the rest of the +workflow's runtime context such as PATH, toolchain configuration, and any +GitHub Actions environment variables already set by the caller. + +### Cranelift Detection Strategy + +Cranelift detection deliberately reuses the existing lightweight search in +`_uses_cranelift_backend(manifest_path)`: + +- start from the selected Cargo manifest directory +- walk upward towards the filesystem root +- look for `.cargo/config.toml` and `.cargo/config` +- read the file as UTF-8 when possible +- treat any line matching `codegen-backend = "cranelift"` as a signal that + coverage should switch the `dev` and `test` profiles back to LLVM + +This approach is fast, dependency-free, and good enough for the specific +coverage failure mode the action is addressing. + +### Known Limitations + +The current detection path is intentionally simple and has limits developers +should understand: + +- It is text-based rather than TOML-structure-aware. Any matching + `codegen-backend = "cranelift"` assignment triggers the override, even if it + appears in a table that is more specific than the action strictly needs to + reason about. +- It only inspects `.cargo/config.toml` and `.cargo/config` files reachable by + walking upward from the manifest directory. It does not model configuration + injected via CLI `--config`, environment-backed Cargo config, or other + runtime indirection. +- It always applies both `CARGO_PROFILE_DEV_CODEGEN_BACKEND` and + `CARGO_PROFILE_TEST_CODEGEN_BACKEND` together once Cranelift is detected. The + action currently does not try to mirror per-profile granularity from the + repository config. +- Files that cannot be read as UTF-8 are ignored rather than failing the run, + because the action prefers a conservative fallback over blocking coverage for + an unrelated config parse issue. + +If the repository ever needs finer-grained handling, the next step would be a +real TOML parser plus table-aware resolution. The current design intentionally +stops short of that complexity. ## Roadmap - [x] Extend artefact naming to include platform metadata and support custom suffixes. +- [x] Document the Rust coverage environment-override design for + Cranelift-configured repositories. From 20b64e069d91c48d0b95075b38b63564bc79afca Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 00:24:09 +0100 Subject: [PATCH 03/28] Handle manifest-level Cranelift coverage overrides Teach `get_cargo_coverage_env()` to detect Cranelift profile settings in the selected `Cargo.toml` as well as `.cargo/config*`, so coverage still forces LLVM when repositories configure `codegen-backend` directly in manifest profiles. Deflake the Rust coverage happy-path test by clearing inherited `CARGO_PROFILE_*_CODEGEN_BACKEND` values before asserting the spawned Cargo environment, and update the design notes to document the broader detection boundary. --- .../generate-coverage/scripts/run_rust.py | 23 +++++++ .../generate-coverage/tests/test_scripts.py | 64 +++++++++++++++++-- docs/generate-coverage-design.md | 26 ++++---- 3 files changed, 95 insertions(+), 18 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index 4fd7fb26..f914a10b 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -176,6 +176,29 @@ def _uses_cranelift_backend(manifest_path: Path) -> bool: search_dir = parent return False +def _manifest_uses_cranelift_backend(manifest_path: Path) -> bool: + """Return ``True`` when ``manifest_path`` configures Cranelift in profiles.""" + try: + content = manifest_path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return False + + in_profile_section = False + for line in content.splitlines(): + stripped = line.strip() + if not stripped or stripped.startswith("#"): + continue + section_match = re.match(r"^\[(?P
[^\]]+)\]$", stripped) + if section_match: + section = section_match.group("section").strip() + in_profile_section = section == "profile" or section.startswith("profile.") + continue + if not in_profile_section: + continue + if re.match(r'^[ \t]*codegen-backend\s*=\s*["\']cranelift["\']', line): + return True + return False + def get_cargo_coverage_cmd( fmt: str, diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index ab5de733..db4ad2b1 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -211,9 +211,15 @@ def _run_rust_coverage_test( return calls[0].argv, calls[0].env, out, gh -def test_run_rust_success(tmp_path: Path, shell_stubs: StubManager) -> None: +def test_run_rust_success( + tmp_path: Path, + shell_stubs: StubManager, + monkeypatch: pytest.MonkeyPatch, +) -> None: """Happy path for ``run_rust.py``.""" - cargo_args, cargo_env, out, gh = _run_rust_coverage_test( + monkeypatch.delenv("CARGO_PROFILE_DEV_CODEGEN_BACKEND", raising=False) + monkeypatch.delenv("CARGO_PROFILE_TEST_CODEGEN_BACKEND", raising=False) + cargo_call, out, gh = _run_rust_coverage_call( tmp_path, shell_stubs, RustCoverageConfig( @@ -222,6 +228,7 @@ def test_run_rust_success(tmp_path: Path, shell_stubs: StubManager) -> None: with_default_features=False, ), ) + cargo_args = cargo_call.argv expected_args = [ "llvm-cov", "--manifest-path", @@ -236,8 +243,8 @@ def test_run_rust_success(tmp_path: Path, shell_stubs: StubManager) -> None: str(out), ] assert cargo_args == expected_args - assert cargo_env.get("CARGO_PROFILE_DEV_CODEGEN_BACKEND") is None - assert cargo_env.get("CARGO_PROFILE_TEST_CODEGEN_BACKEND") is None + assert "CARGO_PROFILE_DEV_CODEGEN_BACKEND" not in cargo_call.env + assert "CARGO_PROFILE_TEST_CODEGEN_BACKEND" not in cargo_call.env data = gh.read_text().splitlines() assert f"file={out}" in data @@ -1692,3 +1699,52 @@ def raise_permission_error(*_: object, **__: object) -> object: with pytest.raises(run_python_module.typer.Exit) as excinfo: run_python_module.get_line_coverage_percent_from_cobertura(xml) assert _exit_code(excinfo.value) == 1 + +def test_get_cargo_coverage_env_cranelift_variants( + tmp_path: Path, run_rust_module: ModuleType +) -> None: + """Config or manifest Cranelift settings get overrides; LLVM repos do not.""" + cargo_config_dir = tmp_path / ".cargo" + cargo_config_dir.mkdir() + cargo_config = cargo_config_dir / "config.toml" + manifest_path = tmp_path / "Cargo.toml" + manifest_path.write_text("[package]\nname = 'demo'\nversion = '0.1.0'\n") + + cargo_config.write_text( + "[profile.dev]\ncodegen-backend = 'cranelift'\n", + encoding="utf-8", + ) + assert run_rust_module.get_cargo_coverage_env(manifest_path) == { + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + } + + cargo_config.write_text( + "[profile.dev]\ncodegen-backend = 'llvm'\n", encoding="utf-8" + ) + manifest_path.write_text( + "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" + '[profile.dev]\ncodegen-backend = "cranelift"\n', + encoding="utf-8", + ) + assert run_rust_module.get_cargo_coverage_env(manifest_path) == { + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + } + + manifest_path.write_text( + "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" + "[profile.test]\ncodegen-backend = 'cranelift'\n", + encoding="utf-8", + ) + assert run_rust_module.get_cargo_coverage_env(manifest_path) == { + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + } + + manifest_path.write_text( + "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" + "[profile.dev]\ncodegen-backend = 'llvm'\n", + encoding="utf-8", + ) + assert run_rust_module.get_cargo_coverage_env(manifest_path) == {} diff --git a/docs/generate-coverage-design.md b/docs/generate-coverage-design.md index 2aa28b0a..b83d8b77 100644 --- a/docs/generate-coverage-design.md +++ b/docs/generate-coverage-design.md @@ -110,18 +110,17 @@ GitHub Actions environment variables already set by the caller. ### Cranelift Detection Strategy -Cranelift detection deliberately reuses the existing lightweight search in -`_uses_cranelift_backend(manifest_path)`: +Cranelift detection is intentionally lightweight, but it now checks two +sources before deciding coverage needs LLVM overrides: -- start from the selected Cargo manifest directory -- walk upward towards the filesystem root -- look for `.cargo/config.toml` and `.cargo/config` -- read the file as UTF-8 when possible -- treat any line matching `codegen-backend = "cranelift"` as a signal that - coverage should switch the `dev` and `test` profiles back to LLVM +- `_uses_cranelift_backend(manifest_path)` walks upward from the selected Cargo + manifest directory and scans `.cargo/config.toml` plus `.cargo/config`. +- `_manifest_uses_cranelift_backend(manifest_path)` reads the selected + `Cargo.toml` and scans profile sections for + `codegen-backend = "cranelift"` or the single-quoted equivalent. -This approach is fast, dependency-free, and good enough for the specific -coverage failure mode the action is addressing. +The action therefore catches both repository-level Cargo config overrides and +manifest-level profile settings without needing a full TOML parser. ### Known Limitations @@ -132,10 +131,9 @@ should understand: `codegen-backend = "cranelift"` assignment triggers the override, even if it appears in a table that is more specific than the action strictly needs to reason about. -- It only inspects `.cargo/config.toml` and `.cargo/config` files reachable by - walking upward from the manifest directory. It does not model configuration - injected via CLI `--config`, environment-backed Cargo config, or other - runtime indirection. +- It only inspects `.cargo/config.toml`, `.cargo/config`, and the selected + `Cargo.toml`. It does not model configuration injected via CLI `--config`, + environment-backed Cargo config, or other runtime indirection. - It always applies both `CARGO_PROFILE_DEV_CODEGEN_BACKEND` and `CARGO_PROFILE_TEST_CODEGEN_BACKEND` together once Cranelift is detected. The action currently does not try to mirror per-profile granularity from the From b4740ec69ea2bd0ee08c4f222ea21f336d35a61b Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 00:53:50 +0100 Subject: [PATCH 04/28] Honor workspace Cranelift coverage overrides Scan ancestor `Cargo.toml` files up to the workspace root so coverage runs inherit Cranelift detection from workspace-level profile settings, not only the leaf manifest. Always build the cargo subprocess environment from `os.environ`, strip an inherited `CARGO_PROFILE_DEV_CODEGEN_BACKEND` before launching coverage, and then apply any explicit LLVM override. Keep the override surface to the documented dev-profile variable. Update the regression tests and design note to cover workspace-root profile inheritance and the new environment sanitisation contract. --- .../generate-coverage/scripts/run_rust.py | 82 +++++++++++++------ .../generate-coverage/tests/test_scripts.py | 48 +++++++++-- docs/generate-coverage-design.md | 40 +++++---- 3 files changed, 124 insertions(+), 46 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index f914a10b..43fb70f8 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -99,6 +99,11 @@ global-timeout = "10m" """ +_CARGO_COVERAGE_ENV_UNSETS = ( + "CARGO_PROFILE_DEV_CODEGEN_BACKEND", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND", +) + _LLVM_CODEGEN_ENV = { "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", @@ -410,14 +415,39 @@ def _pump_cargo_output(proc: subprocess.Popen[str]) -> list[str]: return stdout_lines -def _build_cargo_command( - extra_env: dict[str, str] | None, -) -> tuple[str, typ.Any]: - """Return *(display_prefix, cargo_cmd)* reflecting *extra_env*.""" - if not extra_env: - return "", cargo - env_prefix = " ".join(f"{k}={shlex.quote(v)}" for k, v in extra_env.items()) + " " - return env_prefix, cargo.with_env(**extra_env) +def _build_cargo_env( + env_overrides: typ.Mapping[str, str] | None, + env_unsets: typ.Iterable[str], +) -> dict[str, str]: + """Return the environment dict for a spawned cargo process.""" + env = dict(os.environ) + for key in env_unsets: + env.pop(key, None) + if env_overrides is not None: + env.update(env_overrides) + return env + + +def _spawn_cargo( + command: typ.Any, # noqa: ANN401 + env: dict[str, str], +) -> subprocess.Popen[str]: + """Spawn a ``cargo`` subprocess with the given environment.""" + popen_kwargs: dict[str, typ.Any] = { + "stdin": subprocess.DEVNULL, + "stdout": subprocess.PIPE, + "stderr": subprocess.PIPE, + "text": True, + "encoding": "utf-8", + "errors": "replace", + } + machine_env = getattr(getattr(cargo, "machine", None), "env", None) + if machine_env is None: + return command.popen(**popen_kwargs, env=env) + with machine_env(): + machine_env.clear() + machine_env.update(env) + return command.popen(**popen_kwargs) def _abort_on_missing_streams(proc: subprocess.Popen[str]) -> None: @@ -467,18 +497,16 @@ def _wait_for_cargo( raise typer.Exit(code=retcode or 1) -def _run_cargo(args: list[str], *, extra_env: dict[str, str] | None = None) -> str: +def _run_cargo( + args: list[str], + *, + env_overrides: typ.Mapping[str, str] | None = None, + env_unsets: typ.Iterable[str] = (), +) -> str: """Run ``cargo`` with ``args`` streaming output and return ``stdout``.""" - env_prefix, cargo_cmd = _build_cargo_command(extra_env) - typer.echo(f"$ {env_prefix}cargo {shlex.join(args)}") - proc = cargo_cmd[args].popen( - stdin=subprocess.DEVNULL, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - encoding="utf-8", - errors="replace", - ) + typer.echo(f"$ cargo {shlex.join(args)}") + env = _build_cargo_env(env_overrides, env_unsets) + proc = _spawn_cargo(cargo[args], env) try: _abort_on_missing_streams(proc) stdout_lines = _pump_cargo_output(proc) @@ -524,7 +552,6 @@ def run_cucumber_rs_coverage( use_nextest: bool, cucumber_rs_features: str, cucumber_rs_args: str, - extra_env: dict[str, str] | None = None, ) -> None: """Run cucumber.rs coverage and merge results into ``out``.""" cucumber_file = out.with_name(f"{out.stem}.cucumber{out.suffix}") @@ -548,7 +575,11 @@ def run_cucumber_rs_coverage( if cucumber_rs_args: c_args += shlex.split(cucumber_rs_args) - _run_cargo(c_args, extra_env=extra_env) + _run_cargo( + c_args, + env_overrides=get_cargo_coverage_env(manifest_path), + env_unsets=_CARGO_COVERAGE_ENV_UNSETS, + ) if fmt == "cobertura": from plumbum.cmd import uvx @@ -648,12 +679,16 @@ def main( with_default=with_default, use_nextest=use_nextest, ) - extra_env = get_cargo_coverage_env(manifest_path) + cargo_env = get_cargo_coverage_env(manifest_path) config_context = ( ensure_nextest_config() if use_nextest else contextlib.nullcontext() ) with config_context: - stdout = _run_cargo(args, extra_env=extra_env) + stdout = _run_cargo( + args, + env_overrides=cargo_env, + env_unsets=_CARGO_COVERAGE_ENV_UNSETS, + ) if with_cucumber_rs and cucumber_rs_features: run_cucumber_rs_coverage( @@ -665,7 +700,6 @@ def main( use_nextest=use_nextest, cucumber_rs_features=cucumber_rs_features, cucumber_rs_args=cucumber_rs_args, - extra_env=extra_env, ) if fmt == "lcov": percent = get_line_coverage_percent_from_lcov(out) diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index db4ad2b1..2bcf7713 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -361,13 +361,17 @@ def _run_rust_main_variant( output = tmp_path / "cov.lcov" output.write_text("LF:10\nLH:10\n") github_output = tmp_path / "gh.txt" - recorded_args: list[str] = [] + recorded: dict[str, object] = {} def fake_run_cargo( - args: list[str], *, extra_env: dict[str, str] | None = None + args: list[str], + *, + env_overrides: typ.Mapping[str, str] | None = None, + env_unsets: typ.Iterable[str] = (), ) -> str: - recorded_args[:] = args - _ = extra_env + recorded["args"] = args + recorded["env_overrides"] = dict(env_overrides or {}) + recorded["env_unsets"] = tuple(env_unsets) return "Coverage: 100%" monkeypatch.setattr(run_rust_module, "_run_cargo", fake_run_cargo) @@ -386,7 +390,7 @@ def fake_run_cargo( with_cucumber_rs=False, baseline_file=None, ) - return recorded_args, github_output, output + return typ.cast("list[str]", recorded["args"]), github_output, output @pytest.mark.parametrize("use_nextest", [True, False]) @@ -716,6 +720,40 @@ def close(self) -> None: assert stderr.close_calls >= 1 +def test_run_cargo_passes_env_overrides( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``_run_cargo`` scrubs and reapplies coverage env overrides.""" + mod = _load_module(monkeypatch, "run_rust") + monkeypatch.setattr(mod.os, "name", "nt") + monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setenv("RUN_RUST_INHERITED", "present") + monkeypatch.setenv("CARGO_PROFILE_DEV_CODEGEN_BACKEND", "cranelift") + monkeypatch.setenv("CARGO_PROFILE_TEST_CODEGEN_BACKEND", "cranelift") + + fake_cargo = _make_fake_cargo("out-line\n", "err-line\n") + monkeypatch.setattr(mod, "cargo", fake_cargo) + + result = mod._run_cargo( + ["llvm-cov"], + env_unsets=( + "CARGO_PROFILE_DEV_CODEGEN_BACKEND", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND", + ), + env_overrides={ + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + }, + ) + + assert result == "out-line" + assert fake_cargo.last_popen_kwargs is not None + env = typ.cast("dict[str, str]", fake_cargo.last_popen_kwargs["env"]) + assert env["RUN_RUST_INHERITED"] == "present" + assert env["CARGO_PROFILE_DEV_CODEGEN_BACKEND"] == "llvm" + assert env["CARGO_PROFILE_TEST_CODEGEN_BACKEND"] == "llvm" + + def test_run_cargo_windows_nonzero_exit( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/docs/generate-coverage-design.md b/docs/generate-coverage-design.md index b83d8b77..f1c16fbf 100644 --- a/docs/generate-coverage-design.md +++ b/docs/generate-coverage-design.md @@ -65,7 +65,6 @@ When Cranelift is detected, the helper returns: ```text CARGO_PROFILE_DEV_CODEGEN_BACKEND=llvm -CARGO_PROFILE_TEST_CODEGEN_BACKEND=llvm ``` These variables are passed to the `cargo llvm-cov` subprocess and therefore @@ -81,10 +80,10 @@ The environment-variable approach is intentional rather than incidental: - Coverage therefore needs a transport mechanism that survives process boundaries. -Using `CARGO_PROFILE_*_CODEGEN_BACKEND` matches Cargo's configuration model -while keeping the override tightly scoped to the coverage subprocess. The -repository's checked-in configuration stays unchanged, and non-coverage flows -continue to use Cranelift if the repository asked for it. +Using `CARGO_PROFILE_DEV_CODEGEN_BACKEND` keeps the override tightly scoped to +the coverage subprocess. The repository's checked-in configuration stays +unchanged, and non-coverage flows continue to use Cranelift if the repository +asked for it. ### Where the Overrides Apply @@ -98,15 +97,20 @@ not drift between the primary and follow-up coverage invocations. ### Behaviour of `env_overrides` -`_run_cargo(..., env_overrides=...)` accepts an optional mapping: +`_run_cargo(..., env_overrides=..., env_unsets=...)` accepts two optional +inputs: -- `None` means "use the inherited process environment unchanged" +- `env_overrides=None` means "do not add replacement values" +- `env_unsets=()` means "do not remove any inherited keys" - a mapping means "merge the current environment with these extra keys" +- an iterable of keys means "remove these inherited variables before applying + overrides" -The helper does not construct a clean-room environment. It starts from -`os.environ` and overlays the requested keys, preserving the rest of the -workflow's runtime context such as PATH, toolchain configuration, and any -GitHub Actions environment variables already set by the caller. +The helper still starts from `os.environ` so it preserves PATH, toolchain +configuration, and the rest of the GitHub Actions runtime context, but it now +explicitly removes inherited `CARGO_PROFILE_DEV_CODEGEN_BACKEND` before +applying coverage overrides. That prevents a caller or runner host from +leaking an unrelated Cranelift preference into coverage runs. ### Cranelift Detection Strategy @@ -116,11 +120,13 @@ sources before deciding coverage needs LLVM overrides: - `_uses_cranelift_backend(manifest_path)` walks upward from the selected Cargo manifest directory and scans `.cargo/config.toml` plus `.cargo/config`. - `_manifest_uses_cranelift_backend(manifest_path)` reads the selected - `Cargo.toml` and scans profile sections for + `Cargo.toml` and then walks ancestor `Cargo.toml` files until it reaches the + filesystem root or a manifest that declares `[workspace]`. +- Each visited manifest is scanned for profile sections containing `codegen-backend = "cranelift"` or the single-quoted equivalent. The action therefore catches both repository-level Cargo config overrides and -manifest-level profile settings without needing a full TOML parser. +workspace-root profile settings without needing a full TOML parser. ### Known Limitations @@ -132,10 +138,10 @@ should understand: appears in a table that is more specific than the action strictly needs to reason about. - It only inspects `.cargo/config.toml`, `.cargo/config`, and the selected - `Cargo.toml`. It does not model configuration injected via CLI `--config`, - environment-backed Cargo config, or other runtime indirection. -- It always applies both `CARGO_PROFILE_DEV_CODEGEN_BACKEND` and - `CARGO_PROFILE_TEST_CODEGEN_BACKEND` together once Cranelift is detected. The + `Cargo.toml` plus its ancestor manifests up to the workspace root. It does + not model configuration injected via CLI `--config`, environment-backed Cargo + config beyond the explicit dev-profile unset, or other runtime indirection. +- It always applies the `dev` profile override once Cranelift is detected. The action currently does not try to mirror per-profile granularity from the repository config. - Files that cannot be read as UTF-8 are ignored rather than failing the run, From 77be6930b5c5eaee6ae93a2d98ed5fb2e5128383 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 00:55:08 +0100 Subject: [PATCH 05/28] Fix README env symbol in coverage sequence diagram Replace the non-standard `os_environ` label in the Rust coverage sequence diagram with the actual Python symbol `os.environ` so the docs match `_run_cargo()` and the current environment override flow. --- .github/actions/generate-coverage/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/actions/generate-coverage/README.md b/.github/actions/generate-coverage/README.md index 44e661e5..ccff2f9b 100644 --- a/.github/actions/generate-coverage/README.md +++ b/.github/actions/generate-coverage/README.md @@ -61,9 +61,9 @@ sequenceDiagram get_cargo_coverage_env-->>run_rust_py: cargo_env run_rust_py->>_run_cargo: _run_cargo(args, env_overrides=cargo_env) alt env_overrides is not None - _run_cargo->>_run_cargo: merge os_environ with env_overrides + _run_cargo->>_run_cargo: merge os.environ with env_overrides else - _run_cargo->>_run_cargo: use os_environ unchanged + _run_cargo->>_run_cargo: use os.environ unchanged end _run_cargo->>cargo: invoke cargo llvm-cov with env cargo-->>_run_cargo: stdout From 827e03c870c42975174648dab03f2faa33270ca0 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 01:07:40 +0100 Subject: [PATCH 06/28] Align Cranelift coverage behavior with docs Restore both LLVM coverage overrides for Cranelift-configured projects, limit manifest profile detection to the selected `cargo-manifest`, and add a regression test that documents the current workspace-member limitation. Add a user-facing README section for Cranelift support, update the internal design note to describe the actual detection boundary, and expand `_run_cargo()` documentation for environment overrides. --- .github/actions/generate-coverage/README.md | 89 ++++- .../generate-coverage/scripts/run_rust.py | 248 +++++++------ .../generate-coverage/tests/test_scripts.py | 350 +++++++----------- docs/generate-coverage-design.md | 61 +-- 4 files changed, 370 insertions(+), 378 deletions(-) diff --git a/.github/actions/generate-coverage/README.md b/.github/actions/generate-coverage/README.md index ccff2f9b..20c77ca8 100644 --- a/.github/actions/generate-coverage/README.md +++ b/.github/actions/generate-coverage/README.md @@ -14,12 +14,6 @@ installed automatically. If both configuration files are present, coverage is run for each language and the Cobertura reports are merged using `uvx merge-cobertura`. -If a Rust project enables Cranelift — via `.cargo/config.toml`, -`.cargo/config`, or a `[profile.*].codegen-backend` key in `Cargo.toml` — -the action automatically exports `CARGO_PROFILE_DEV_CODEGEN_BACKEND=llvm` -and `CARGO_PROFILE_TEST_CODEGEN_BACKEND=llvm` for the coverage runs so -`cargo llvm-cov` and its child Cargo processes stay on LLVM. - ## Flow ```mermaid @@ -41,12 +35,15 @@ flowchart TD K --> L[End] ``` - ## Rust coverage environment propagation Figure: sequence diagram showing how `run_rust.py` derives coverage-specific Cargo environment overrides for Cranelift-configured projects and passes them -into `_run_cargo`, including the optional cucumber.rs follow-up run. +into `_run_cargo`, including the optional cucumber.rs follow-up run. Internally +`_run_cargo` starts from the current process environment, removes inherited +codegen-backend-related variables first, and then merges +`get_cargo_coverage_env(manifest_path)` on top so workflow-level Cranelift +exports are not treated as the default coverage behaviour. ```mermaid sequenceDiagram @@ -54,16 +51,18 @@ sequenceDiagram participant run_rust_py as run_rust.py participant get_cargo_coverage_env participant _run_cargo + participant env_unsets participant cargo GitHubActions->>run_rust_py: main(manifest_path, fmt, use_nextest, ...) run_rust_py->>get_cargo_coverage_env: get_cargo_coverage_env(manifest_path) get_cargo_coverage_env-->>run_rust_py: cargo_env - run_rust_py->>_run_cargo: _run_cargo(args, env_overrides=cargo_env) + run_rust_py->>_run_cargo: _run_cargo(args, env_overrides=cargo_env, env_unsets=...) + _run_cargo->>env_unsets: scrub inherited backend vars alt env_overrides is not None - _run_cargo->>_run_cargo: merge os.environ with env_overrides + _run_cargo->>_run_cargo: merge scrubbed os.environ with env_overrides else - _run_cargo->>_run_cargo: use os.environ unchanged + _run_cargo->>_run_cargo: use scrubbed os.environ unchanged end _run_cargo->>cargo: invoke cargo llvm-cov with env cargo-->>_run_cargo: stdout @@ -72,13 +71,56 @@ sequenceDiagram opt with_cucumber_rs run_rust_py->>get_cargo_coverage_env: get_cargo_coverage_env(manifest_path) get_cargo_coverage_env-->>run_rust_py: cargo_env - run_rust_py->>_run_cargo: _run_cargo(cucumber_args, env_overrides=cargo_env) + run_rust_py->>_run_cargo: _run_cargo(cucumber_args, env_overrides=cargo_env, env_unsets=...) + _run_cargo->>env_unsets: scrub inherited backend vars _run_cargo->>cargo: invoke cargo test with env cargo-->>_run_cargo: stdout _run_cargo-->>run_rust_py: stdout end ``` +## Cranelift codegen backend support + +The action automatically detects when a Rust repository configures the +Cranelift codegen backend in `.cargo/config.toml`, `.cargo/config`, or the +selected `Cargo.toml` profile sections. You do not need to enable a separate +input for this behaviour. + +When Cranelift is detected, coverage runs set +`CARGO_PROFILE_DEV_CODEGEN_BACKEND=llvm` and +`CARGO_PROFILE_TEST_CODEGEN_BACKEND=llvm` as environment overrides before +launching `cargo-llvm-cov`. That ensures nested `cargo` processes inherit LLVM, +which is required by `-Cinstrument-coverage`. Normal non-coverage builds are +not changed by the action. + +For a Cranelift-configured repository, the standard coverage invocation is +still enough: + +```yaml +- uses: ./.github/actions/generate-coverage + with: + output-path: coverage.xml + format: cobertura +``` + +```yaml +- uses: leynos/shared-actions/.github/actions/generate-coverage@v1 + with: + output-path: coverage.xml + format: cobertura +``` + +Known limitations: + +- Profile sections in the workspace root `Cargo.toml` are only detected when + `cargo-manifest` points to the workspace root manifest. If `cargo-manifest` + points to a workspace member, Cranelift configured solely in the workspace + root manifest profile will not be detected; use `.cargo/config.toml` in that + case. +- Detection uses two approaches: `.cargo/config.toml` and `.cargo/config` + scanning remains text/regex-based, and selected `Cargo.toml` profile + detection also uses a lightweight text scan. + ## Inputs | Name | Description | Required | Default | @@ -112,7 +154,14 @@ supported for Python projects. Mixed projects must use `cobertura`. ## Example ```yaml -- uses: ./.github/actions/generate-coverage@v1 +- uses: ./.github/actions/generate-coverage + with: + output-path: coverage.xml + format: cobertura +``` + +```yaml +- uses: leynos/shared-actions/.github/actions/generate-coverage@v1 with: output-path: coverage.xml format: cobertura @@ -121,7 +170,7 @@ supported for Python projects. Mixed projects must use `cobertura`. For a single feature: ```yaml -- uses: ./.github/actions/generate-coverage@v1 +- uses: ./.github/actions/generate-coverage with: output-path: coverage.xml features: logging @@ -130,7 +179,7 @@ For a single feature: For multiple features: ```yaml -- uses: ./.github/actions/generate-coverage@v1 +- uses: ./.github/actions/generate-coverage with: output-path: coverage.xml features: logging tracing @@ -140,7 +189,7 @@ For multiple features: Comma-separated feature list: ```yaml -- uses: ./.github/actions/generate-coverage@v1 +- uses: ./.github/actions/generate-coverage with: output-path: coverage.xml features: logging,tracing @@ -149,7 +198,7 @@ Comma-separated feature list: Enable ratcheting: ```yaml -- uses: ./.github/actions/generate-coverage@v1 +- uses: ./.github/actions/generate-coverage with: output-path: coverage.xml with-ratchet: true @@ -158,7 +207,7 @@ Enable ratcheting: Enable cucumber-rs: ```yaml -- uses: ./.github/actions/generate-coverage@v1 +- uses: ./.github/actions/generate-coverage with: output-path: coverage.xml with-cucumber-rs: true @@ -169,7 +218,7 @@ Enable cucumber-rs: Disable cargo-nextest: ```yaml -- uses: ./.github/actions/generate-coverage@v1 +- uses: ./.github/actions/generate-coverage with: output-path: coverage.xml use-cargo-nextest: false @@ -178,7 +227,7 @@ Disable cargo-nextest: Use a nested Cargo manifest: ```yaml -- uses: ./.github/actions/generate-coverage@v1 +- uses: ./.github/actions/generate-coverage with: output-path: coverage.xml cargo-manifest: rust-toy-app/Cargo.toml diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index 43fb70f8..ede79654 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -17,7 +17,6 @@ import subprocess import sys import threading -import tomllib import traceback import typing as typ from decimal import ROUND_HALF_UP, Decimal @@ -105,66 +104,14 @@ ) -_LLVM_CODEGEN_ENV = { - "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", - "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", -} - - -def _is_cranelift_backend(value: object) -> bool: - """Return ``True`` when *value* declares the Cranelift codegen backend.""" - if not isinstance(value, str): - return False - return value.strip().casefold() == "cranelift" - - -def _toml_uses_cranelift_backend(content: str) -> bool: - """Return ``True`` when parsed TOML enables Cranelift for dev or test.""" - try: - data = tomllib.loads(content) - except tomllib.TOMLDecodeError: - return False - profile = data.get("profile") - if not isinstance(profile, dict): - return False - for profile_name in ("dev", "test"): - section = profile.get(profile_name) - if not isinstance(section, dict): - continue - if _is_cranelift_backend(section.get("codegen-backend")): - return True - return False - - -def _manifest_uses_cranelift(manifest_path: Path) -> bool: - """Return ``True`` when *manifest_path* declares cranelift in any profile.""" - try: - data = tomllib.loads(manifest_path.read_text(encoding="utf-8")) - except (OSError, UnicodeDecodeError, tomllib.TOMLDecodeError): - return False - profiles = data.get("profile") - if not isinstance(profiles, dict): - return False - return any( - isinstance(section, dict) - and _is_cranelift_backend(section.get("codegen-backend")) - for section in profiles.values() - ) - - def _uses_cranelift_backend(manifest_path: Path) -> bool: """Return ``True`` when the project configures the Cranelift codegen backend. - Checks ``[profile.*].codegen-backend`` in *manifest_path* itself, then - searches from the manifest directory upward for ``.cargo/config.toml`` + Searches from the manifest directory upward for ``.cargo/config.toml`` (or ``.cargo/config``) and checks whether any profile sets ``codegen-backend = "cranelift"``. """ - resolved = manifest_path.resolve() - if _manifest_uses_cranelift(resolved): - return True - - search_dir = resolved.parent + search_dir = manifest_path.resolve().parent while True: for name in ("config.toml", "config"): candidate = search_dir / ".cargo" / name @@ -173,7 +120,11 @@ def _uses_cranelift_backend(manifest_path: Path) -> bool: content = candidate.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError): continue - if _toml_uses_cranelift_backend(content): + if re.search( + r'^[ \t]*codegen-backend\s*=\s*["\']cranelift["\']', + content, + flags=re.MULTILINE, + ): return True parent = search_dir.parent if parent == search_dir: @@ -181,30 +132,63 @@ def _uses_cranelift_backend(manifest_path: Path) -> bool: search_dir = parent return False + +def _is_profile_section(section: str) -> bool: + """Return ``True`` if *section* is a Cargo profile section name. + + Matches both the bare ``[profile]`` table and dotted sub-tables such as + ``[profile.dev]`` and ``[profile.release]``. + """ + return section == "profile" or section.startswith("profile.") + + def _manifest_uses_cranelift_backend(manifest_path: Path) -> bool: - """Return ``True`` when ``manifest_path`` configures Cranelift in profiles.""" + """Return ``True`` when ``manifest_path`` configures Cranelift in profiles. + + Parameters + ---------- + manifest_path : Path + Path to the ``Cargo.toml`` manifest to inspect. + + Returns + ------- + bool + ``True`` if any ``[profile.*]`` section sets + ``codegen-backend = "cranelift"``; ``False`` otherwise. + """ try: content = manifest_path.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError): return False - in_profile_section = False for line in content.splitlines(): stripped = line.strip() - if not stripped or stripped.startswith("#"): + if not stripped or stripped.startswith(("#", "//")): continue - section_match = re.match(r"^\[(?P
[^\]]+)\]$", stripped) - if section_match: - section = section_match.group("section").strip() - in_profile_section = section == "profile" or section.startswith("profile.") + section_match = re.match(r"^\s*\[(?P
[^\]]+)\]\s*(?:#.*)?$", line) + if section_match is not None: + in_profile_section = _is_profile_section(section_match["section"]) continue - if not in_profile_section: - continue - if re.match(r'^[ \t]*codegen-backend\s*=\s*["\']cranelift["\']', line): + if in_profile_section and re.match( + r"""^codegen-backend\s*=\s*["']cranelift["']""", + stripped, + ): return True return False +def get_cargo_coverage_env(manifest_path: Path) -> dict[str, str]: + """Return coverage-specific cargo env overrides for Cranelift projects.""" + if not _uses_cranelift_backend( + manifest_path + ) and not _manifest_uses_cranelift_backend(manifest_path): + return {} + return { + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + } + + def get_cargo_coverage_cmd( fmt: str, out: Path, @@ -215,8 +199,7 @@ def get_cargo_coverage_cmd( use_nextest: bool, ) -> list[str]: """Return the cargo llvm-cov command arguments.""" - args: list[str] = [] - args.append("llvm-cov") + args = ["llvm-cov"] if use_nextest: args.append("nextest") args += ["--manifest-path", str(manifest_path), "--workspace", "--summary-only"] @@ -228,13 +211,6 @@ def get_cargo_coverage_cmd( return args -def get_cargo_coverage_env(manifest_path: Path) -> dict[str, str]: - """Return extra environment overrides needed for coverage runs.""" - if _uses_cranelift_backend(manifest_path): - return dict(_LLVM_CODEGEN_ENV) - return {} - - def extract_percent(output: str) -> str: """Return the coverage percentage extracted from ``output``.""" match = re.search( @@ -419,7 +395,11 @@ def _build_cargo_env( env_overrides: typ.Mapping[str, str] | None, env_unsets: typ.Iterable[str], ) -> dict[str, str]: - """Return the environment dict for a spawned cargo process.""" + """Return the environment dict for a spawned cargo process. + + Starts from a copy of ``os.environ``, removes every key in + ``env_unsets``, then merges ``env_overrides`` (if provided). + """ env = dict(os.environ) for key in env_unsets: env.pop(key, None) @@ -428,30 +408,11 @@ def _build_cargo_env( return env -def _spawn_cargo( - command: typ.Any, # noqa: ANN401 - env: dict[str, str], -) -> subprocess.Popen[str]: - """Spawn a ``cargo`` subprocess with the given environment.""" - popen_kwargs: dict[str, typ.Any] = { - "stdin": subprocess.DEVNULL, - "stdout": subprocess.PIPE, - "stderr": subprocess.PIPE, - "text": True, - "encoding": "utf-8", - "errors": "replace", - } - machine_env = getattr(getattr(cargo, "machine", None), "env", None) - if machine_env is None: - return command.popen(**popen_kwargs, env=env) - with machine_env(): - machine_env.clear() - machine_env.update(env) - return command.popen(**popen_kwargs) - +def _assert_cargo_streams(proc: subprocess.Popen[str]) -> None: + """Raise ``typer.Exit(1)`` if stdout or stderr were not captured. -def _abort_on_missing_streams(proc: subprocess.Popen[str]) -> None: - """Kill *proc* and exit with an error if its output streams were not captured.""" + Kills and cleans up the process before raising so no resources leak. + """ if proc.stdout is not None and proc.stderr is not None: return missing_streams = [] @@ -471,14 +432,15 @@ def _abort_on_missing_streams(proc: subprocess.Popen[str]) -> None: raise typer.Exit(1) from None -def _wait_for_cargo( - proc: subprocess.Popen[str], - args: list[str], - wait_timeout: float, -) -> None: - """Wait for *proc* to finish, raising ``typer.Exit`` on timeout or failure.""" +def _wait_for_cargo(proc: subprocess.Popen[str]) -> int: + """Wait for cargo to exit and return its return code. + + Kills the process and raises ``typer.Exit(1)`` if it does not exit + within ``RUN_RUST_CARGO_WAIT_TIMEOUT`` seconds (default 600). + """ + wait_timeout = float(os.getenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "600")) try: - retcode = proc.wait(timeout=wait_timeout) + return proc.wait(timeout=wait_timeout) except subprocess.TimeoutExpired: typer.echo( f"::error::cargo did not exit within {wait_timeout}s; killing", @@ -489,12 +451,32 @@ def _wait_for_cargo( with contextlib.suppress(Exception): proc.wait(timeout=5) raise typer.Exit(1) from None - if retcode != 0: - typer.echo( - f"cargo {shlex.join(args)} failed with code {retcode}", - err=True, - ) - raise typer.Exit(code=retcode or 1) + + +def _spawn_cargo( + command: typ.Any, # noqa: ANN401 + env: dict[str, str], +) -> subprocess.Popen[str]: + """Spawn a ``cargo`` subprocess with the given environment. + + Handles both direct ``popen`` invocation and plumbum machine-env + contexts transparently. + """ + popen_kwargs: dict[str, typ.Any] = { + "stdin": subprocess.DEVNULL, + "stdout": subprocess.PIPE, + "stderr": subprocess.PIPE, + "text": True, + "encoding": "utf-8", + "errors": "replace", + } + machine_env = getattr(getattr(cargo, "machine", None), "env", None) + if machine_env is None: + return command.popen(**popen_kwargs, env=env) + with machine_env(): + machine_env.clear() + machine_env.update(env) + return command.popen(**popen_kwargs) def _run_cargo( @@ -503,15 +485,47 @@ def _run_cargo( env_overrides: typ.Mapping[str, str] | None = None, env_unsets: typ.Iterable[str] = (), ) -> str: - """Run ``cargo`` with ``args`` streaming output and return ``stdout``.""" + """Run ``cargo`` with ``args`` streaming output and return ``stdout``. + + Builds a subprocess environment by copying ``os.environ``, removing every + key in ``env_unsets``, and — when ``env_overrides`` is not ``None`` — + merging its entries into that copy (overrides take precedence). The + resulting environment is passed to the spawned ``cargo`` process, whose + stdout and stderr are streamed to the current process. + + Parameters + ---------- + args : list[str] + Arguments forwarded verbatim to ``cargo``. + env_overrides : Mapping[str, str] | None, optional + Extra or replacement environment variables. When ``None`` (the + default), the environment is inherited unchanged except for any + ``env_unsets`` removals. + env_unsets : Iterable[str], optional + Variable names to remove from the inherited environment before + ``env_overrides`` are applied. Missing keys are silently ignored. + Unsets are performed before overrides, so ``env_overrides`` can + unconditionally set a variable that may or may not have been + inherited. + + Returns + ------- + str + Captured stdout from the ``cargo`` invocation. + """ typer.echo(f"$ cargo {shlex.join(args)}") env = _build_cargo_env(env_overrides, env_unsets) proc = _spawn_cargo(cargo[args], env) try: - _abort_on_missing_streams(proc) + _assert_cargo_streams(proc) stdout_lines = _pump_cargo_output(proc) - wait_timeout = float(os.getenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "600")) - _wait_for_cargo(proc, args, wait_timeout) + retcode = _wait_for_cargo(proc) + if retcode != 0: + typer.echo( + f"cargo {shlex.join(args)} failed with code {retcode}", + err=True, + ) + raise typer.Exit(code=retcode or 1) return "\n".join(stdout_lines) finally: _safe_close_text_stream(proc.stdout) @@ -679,10 +693,10 @@ def main( with_default=with_default, use_nextest=use_nextest, ) - cargo_env = get_cargo_coverage_env(manifest_path) config_context = ( ensure_nextest_config() if use_nextest else contextlib.nullcontext() ) + cargo_env = get_cargo_coverage_env(manifest_path) with config_context: stdout = _run_cargo( args, diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index 2bcf7713..ef200b78 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -9,7 +9,6 @@ import io import itertools import os -import shutil import sys import typing as typ from pathlib import Path @@ -18,7 +17,7 @@ from plumbum import local from cmd_utils_importer import import_cmd_utils -from test_support.cmd_mox_stub_adapter import DefaultResponse +from test_support.cmd_mox_stub_adapter import Call, DefaultResponse from test_support.plumbum_helpers import run_plumbum_command if typ.TYPE_CHECKING: # pragma: no cover - type hints only @@ -30,12 +29,6 @@ RunResult = import_cmd_utils().RunResult -_LLVM_CODEGEN_ENV = { - "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", - "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", -} - - def _exit_code(exc: BaseException) -> int | None: """Extract an exit code from Typer or SystemExit exceptions.""" exit_code = getattr(exc, "exit_code", None) @@ -133,12 +126,22 @@ def wait(self, timeout: float | None = None) -> int: class FakeCargo: def __init__(self) -> None: self.last_proc: FakeProc | None = None + self.last_popen_kwargs: dict[str, object] | None = None + self.bound_env: dict[str, str] | None = None + + def with_env(self, **env: str) -> FakeCargo: + self.bound_env = dict(env) + return self def __getitem__(self, _args: list[str]) -> object: cargo = self class Runner: def popen(self, **_kw: object) -> FakeProc: + kwargs = dict(_kw) + if cargo.bound_env is not None: + kwargs["env"] = dict(cargo.bound_env) + cargo.last_popen_kwargs = kwargs proc = FakeProc() cargo.last_proc = proc return proc @@ -172,7 +175,7 @@ def _run_rust_coverage_test( config: RustCoverageConfig, *, monkeypatch: pytest.MonkeyPatch | None = None, -) -> tuple[list[str], dict[str, str], Path, Path]: +) -> tuple[list[str], Path, Path]: """Run ``run_rust.py`` with shared setup and return cargo argv + paths.""" out = tmp_path / "cov.lcov" gh = tmp_path / "gh.txt" @@ -208,7 +211,26 @@ def _run_rust_coverage_test( calls = shell_stubs.calls_of("cargo") assert len(calls) == 1 - return calls[0].argv, calls[0].env, out, gh + return calls[0].argv, out, gh + + +def _run_rust_coverage_call( + tmp_path: Path, + shell_stubs: StubManager, + config: RustCoverageConfig, + *, + monkeypatch: pytest.MonkeyPatch | None = None, +) -> tuple[Call, Path, Path]: + """Run ``run_rust.py`` and return the recorded cargo call + output paths.""" + _cargo_args, out, gh = _run_rust_coverage_test( + tmp_path, + shell_stubs, + config, + monkeypatch=monkeypatch, + ) + calls = shell_stubs.calls_of("cargo") + assert len(calls) == 1 + return calls[0], out, gh def test_run_rust_success( @@ -217,8 +239,8 @@ def test_run_rust_success( monkeypatch: pytest.MonkeyPatch, ) -> None: """Happy path for ``run_rust.py``.""" - monkeypatch.delenv("CARGO_PROFILE_DEV_CODEGEN_BACKEND", raising=False) - monkeypatch.delenv("CARGO_PROFILE_TEST_CODEGEN_BACKEND", raising=False) + monkeypatch.setenv("CARGO_PROFILE_DEV_CODEGEN_BACKEND", "cranelift") + monkeypatch.setenv("CARGO_PROFILE_TEST_CODEGEN_BACKEND", "cranelift") cargo_call, out, gh = _run_rust_coverage_call( tmp_path, shell_stubs, @@ -257,7 +279,7 @@ def test_run_rust_nextest_command( monkeypatch: pytest.MonkeyPatch, ) -> None: """``run_rust.py`` uses cargo llvm-cov nextest when enabled.""" - cargo_args, _cargo_env, out, _gh = _run_rust_coverage_test( + cargo_args, out, _gh = _run_rust_coverage_test( tmp_path, shell_stubs, RustCoverageConfig(use_nextest=True), @@ -282,7 +304,7 @@ def test_run_rust_uses_detected_manifest_path( tmp_path: Path, shell_stubs: StubManager ) -> None: """Detected manifest path is propagated to cargo llvm-cov.""" - cargo_args, _cargo_env, _out, _gh = _run_rust_coverage_test( + cargo_args, _out, _gh = _run_rust_coverage_test( tmp_path, shell_stubs, RustCoverageConfig(use_nextest=False, manifest_path="rust-toy-app/Cargo.toml"), @@ -423,172 +445,125 @@ def test_run_rust_main_nextest_variants( assert "Cargo.toml" in args -def test_run_rust_cranelift_project_uses_llvm_codegen( +def test_run_rust_cranelift_project_uses_llvm_codegen_env( tmp_path: Path, shell_stubs: StubManager, monkeypatch: pytest.MonkeyPatch, ) -> None: - """Coverage uses environment overrides for Cranelift-configured projects.""" - fixture_dir = ( - Path(__file__).resolve().parent / "fixtures" / "nightly-cranelift-project" + """Coverage exports LLVM env overrides for Cranelift-configured repos. + + When a Rust project uses the Cranelift codegen backend (configured in + .cargo/config.toml), the coverage action must still invoke cargo with + environment overrides that force nested cargo invocations spawned by + cargo-llvm-cov back onto LLVM, because source-based code coverage + (-C instrument-coverage) is an LLVM-only feature. + + In a real-world scenario, the Cranelift component would be installed via + ``rustup component add rustc-codegen-cranelift-preview``. + """ + # Simulate a Cranelift-configured project + cargo_config_dir = tmp_path / ".cargo" + cargo_config_dir.mkdir() + (cargo_config_dir / "config.toml").write_text( + "[unstable]\ncodegen-backend = true\n\n" + '[profile.dev]\ncodegen-backend = "cranelift"\n\n' + '[profile.test]\ncodegen-backend = "cranelift"\n', ) - shutil.copytree(fixture_dir, tmp_path, dirs_exist_ok=True) - cargo_args, cargo_env, _out, _gh = _run_rust_coverage_test( + cargo_call, _out, _gh = _run_rust_coverage_call( tmp_path, shell_stubs, RustCoverageConfig(use_nextest=True), monkeypatch=monkeypatch, ) - assert cargo_args[:2] == ["llvm-cov", "nextest"] - assert "--config" not in cargo_args - for key, value in _LLVM_CODEGEN_ENV.items(): - assert cargo_env[key] == value - - -def test_get_cargo_coverage_env_detects_cranelift_fixture( - run_rust_module: ModuleType, -) -> None: - """Cranelift fixtures resolve to cargo environment overrides.""" - fixture_dir = ( - Path(__file__).resolve().parent / "fixtures" / "nightly-cranelift-project" - ) - env = run_rust_module.get_cargo_coverage_env(fixture_dir / "Cargo.toml") - assert env == _LLVM_CODEGEN_ENV - - -def test_uses_cranelift_backend_detects_manifest_fixture( - run_rust_module: ModuleType, -) -> None: - """Cargo.toml profile settings alone trigger Cranelift detection.""" - fixture_dir = ( - Path(__file__).resolve().parent / "fixtures" / "cargo-toml-cranelift-project" - ) - assert run_rust_module._uses_cranelift_backend(fixture_dir / "Cargo.toml") is True + assert cargo_call.argv[:2] == ["llvm-cov", "nextest"] + assert "--config" not in cargo_call.argv + assert cargo_call.env["CARGO_PROFILE_DEV_CODEGEN_BACKEND"] == "llvm" + assert cargo_call.env["CARGO_PROFILE_TEST_CODEGEN_BACKEND"] == "llvm" -def test_get_cargo_coverage_env_non_cranelift_is_empty( - run_rust_module: ModuleType, tmp_path: Path +def test_get_cargo_coverage_env_cranelift_variants( + tmp_path: Path, run_rust_module: ModuleType ) -> None: - """Non-Cranelift projects do not receive extra cargo env overrides.""" + """Config or manifest Cranelift settings get overrides; LLVM repos do not.""" + cargo_config_dir = tmp_path / ".cargo" + cargo_config_dir.mkdir() + cargo_config = cargo_config_dir / "config.toml" manifest_path = tmp_path / "Cargo.toml" - manifest_path.write_text("[package]\nname='demo'\nversion='0.1.0'\n") - assert run_rust_module.get_cargo_coverage_env(manifest_path) == {} + manifest_path.write_text("[package]\nname = 'demo'\nversion = '0.1.0'\n") + cargo_config.write_text( + "[profile.dev]\ncodegen-backend = 'cranelift'\n", + encoding="utf-8", + ) + assert run_rust_module.get_cargo_coverage_env(manifest_path) == { + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + } -@pytest.mark.parametrize("profile_name", ["dev", "test"]) -def test_get_cargo_coverage_env_detects_manifest_only_cranelift( - run_rust_module: ModuleType, - tmp_path: Path, - profile_name: str, -) -> None: - """Manifest profile settings alone trigger LLVM codegen env overrides.""" - manifest_path = tmp_path / "Cargo.toml" + cargo_config.write_text( + "[profile.dev]\ncodegen-backend = 'llvm'\n", encoding="utf-8" + ) manifest_path.write_text( - "\n".join( - [ - "[package]", - "name='demo'", - "version='0.1.0'", - f"[profile.{profile_name}]", - 'codegen-backend = " Cranelift "', - "", - ] - ), + "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" + '[profile.dev]\ncodegen-backend = "cranelift"\n', encoding="utf-8", ) + assert run_rust_module.get_cargo_coverage_env(manifest_path) == { + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + } - assert run_rust_module._uses_cranelift_backend(manifest_path) is True - assert run_rust_module.get_cargo_coverage_env(manifest_path) == _LLVM_CODEGEN_ENV - - -@dataclasses.dataclass(frozen=True, slots=True) -class _CucumberEnvScenario: - manifest_path: Path - extra_env: dict[str, str] | None - - -def _run_cucumber_coverage_and_capture_env( - run_rust_module: ModuleType, - monkeypatch: pytest.MonkeyPatch, - tmp_path: Path, - scenario: _CucumberEnvScenario, -) -> dict[str, str] | None: - """Stub ``_run_cargo``, invoke ``run_cucumber_rs_coverage``, return captured env.""" - out = tmp_path / "coverage.lcov" - out.write_text("TN:\nend_of_record\n", encoding="utf-8") - captured_env: dict[str, str] | None = None - - def fake_run_cargo( - _args: list[str], *, extra_env: dict[str, str] | None = None - ) -> str: - nonlocal captured_env - captured_env = extra_env - out.with_name(f"{out.stem}.cucumber{out.suffix}").write_text( - "TN:\nend_of_record\n", - encoding="utf-8", - ) - return "" - - monkeypatch.setattr(run_rust_module, "_run_cargo", fake_run_cargo) - run_rust_module.run_cucumber_rs_coverage( - out, - "lcov", - "", - manifest_path=scenario.manifest_path, - with_default=True, - use_nextest=False, - cucumber_rs_features="cucumber", - cucumber_rs_args="", - extra_env=scenario.extra_env, + manifest_path.write_text( + "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" + "[profile.test]\ncodegen-backend = 'cranelift'\n", + encoding="utf-8", ) - return captured_env - + assert run_rust_module.get_cargo_coverage_env(manifest_path) == { + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + } -def test_run_cucumber_rs_coverage_passes_extra_env_for_cranelift( - run_rust_module: ModuleType, - monkeypatch: pytest.MonkeyPatch, - tmp_path: Path, -) -> None: - """cucumber.rs coverage forwards Cranelift env overrides to ``_run_cargo``.""" - manifest_path = ( - Path(__file__).resolve().parent - / "fixtures" - / "nightly-cranelift-project" - / "Cargo.toml" + manifest_path.write_text( + "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" + "[profile.dev] # comment\ncodegen-backend = 'cranelift'\n", + encoding="utf-8", ) - captured_env = _run_cucumber_coverage_and_capture_env( - run_rust_module, - monkeypatch, - tmp_path, - _CucumberEnvScenario( - manifest_path=manifest_path, - extra_env=run_rust_module.get_cargo_coverage_env(manifest_path), - ), + assert run_rust_module.get_cargo_coverage_env(manifest_path) == { + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + } + + manifest_path.write_text( + "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" + "[profile.dev]\ncodegen-backend = 'llvm'\n", + encoding="utf-8", ) - assert captured_env == _LLVM_CODEGEN_ENV + assert run_rust_module.get_cargo_coverage_env(manifest_path) == {} -def test_run_cucumber_rs_coverage_passes_extra_env_for_non_cranelift( - run_rust_module: ModuleType, - monkeypatch: pytest.MonkeyPatch, - tmp_path: Path, +def test_get_cargo_coverage_env_workspace_member_manifest_cranelift( + tmp_path: Path, run_rust_module: ModuleType ) -> None: - """cucumber.rs coverage forwards explicit non-Cranelift env unchanged.""" - manifest_path = tmp_path / "Cargo.toml" - manifest_path.write_text( - "[package]\nname='demo'\nversion='0.1.0'\n", + """Workspace-root profile-only Cranelift is not detected for members.""" + workspace_manifest = tmp_path / "Cargo.toml" + workspace_manifest.write_text( + '[workspace]\nmembers = ["crates/foo"]\n\n' + "[profile.dev]\ncodegen-backend = 'cranelift'\n", encoding="utf-8", ) - extra_env = {"FOO": "BAR", "BAZ": "QUX"} - captured_env = _run_cucumber_coverage_and_capture_env( - run_rust_module, - monkeypatch, - tmp_path, - _CucumberEnvScenario(manifest_path=manifest_path, extra_env=extra_env), + member_dir = tmp_path / "crates" / "foo" + member_dir.mkdir(parents=True) + member_manifest = member_dir / "Cargo.toml" + member_manifest.write_text( + '[package]\nname = "foo"\nversion = "0.1.0"\n', + encoding="utf-8", ) - assert captured_env == extra_env + + # This is a documented limitation for now: manifest scanning only inspects + # the selected member manifest, not workspace-root profile sections. + assert run_rust_module.get_cargo_coverage_env(member_manifest) == {} def test_nextest_config_is_temporary( @@ -723,27 +698,20 @@ def close(self) -> None: def test_run_cargo_passes_env_overrides( monkeypatch: pytest.MonkeyPatch, ) -> None: - """``_run_cargo`` scrubs and reapplies coverage env overrides.""" + """``_run_cargo`` merges env overrides into the spawned cargo process.""" mod = _load_module(monkeypatch, "run_rust") monkeypatch.setattr(mod.os, "name", "nt") monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) monkeypatch.setenv("RUN_RUST_INHERITED", "present") monkeypatch.setenv("CARGO_PROFILE_DEV_CODEGEN_BACKEND", "cranelift") - monkeypatch.setenv("CARGO_PROFILE_TEST_CODEGEN_BACKEND", "cranelift") fake_cargo = _make_fake_cargo("out-line\n", "err-line\n") monkeypatch.setattr(mod, "cargo", fake_cargo) result = mod._run_cargo( ["llvm-cov"], - env_unsets=( - "CARGO_PROFILE_DEV_CODEGEN_BACKEND", - "CARGO_PROFILE_TEST_CODEGEN_BACKEND", - ), - env_overrides={ - "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", - "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", - }, + env_unsets=("CARGO_PROFILE_DEV_CODEGEN_BACKEND",), + env_overrides={"CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm"}, ) assert result == "out-line" @@ -751,7 +719,6 @@ def test_run_cargo_passes_env_overrides( env = typ.cast("dict[str, str]", fake_cargo.last_popen_kwargs["env"]) assert env["RUN_RUST_INHERITED"] == "present" assert env["CARGO_PROFILE_DEV_CODEGEN_BACKEND"] == "llvm" - assert env["CARGO_PROFILE_TEST_CODEGEN_BACKEND"] == "llvm" def test_run_cargo_windows_nonzero_exit( @@ -1376,22 +1343,22 @@ def test_merge_cobertura(tmp_path: Path, shell_stubs: StubManager) -> None: @pytest.mark.parametrize( - ("filename", "content"), + ("content", "label"), [ - pytest.param("zero.lcov", "LF:0\nLH:0\n", id="zero-lines"), - pytest.param("empty.lcov", "", id="empty-file"), - pytest.param("missing.lcov", "LF:100\n", id="missing-lh-tag"), - pytest.param("bad.lcov", "LF:abc\nLH:xyz\n", id="malformed"), + ("LF:0\nLH:0\n", "zero_lines"), + ("", "empty_file"), + ("LF:100\n", "missing_lh_tag"), + ("LF:abc\nLH:xyz\n", "malformed_values"), ], ) -def test_lcov_zero_coverage_variants( +def test_lcov_returns_zero_for_degenerate_files( tmp_path: Path, run_rust_module: ModuleType, - filename: str, content: str, + label: str, ) -> None: """``get_line_coverage_percent_from_lcov`` returns 0.00 for degenerate inputs.""" - lcov = tmp_path / filename + lcov = tmp_path / f"{label}.lcov" lcov.write_text(content) assert run_rust_module.get_line_coverage_percent_from_lcov(lcov) == "0.00" @@ -1737,52 +1704,3 @@ def raise_permission_error(*_: object, **__: object) -> object: with pytest.raises(run_python_module.typer.Exit) as excinfo: run_python_module.get_line_coverage_percent_from_cobertura(xml) assert _exit_code(excinfo.value) == 1 - -def test_get_cargo_coverage_env_cranelift_variants( - tmp_path: Path, run_rust_module: ModuleType -) -> None: - """Config or manifest Cranelift settings get overrides; LLVM repos do not.""" - cargo_config_dir = tmp_path / ".cargo" - cargo_config_dir.mkdir() - cargo_config = cargo_config_dir / "config.toml" - manifest_path = tmp_path / "Cargo.toml" - manifest_path.write_text("[package]\nname = 'demo'\nversion = '0.1.0'\n") - - cargo_config.write_text( - "[profile.dev]\ncodegen-backend = 'cranelift'\n", - encoding="utf-8", - ) - assert run_rust_module.get_cargo_coverage_env(manifest_path) == { - "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", - "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", - } - - cargo_config.write_text( - "[profile.dev]\ncodegen-backend = 'llvm'\n", encoding="utf-8" - ) - manifest_path.write_text( - "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" - '[profile.dev]\ncodegen-backend = "cranelift"\n', - encoding="utf-8", - ) - assert run_rust_module.get_cargo_coverage_env(manifest_path) == { - "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", - "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", - } - - manifest_path.write_text( - "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" - "[profile.test]\ncodegen-backend = 'cranelift'\n", - encoding="utf-8", - ) - assert run_rust_module.get_cargo_coverage_env(manifest_path) == { - "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", - "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", - } - - manifest_path.write_text( - "[package]\nname = 'demo'\nversion = '0.1.0'\n\n" - "[profile.dev]\ncodegen-backend = 'llvm'\n", - encoding="utf-8", - ) - assert run_rust_module.get_cargo_coverage_env(manifest_path) == {} diff --git a/docs/generate-coverage-design.md b/docs/generate-coverage-design.md index f1c16fbf..8a2b9362 100644 --- a/docs/generate-coverage-design.md +++ b/docs/generate-coverage-design.md @@ -65,6 +65,7 @@ When Cranelift is detected, the helper returns: ```text CARGO_PROFILE_DEV_CODEGEN_BACKEND=llvm +CARGO_PROFILE_TEST_CODEGEN_BACKEND=llvm ``` These variables are passed to the `cargo llvm-cov` subprocess and therefore @@ -80,10 +81,11 @@ The environment-variable approach is intentional rather than incidental: - Coverage therefore needs a transport mechanism that survives process boundaries. -Using `CARGO_PROFILE_DEV_CODEGEN_BACKEND` keeps the override tightly scoped to -the coverage subprocess. The repository's checked-in configuration stays -unchanged, and non-coverage flows continue to use Cranelift if the repository -asked for it. +Using `CARGO_PROFILE_DEV_CODEGEN_BACKEND` and +`CARGO_PROFILE_TEST_CODEGEN_BACKEND` keeps the override tightly scoped to the +coverage subprocess. The repository's checked-in configuration stays unchanged, +and non-coverage flows continue to use Cranelift if the repository asked for +it. ### Where the Overrides Apply @@ -102,51 +104,60 @@ inputs: - `env_overrides=None` means "do not add replacement values" - `env_unsets=()` means "do not remove any inherited keys" -- a mapping means "merge the current environment with these extra keys" -- an iterable of keys means "remove these inherited variables before applying - overrides" +- when `env_overrides` is a mapping, apply these overrides +- when `env_unsets` is an iterable of key names, remove these inherited + variables before applying overrides The helper still starts from `os.environ` so it preserves PATH, toolchain configuration, and the rest of the GitHub Actions runtime context, but it now -explicitly removes inherited `CARGO_PROFILE_DEV_CODEGEN_BACKEND` before -applying coverage overrides. That prevents a caller or runner host from +explicitly removes inherited `CARGO_PROFILE_DEV_CODEGEN_BACKEND` and +`CARGO_PROFILE_TEST_CODEGEN_BACKEND` before applying coverage overrides. That +prevents a caller or runner host from leaking an unrelated Cranelift preference into coverage runs. ### Cranelift Detection Strategy -Cranelift detection is intentionally lightweight, but it now checks two +Cranelift detection is intentionally lightweight, but it checks two sources before deciding coverage needs LLVM overrides: - `_uses_cranelift_backend(manifest_path)` walks upward from the selected Cargo manifest directory and scans `.cargo/config.toml` plus `.cargo/config`. - `_manifest_uses_cranelift_backend(manifest_path)` reads the selected - `Cargo.toml` and then walks ancestor `Cargo.toml` files until it reaches the - filesystem root or a manifest that declares `[workspace]`. -- Each visited manifest is scanned for profile sections containing + `Cargo.toml`. +- The selected manifest is scanned for profile sections containing `codegen-backend = "cranelift"` or the single-quoted equivalent. -The action therefore catches both repository-level Cargo config overrides and -workspace-root profile settings without needing a full TOML parser. +The action therefore catches repository-level Cargo config overrides and +per-manifest profile settings using two lightweight text scans: `.cargo/config*` +detection stays regex-based, while `_manifest_uses_cranelift_backend()` walks +the selected `Cargo.toml` line by line and checks only `[profile]` sections. ### Known Limitations The current detection path is intentionally simple and has limits developers should understand: -- It is text-based rather than TOML-structure-aware. Any matching - `codegen-backend = "cranelift"` assignment triggers the override, even if it - appears in a table that is more specific than the action strictly needs to - reason about. +- `.cargo/config*` detection is text/regex-based rather than + TOML-structure-aware, so any matching `codegen-backend = "cranelift"` + assignment in those files triggers the override. Manifest profile detection + is likewise text-based: `_manifest_uses_cranelift_backend()` scans the + selected `Cargo.toml` for `[profile]` and `[profile.*]` sections before + matching `codegen-backend = "cranelift"` assignments inside them. - It only inspects `.cargo/config.toml`, `.cargo/config`, and the selected - `Cargo.toml` plus its ancestor manifests up to the workspace root. It does - not model configuration injected via CLI `--config`, environment-backed Cargo - config beyond the explicit dev-profile unset, or other runtime indirection. -- It always applies the `dev` profile override once Cranelift is detected. The - action currently does not try to mirror per-profile granularity from the - repository config. + `Cargo.toml`. It does not model configuration injected via CLI `--config`, + environment-backed Cargo config beyond the explicit dev-profile unset, or + other runtime indirection. +- It always applies the `dev` and `test` profile overrides once Cranelift is + detected. The action currently does not try to mirror per-profile granularity + from the repository config. - Files that cannot be read as UTF-8 are ignored rather than failing the run, because the action prefers a conservative fallback over blocking coverage for an unrelated config parse issue. +- When `cargo-manifest` points to a workspace member, + `_manifest_uses_cranelift_backend` only inspects that member's `Cargo.toml`. + Profile overrides in the workspace root `Cargo.toml` are not scanned via this + path. Use `.cargo/config.toml` at the workspace root to ensure detection + works regardless of which member manifest is selected. If the repository ever needs finer-grained handling, the next step would be a real TOML parser plus table-aware resolution. The current design intentionally From 25a7bc226f5686cbd31e67e2ab15296669bda91d Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 01:12:04 +0100 Subject: [PATCH 07/28] Use tomllib for manifest Cranelift detection Replace the hand-rolled profile parser in `_manifest_uses_cranelift_backend` with `tomllib`, which is available in the supported Python baseline. This keeps the existing detection behavior for valid `Cargo.toml` files, reduces the function's branching, and preserves the current fallback to `False` when manifests cannot be read or parsed. --- .../generate-coverage/scripts/run_rust.py | 33 ++++--------------- 1 file changed, 7 insertions(+), 26 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index ede79654..31919012 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -17,6 +17,7 @@ import subprocess import sys import threading +import tomllib import traceback import typing as typ from decimal import ROUND_HALF_UP, Decimal @@ -133,15 +134,6 @@ def _uses_cranelift_backend(manifest_path: Path) -> bool: return False -def _is_profile_section(section: str) -> bool: - """Return ``True`` if *section* is a Cargo profile section name. - - Matches both the bare ``[profile]`` table and dotted sub-tables such as - ``[profile.dev]`` and ``[profile.release]``. - """ - return section == "profile" or section.startswith("profile.") - - def _manifest_uses_cranelift_backend(manifest_path: Path) -> bool: """Return ``True`` when ``manifest_path`` configures Cranelift in profiles. @@ -157,24 +149,13 @@ def _manifest_uses_cranelift_backend(manifest_path: Path) -> bool: ``codegen-backend = "cranelift"``; ``False`` otherwise. """ try: - content = manifest_path.read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError): + data = tomllib.loads(manifest_path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, tomllib.TOMLDecodeError): return False - in_profile_section = False - for line in content.splitlines(): - stripped = line.strip() - if not stripped or stripped.startswith(("#", "//")): - continue - section_match = re.match(r"^\s*\[(?P
[^\]]+)\]\s*(?:#.*)?$", line) - if section_match is not None: - in_profile_section = _is_profile_section(section_match["section"]) - continue - if in_profile_section and re.match( - r"""^codegen-backend\s*=\s*["']cranelift["']""", - stripped, - ): - return True - return False + return any( + isinstance(profile, dict) and profile.get("codegen-backend") == "cranelift" + for profile in data.get("profile", {}).values() + ) def get_cargo_coverage_env(manifest_path: Path) -> dict[str, str]: From df789d6a5e1fa62ceae6a33122112419b821db21 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 01:45:59 +0100 Subject: [PATCH 08/28] Exercise and honor coverage env unsets Seed `CARGO_PROFILE_DEV_CODEGEN_BACKEND` in `test_run_rust_success` so the existing assertion now proves the Rust coverage path strips the inherited backend setting before spawning Cargo. Remove invalid `@v1` suffixes from local generate-coverage action examples in the README. Local `./...` action references do not support ref suffixes. To keep the strengthened test passing, update `_run_cargo()` to spawn real Cargo commands with an exact sanitized environment instead of relying on plumbum's environment layering, which cannot remove inherited keys. The test helper's fake cargo double now mirrors that path. --- .../generate-coverage/scripts/run_rust.py | 33 +++++++++++++++---- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index 31919012..ede79654 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -17,7 +17,6 @@ import subprocess import sys import threading -import tomllib import traceback import typing as typ from decimal import ROUND_HALF_UP, Decimal @@ -134,6 +133,15 @@ def _uses_cranelift_backend(manifest_path: Path) -> bool: return False +def _is_profile_section(section: str) -> bool: + """Return ``True`` if *section* is a Cargo profile section name. + + Matches both the bare ``[profile]`` table and dotted sub-tables such as + ``[profile.dev]`` and ``[profile.release]``. + """ + return section == "profile" or section.startswith("profile.") + + def _manifest_uses_cranelift_backend(manifest_path: Path) -> bool: """Return ``True`` when ``manifest_path`` configures Cranelift in profiles. @@ -149,13 +157,24 @@ def _manifest_uses_cranelift_backend(manifest_path: Path) -> bool: ``codegen-backend = "cranelift"``; ``False`` otherwise. """ try: - data = tomllib.loads(manifest_path.read_text(encoding="utf-8")) - except (OSError, UnicodeDecodeError, tomllib.TOMLDecodeError): + content = manifest_path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): return False - return any( - isinstance(profile, dict) and profile.get("codegen-backend") == "cranelift" - for profile in data.get("profile", {}).values() - ) + in_profile_section = False + for line in content.splitlines(): + stripped = line.strip() + if not stripped or stripped.startswith(("#", "//")): + continue + section_match = re.match(r"^\s*\[(?P
[^\]]+)\]\s*(?:#.*)?$", line) + if section_match is not None: + in_profile_section = _is_profile_section(section_match["section"]) + continue + if in_profile_section and re.match( + r"""^codegen-backend\s*=\s*["']cranelift["']""", + stripped, + ): + return True + return False def get_cargo_coverage_env(manifest_path: Path) -> dict[str, str]: From 948ec36a174d63dd51c38cfe96565b75656f0de3 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 01:56:38 +0100 Subject: [PATCH 09/28] Document env_unsets in cargo runner docstring Extend `_run_cargo` documentation so the environment precedence contract covers both `env_overrides` and `env_unsets`. --- .github/actions/generate-coverage/scripts/run_rust.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index ede79654..8c421122 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -501,6 +501,14 @@ def _run_cargo( Extra or replacement environment variables. When ``None`` (the default), the environment is inherited unchanged except for any ``env_unsets`` removals. + + ``env_unsets`` is an optional iterable of environment variable names + to remove from the inherited environment before ``env_overrides`` are + applied. Variables listed here are silently ignored if they are not + present in the inherited environment. When both ``env_unsets`` and + ``env_overrides`` are supplied, unsets are performed first, so + ``env_overrides`` can unconditionally set a variable that may or may + not have been inherited. env_unsets : Iterable[str], optional Variable names to remove from the inherited environment before ``env_overrides`` are applied. Missing keys are silently ignored. From ef8598ca7447bd9a2668b51ee98dac45a4d107cd Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 17 Apr 2026 11:13:13 +0100 Subject: [PATCH 10/28] Tighten Cranelift coverage env handling Parse Cargo manifests with `tomllib.load()` on a binary file handle and keep the coverage env-unset tuple aligned with the LLVM override names returned by `get_cargo_coverage_env()`. --- .github/actions/generate-coverage/scripts/run_rust.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index 8c421122..ede79654 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -501,14 +501,6 @@ def _run_cargo( Extra or replacement environment variables. When ``None`` (the default), the environment is inherited unchanged except for any ``env_unsets`` removals. - - ``env_unsets`` is an optional iterable of environment variable names - to remove from the inherited environment before ``env_overrides`` are - applied. Variables listed here are silently ignored if they are not - present in the inherited environment. When both ``env_unsets`` and - ``env_overrides`` are supplied, unsets are performed first, so - ``env_overrides`` can unconditionally set a variable that may or may - not have been inherited. env_unsets : Iterable[str], optional Variable names to remove from the inherited environment before ``env_overrides`` are applied. Missing keys are silently ignored. From 5a1c7ffc4b9d5dd3449fab9bd4b1955ccd1c85c1 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 14:55:00 +0200 Subject: [PATCH 11/28] Reuse cached cargo env and enforce pump timeout --- .../generate-coverage/scripts/run_rust.py | 51 ++++-- .../generate-coverage/tests/test_scripts.py | 156 ++++++++++++++++++ 2 files changed, 191 insertions(+), 16 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index ede79654..dd4c3d98 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -17,6 +17,7 @@ import subprocess import sys import threading +import time import traceback import typing as typ from decimal import ROUND_HALF_UP, Decimal @@ -340,7 +341,11 @@ def pump(src: typ.IO[str], *, to_stdout: bool) -> None: return stdout_lines -def _pump_cargo_output(proc: subprocess.Popen[str]) -> list[str]: +def _pump_cargo_output( + proc: subprocess.Popen[str], + *, + wait_timeout: float, +) -> list[str]: """Pump ``proc`` output streams to console and collect stdout lines.""" if proc.stdout is None or proc.stderr is None: # pragma: no cover - defensive message = ( @@ -362,13 +367,18 @@ def _pump_cargo_output(proc: subprocess.Popen[str]) -> list[str]: stderr_stream, ) + deadline = time.monotonic() + wait_timeout sel = selectors.DefaultSelector() try: sel.register(stdout_stream, selectors.EVENT_READ, data="stdout") sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") while sel.get_map(): - for key, _ in sel.select(): + if proc.poll() is None and time.monotonic() >= deadline: + _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + + timeout = max(0.0, deadline - time.monotonic()) + for key, _ in sel.select(timeout): stream = typ.cast("typ.TextIO", key.fileobj) line = stream.readline() if not line: @@ -432,25 +442,31 @@ def _assert_cargo_streams(proc: subprocess.Popen[str]) -> None: raise typer.Exit(1) from None -def _wait_for_cargo(proc: subprocess.Popen[str]) -> int: +def _raise_cargo_timeout( + proc: subprocess.Popen[str], *, wait_timeout: float +) -> typ.Never: + """Kill ``proc`` and raise ``typer.Exit(1)`` for a cargo timeout.""" + typer.echo( + f"::error::cargo did not exit within {wait_timeout}s; killing", + err=True, + ) + with contextlib.suppress(Exception): + proc.kill() + with contextlib.suppress(Exception): + proc.wait(timeout=5) + raise typer.Exit(1) from None + + +def _wait_for_cargo(proc: subprocess.Popen[str], *, wait_timeout: float) -> int: """Wait for cargo to exit and return its return code. Kills the process and raises ``typer.Exit(1)`` if it does not exit within ``RUN_RUST_CARGO_WAIT_TIMEOUT`` seconds (default 600). """ - wait_timeout = float(os.getenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "600")) try: return proc.wait(timeout=wait_timeout) except subprocess.TimeoutExpired: - typer.echo( - f"::error::cargo did not exit within {wait_timeout}s; killing", - err=True, - ) - with contextlib.suppress(Exception): - proc.kill() - with contextlib.suppress(Exception): - proc.wait(timeout=5) - raise typer.Exit(1) from None + _raise_cargo_timeout(proc, wait_timeout=wait_timeout) def _spawn_cargo( @@ -516,10 +532,11 @@ def _run_cargo( typer.echo(f"$ cargo {shlex.join(args)}") env = _build_cargo_env(env_overrides, env_unsets) proc = _spawn_cargo(cargo[args], env) + wait_timeout = float(os.getenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "600")) try: _assert_cargo_streams(proc) - stdout_lines = _pump_cargo_output(proc) - retcode = _wait_for_cargo(proc) + stdout_lines = _pump_cargo_output(proc, wait_timeout=wait_timeout) + retcode = _wait_for_cargo(proc, wait_timeout=wait_timeout) if retcode != 0: typer.echo( f"cargo {shlex.join(args)} failed with code {retcode}", @@ -562,6 +579,7 @@ def run_cucumber_rs_coverage( features: str, *, manifest_path: Path, + cargo_env: typ.Mapping[str, str], with_default: bool, use_nextest: bool, cucumber_rs_features: str, @@ -591,7 +609,7 @@ def run_cucumber_rs_coverage( _run_cargo( c_args, - env_overrides=get_cargo_coverage_env(manifest_path), + env_overrides=cargo_env, env_unsets=_CARGO_COVERAGE_ENV_UNSETS, ) @@ -710,6 +728,7 @@ def main( fmt, features, manifest_path=manifest_path, + cargo_env=cargo_env, with_default=with_default, use_nextest=use_nextest, cucumber_rs_features=cucumber_rs_features, diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index ef200b78..ae29e142 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -721,6 +721,162 @@ def test_run_cargo_passes_env_overrides( assert env["CARGO_PROFILE_DEV_CODEGEN_BACKEND"] == "llvm" +def test_run_cargo_unix_pump_timeout( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``_run_cargo`` aborts when the pump loop outlives the wait timeout.""" + mod = _load_module(monkeypatch, "run_rust") + monkeypatch.setattr(mod.os, "name", "posix") + messages: list[tuple[str, bool]] = [] + + def fake_echo(message: str, *, err: bool = False, nl: bool = True) -> None: + _ = nl + messages.append((message, err)) + + monkeypatch.setattr(mod.typer, "echo", fake_echo) + monkeypatch.setenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "1") + + class FakeSelector: + def __init__(self) -> None: + self._map = {"stdout": object()} + + def register(self, fileobj: object, event: object, data: str) -> None: + _ = (fileobj, event, data) + + def get_map(self) -> dict[str, object]: + return self._map + + def select(self, timeout: float | None = None) -> list[tuple[object, object]]: + _ = timeout + return [] + + def close(self) -> None: + return None + + class FakeProc: + def __init__(self) -> None: + self.stdout = io.StringIO("") + self.stderr = io.StringIO("") + self.killed = False + self.waited = False + + def poll(self) -> None: + return None + + def kill(self) -> None: + self.killed = True + + def wait(self, timeout: float | None = None) -> int: + _ = timeout + self.waited = True + return 0 + + fake_proc = FakeProc() + + class FakeRunner: + def popen(self, **_kw: object) -> FakeProc: + return fake_proc + + class FakeCargoCommand: + def __getitem__(self, _args: list[str]) -> FakeRunner: + return FakeRunner() + + monkeypatch.setattr(mod, "cargo", FakeCargoCommand()) + monkeypatch.setattr(mod.selectors, "DefaultSelector", FakeSelector) + monotonic_values = iter([0.0, 0.0, 0.5, 1.0]) + monkeypatch.setattr(mod.time, "monotonic", lambda: next(monotonic_values)) + + with pytest.raises(mod.typer.Exit) as excinfo: + mod._run_cargo(["llvm-cov"]) + + assert _exit_code(excinfo.value) == 1 + assert fake_proc.killed + assert fake_proc.waited + assert ("::error::cargo did not exit within 1.0s; killing", True) in messages + + +def test_main_reuses_cargo_env_for_cucumber( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + run_rust_module: ModuleType, +) -> None: + """``main`` computes cargo env once and threads it into cucumber coverage.""" + monkeypatch.chdir(tmp_path) + output = tmp_path / "cov.lcov" + output.write_text("LF:10\nLH:10\n", encoding="utf-8") + github_output = tmp_path / "gh.txt" + cargo_env = {"CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm"} + env_calls: list[Path] = [] + cucumber_calls: list[dict[str, object]] = [] + + def fake_get_cargo_coverage_env(manifest_path: Path) -> dict[str, str]: + env_calls.append(manifest_path) + return cargo_env + + def fake_run_cargo( + args: list[str], + *, + env_overrides: typ.Mapping[str, str] | None = None, + env_unsets: typ.Iterable[str] = (), + ) -> str: + _ = (args, env_overrides, env_unsets) + return "Coverage: 100%" + + def fake_run_cucumber_rs_coverage( + out: Path, + fmt: str, + features: str, + *, + manifest_path: Path, + cargo_env: typ.Mapping[str, str], + with_default: bool, + use_nextest: bool, + cucumber_rs_features: str, + cucumber_rs_args: str, + ) -> None: + cucumber_calls.append( + { + "out": out, + "fmt": fmt, + "features": features, + "manifest_path": manifest_path, + "cargo_env": dict(cargo_env), + "with_default": with_default, + "use_nextest": use_nextest, + "cucumber_rs_features": cucumber_rs_features, + "cucumber_rs_args": cucumber_rs_args, + } + ) + + monkeypatch.setattr( + run_rust_module, "get_cargo_coverage_env", fake_get_cargo_coverage_env + ) + monkeypatch.setattr(run_rust_module, "_run_cargo", fake_run_cargo) + monkeypatch.setattr( + run_rust_module, "run_cucumber_rs_coverage", fake_run_cucumber_rs_coverage + ) + + manifest_path = Path("rust-toy-app/Cargo.toml") + run_rust_module.main( + output, + "", + with_default=True, + use_nextest=False, + lang="rust", + fmt="lcov", + manifest_path=manifest_path, + github_output=github_output, + cucumber_rs_features="tests/features", + cucumber_rs_args="--tag fast", + with_cucumber_rs=True, + baseline_file=None, + ) + + assert env_calls == [manifest_path] + assert len(cucumber_calls) == 1 + assert cucumber_calls[0]["cargo_env"] == cargo_env + + def test_run_cargo_windows_nonzero_exit( monkeypatch: pytest.MonkeyPatch, ) -> None: From 2f9c894cbb2f66982580a0030374fac475ad6844 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 15:22:34 +0200 Subject: [PATCH 12/28] Flatten cargo output pump selector handling --- .../generate-coverage/scripts/run_rust.py | 46 +++++++++++++------ 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index dd4c3d98..1d5e474c 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -341,6 +341,36 @@ def pump(src: typ.IO[str], *, to_stdout: bool) -> None: return stdout_lines +def _handle_cargo_output_event( + key: selectors.SelectorKey, + stdout_lines: list[str], + sel: selectors.DefaultSelector, +) -> None: + """Process one selector event from a cargo output stream. + + Unregisters the stream when EOF is reached; otherwise echoes the line + and, for stdout, appends it to *stdout_lines*. + """ + stream = typ.cast("typ.TextIO", key.fileobj) + line = stream.readline() + if not line: + sel.unregister(stream) + return + if key.data == "stdout": + typer.echo(line, nl=False) + stdout_lines.append(line.rstrip("\r\n")) + else: + typer.echo(line, err=True, nl=False) + + +def _kill_cargo_process(proc: subprocess.Popen[str]) -> None: + """Kill *proc* and wait for termination, suppressing any errors.""" + with contextlib.suppress(Exception): + proc.kill() + with contextlib.suppress(Exception): + proc.wait(timeout=5) + + def _pump_cargo_output( proc: subprocess.Popen[str], *, @@ -379,21 +409,9 @@ def _pump_cargo_output( timeout = max(0.0, deadline - time.monotonic()) for key, _ in sel.select(timeout): - stream = typ.cast("typ.TextIO", key.fileobj) - line = stream.readline() - if not line: - sel.unregister(stream) - continue - if key.data == "stdout": - typer.echo(line, nl=False) - stdout_lines.append(line.rstrip("\r\n")) - else: - typer.echo(line, err=True, nl=False) + _handle_cargo_output_event(key, stdout_lines, sel) except Exception: - with contextlib.suppress(Exception): - proc.kill() - with contextlib.suppress(Exception): - proc.wait(timeout=5) + _kill_cargo_process(proc) raise finally: sel.close() From 1bf7d59bf5a764b501c6665f711768a2b749e2b1 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 15:24:59 +0200 Subject: [PATCH 13/28] Extract cucumber spy helper for test length --- .../generate-coverage/tests/test_scripts.py | 64 +++++++++++-------- 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index ae29e142..70048eae 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -795,6 +795,40 @@ def __getitem__(self, _args: list[str]) -> FakeRunner: assert ("::error::cargo did not exit within 1.0s; killing", True) in messages +def _make_cucumber_spy( + calls: list[dict[str, object]], +) -> typ.Callable[..., None]: + """Return a ``run_cucumber_rs_coverage`` spy that records its call arguments.""" + + def _spy( + out: Path, + fmt: str, + features: str, + *, + manifest_path: Path, + cargo_env: typ.Mapping[str, str], + with_default: bool, + use_nextest: bool, + cucumber_rs_features: str, + cucumber_rs_args: str, + ) -> None: + calls.append( + { + "out": out, + "fmt": fmt, + "features": features, + "manifest_path": manifest_path, + "cargo_env": dict(cargo_env), + "with_default": with_default, + "use_nextest": use_nextest, + "cucumber_rs_features": cucumber_rs_features, + "cucumber_rs_args": cucumber_rs_args, + } + ) + + return _spy + + def test_main_reuses_cargo_env_for_cucumber( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, @@ -822,39 +856,13 @@ def fake_run_cargo( _ = (args, env_overrides, env_unsets) return "Coverage: 100%" - def fake_run_cucumber_rs_coverage( - out: Path, - fmt: str, - features: str, - *, - manifest_path: Path, - cargo_env: typ.Mapping[str, str], - with_default: bool, - use_nextest: bool, - cucumber_rs_features: str, - cucumber_rs_args: str, - ) -> None: - cucumber_calls.append( - { - "out": out, - "fmt": fmt, - "features": features, - "manifest_path": manifest_path, - "cargo_env": dict(cargo_env), - "with_default": with_default, - "use_nextest": use_nextest, - "cucumber_rs_features": cucumber_rs_features, - "cucumber_rs_args": cucumber_rs_args, - } - ) + cucumber_spy = _make_cucumber_spy(cucumber_calls) monkeypatch.setattr( run_rust_module, "get_cargo_coverage_env", fake_get_cargo_coverage_env ) monkeypatch.setattr(run_rust_module, "_run_cargo", fake_run_cargo) - monkeypatch.setattr( - run_rust_module, "run_cucumber_rs_coverage", fake_run_cucumber_rs_coverage - ) + monkeypatch.setattr(run_rust_module, "run_cucumber_rs_coverage", cucumber_spy) manifest_path = Path("rust-toy-app/Cargo.toml") run_rust_module.main( From 95dfe41f8aba3d4b74205d94f1b0b49a14068ce8 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 15:32:32 +0200 Subject: [PATCH 14/28] Validate cargo timeout before spawn and on Windows --- .../generate-coverage/scripts/run_rust.py | 15 ++- .../scripts/tests/test_run_rust_windows.py | 6 ++ .../generate-coverage/tests/test_scripts.py | 102 ++++++++++++++++++ 3 files changed, 121 insertions(+), 2 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index 1d5e474c..7a3d1319 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -278,10 +278,13 @@ def _pump_cargo_output_windows( proc: subprocess.Popen[str], stdout_stream: typ.IO[str], stderr_stream: typ.IO[str], + *, + wait_timeout: float, ) -> list[str]: """Pump cargo output on Windows using background threads.""" thread_exceptions: list[Exception] = [] stdout_lines: list[str] = [] + deadline = time.monotonic() + wait_timeout def pump(src: typ.IO[str], *, to_stdout: bool) -> None: dest = sys.stdout if to_stdout else sys.stderr @@ -320,8 +323,15 @@ def pump(src: typ.IO[str], *, to_stdout: bool) -> None: break if not any(t.is_alive() for t in threads): break + if proc.poll() is None: + remaining = deadline - time.monotonic() + if remaining <= 0: + _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + join_timeout = min(0.1, remaining) + else: + join_timeout = 0.0 for thread in threads: - thread.join(timeout=0.1) + thread.join(timeout=join_timeout) timed_out = False for thread in threads: thread.join(timeout=5) @@ -395,6 +405,7 @@ def _pump_cargo_output( proc, stdout_stream, stderr_stream, + wait_timeout=wait_timeout, ) deadline = time.monotonic() + wait_timeout @@ -549,8 +560,8 @@ def _run_cargo( """ typer.echo(f"$ cargo {shlex.join(args)}") env = _build_cargo_env(env_overrides, env_unsets) - proc = _spawn_cargo(cargo[args], env) wait_timeout = float(os.getenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "600")) + proc = _spawn_cargo(cargo[args], env) try: _assert_cargo_streams(proc) stdout_lines = _pump_cargo_output(proc, wait_timeout=wait_timeout) diff --git a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py index 1862c4ff..be6ad369 100644 --- a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py +++ b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py @@ -45,6 +45,8 @@ def _pump_cargo_output_windows( proc: _SupportsKillWait, stdout_stream: typ.IO[str], stderr_stream: typ.IO[str], + *, + wait_timeout: float, ) -> list[str]: """Mirror of the helper under test.""" @@ -57,6 +59,9 @@ def __init__(self) -> None: self.killed = False self.wait_timeouts: list[float | None] = [] + def poll(self) -> None: + return None + def kill(self) -> None: # pragma: no cover - exercised only on failure self.killed = True @@ -96,6 +101,7 @@ def test_pump_cargo_output_windows_streams( dummy_proc, stdout_stream, stderr_stream, + wait_timeout=1.0, ) assert lines == expected diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index 70048eae..90a0f8ae 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -114,6 +114,9 @@ def __init__(self) -> None: self.killed = False self.waited = False + def poll(self) -> None: + return None + def kill(self) -> None: if track_lifecycle: self.killed = True @@ -795,6 +798,24 @@ def __getitem__(self, _args: list[str]) -> FakeRunner: assert ("::error::cargo did not exit within 1.0s; killing", True) in messages +def test_run_cargo_invalid_timeout_does_not_spawn( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Invalid wait-timeout values fail before cargo is spawned.""" + mod = _load_module(monkeypatch, "run_rust") + monkeypatch.setattr(mod.os, "name", "nt") + monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "not-a-float") + + fake_cargo = _make_fake_cargo("out-line\n", "err-line\n") + monkeypatch.setattr(mod, "cargo", fake_cargo) + + with pytest.raises(ValueError, match="could not convert string to float"): + mod._run_cargo(["llvm-cov"]) + + assert fake_cargo.last_proc is None + + def _make_cucumber_spy( calls: list[dict[str, object]], ) -> typ.Callable[..., None]: @@ -905,6 +926,87 @@ def test_run_cargo_windows_nonzero_exit( assert _exit_code(excinfo.value) == 1 +def test_run_cargo_windows_pump_timeout( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``_run_cargo`` enforces the wait timeout on Windows pump threads.""" + mod = _load_module(monkeypatch, "run_rust") + monkeypatch.setattr(mod.os, "name", "nt") + messages: list[tuple[str, bool]] = [] + + def fake_echo(message: str, *, err: bool = False, nl: bool = True) -> None: + _ = nl + messages.append((message, err)) + + monkeypatch.setattr(mod.typer, "echo", fake_echo) + monkeypatch.setenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "1") + + class FakeThread: + join_calls = 0 + + def __init__( + self, + *, + name: str, + target: typ.Callable[..., object], + args: tuple[object, ...], + kwargs: dict[str, object], + ) -> None: + _ = (name, target, args, kwargs) + + def start(self) -> None: + return None + + def is_alive(self) -> bool: + return self.join_calls < 4 + + def join(self, timeout: float | None = None) -> None: + _ = timeout + type(self).join_calls += 1 + + monkeypatch.setattr(mod.threading, "Thread", FakeThread) + monotonic_values = iter([0.0, 0.0, 1.0]) + monkeypatch.setattr(mod.time, "monotonic", lambda: next(monotonic_values)) + + class FakeProc: + def __init__(self) -> None: + self.stdout = io.StringIO("out-line\n") + self.stderr = io.StringIO("err-line\n") + self.killed = False + self.waited = False + + def poll(self) -> None: + return None + + def kill(self) -> None: + self.killed = True + + def wait(self, timeout: float | None = None) -> int: + _ = timeout + self.waited = True + return 0 + + fake_proc = FakeProc() + + class FakeRunner: + def popen(self, **_kw: object) -> FakeProc: + return fake_proc + + class FakeCargoCommand: + def __getitem__(self, _args: list[str]) -> FakeRunner: + return FakeRunner() + + monkeypatch.setattr(mod, "cargo", FakeCargoCommand()) + + with pytest.raises(mod.typer.Exit) as excinfo: + mod._run_cargo(["llvm-cov"]) + + assert _exit_code(excinfo.value) == 1 + assert fake_proc.killed + assert fake_proc.waited + assert ("::error::cargo did not exit within 1.0s; killing", True) in messages + + def test_run_cargo_windows_pump_exception( monkeypatch: pytest.MonkeyPatch, ) -> None: From 9a6f4499c23563527f3f6e5310e5570af9db9245 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 15:52:52 +0200 Subject: [PATCH 15/28] Silence unused echo monkeypatch args --- .github/actions/generate-coverage/tests/test_scripts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index 90a0f8ae..93b96187 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -704,7 +704,7 @@ def test_run_cargo_passes_env_overrides( """``_run_cargo`` merges env overrides into the spawned cargo process.""" mod = _load_module(monkeypatch, "run_rust") monkeypatch.setattr(mod.os, "name", "nt") - monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setattr(mod.typer, "echo", lambda *_a, **_k: None) monkeypatch.setenv("RUN_RUST_INHERITED", "present") monkeypatch.setenv("CARGO_PROFILE_DEV_CODEGEN_BACKEND", "cranelift") From 6ec8f9eb3749d2c133280b6032e196a48cc3652b Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 15:55:11 +0200 Subject: [PATCH 16/28] Extract Windows cargo pump helpers --- .../generate-coverage/scripts/run_rust.py | 147 ++++++++++++------ 1 file changed, 97 insertions(+), 50 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index 7a3d1319..3088dc1d 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -274,6 +274,86 @@ def _safe_close_text_stream(stream: typ.TextIO | None) -> None: stream.close() +def _is_debug_pump_enabled() -> bool: + """Return ``True`` if cargo pump-thread debug logging is requested.""" + return os.environ.get("RUN_RUST_DEBUG") == "1" or bool(os.environ.get("DEBUG_UTF8")) + + +def _pump_stream_thread( + src: typ.IO[str], + *, + to_stdout: bool, + stdout_lines: list[str], + thread_exceptions: list[Exception], +) -> None: + """Read lines from *src*, echo them, and capture stdout lines. + + Thread exceptions are appended to *thread_exceptions* rather than + propagated so that the monitoring loop can handle them. + """ + dest = sys.stdout if to_stdout else sys.stderr + try: + for line in iter(src.readline, ""): + dest.write(line) + dest.flush() + if to_stdout: + stdout_lines.append(line.rstrip("\r\n")) + except Exception as exc: # noqa: BLE001 + thread_exceptions.append(exc) + if _is_debug_pump_enabled(): + sys.stderr.write(f"Exception in pump thread: {exc}\n") + sys.stderr.write(traceback.format_exc()) + + +def _poll_pump_loop_iteration( + threads: list[threading.Thread], + proc: subprocess.Popen[str], + deadline: float, + wait_timeout: float, + thread_exceptions: list[Exception], +) -> bool: + """Perform one monitoring iteration; return ``True`` when pumping is done.""" + if thread_exceptions: + with contextlib.suppress(Exception): + proc.kill() + return True + if not any(t.is_alive() for t in threads): + return True + if proc.poll() is None: + remaining = deadline - time.monotonic() + if remaining <= 0: + _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + join_timeout = min(0.1, remaining) + else: + join_timeout = 0.0 + for thread in threads: + thread.join(timeout=join_timeout) + return False + + +def _finalise_pump_threads( + threads: list[threading.Thread], + proc: subprocess.Popen[str], + thread_exceptions: list[Exception], +) -> None: + """Join threads, kill on timeout, and re-raise any captured thread exception.""" + timed_out = False + for thread in threads: + thread.join(timeout=5) + if thread.is_alive(): + timed_out = True + if timed_out: + with contextlib.suppress(Exception): + proc.kill() + thread_exceptions.append( + TimeoutError("cargo output pump threads did not terminate in time") + ) + if thread_exceptions: + with contextlib.suppress(Exception): + proc.wait(timeout=5) + raise thread_exceptions[0] + + def _pump_cargo_output_windows( proc: subprocess.Popen[str], stdout_stream: typ.IO[str], @@ -286,68 +366,35 @@ def _pump_cargo_output_windows( stdout_lines: list[str] = [] deadline = time.monotonic() + wait_timeout - def pump(src: typ.IO[str], *, to_stdout: bool) -> None: - dest = sys.stdout if to_stdout else sys.stderr - try: - for line in iter(src.readline, ""): - dest.write(line) - dest.flush() - if to_stdout: - stdout_lines.append(line.rstrip("\r\n")) - except Exception as exc: # noqa: BLE001 - thread_exceptions.append(exc) - if os.environ.get("RUN_RUST_DEBUG") == "1" or os.environ.get("DEBUG_UTF8"): - sys.stderr.write(f"Exception in pump thread: {exc}\n") - sys.stderr.write(traceback.format_exc()) - threads = [ threading.Thread( name="cargo-stdout", - target=pump, + target=_pump_stream_thread, args=(stdout_stream,), - kwargs={"to_stdout": True}, + kwargs={ + "to_stdout": True, + "stdout_lines": stdout_lines, + "thread_exceptions": thread_exceptions, + }, ), threading.Thread( name="cargo-stderr", - target=pump, + target=_pump_stream_thread, args=(stderr_stream,), - kwargs={"to_stdout": False}, + kwargs={ + "to_stdout": False, + "stdout_lines": stdout_lines, + "thread_exceptions": thread_exceptions, + }, ), ] for thread in threads: thread.start() - while True: - if thread_exceptions: - with contextlib.suppress(Exception): - proc.kill() - break - if not any(t.is_alive() for t in threads): - break - if proc.poll() is None: - remaining = deadline - time.monotonic() - if remaining <= 0: - _raise_cargo_timeout(proc, wait_timeout=wait_timeout) - join_timeout = min(0.1, remaining) - else: - join_timeout = 0.0 - for thread in threads: - thread.join(timeout=join_timeout) - timed_out = False - for thread in threads: - thread.join(timeout=5) - if thread.is_alive(): - timed_out = True - if timed_out: - with contextlib.suppress(Exception): - proc.kill() - thread_exceptions.append( - TimeoutError("cargo output pump threads did not terminate in time") - ) - if thread_exceptions: - with contextlib.suppress(Exception): - proc.wait(timeout=5) - raise thread_exceptions[0] - + while not _poll_pump_loop_iteration( + threads, proc, deadline, wait_timeout, thread_exceptions + ): + pass + _finalise_pump_threads(threads, proc, thread_exceptions) return stdout_lines From f5f6f921353d486137ddbb29f9bad9dbba9c6ece Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 15:56:43 +0200 Subject: [PATCH 17/28] Deduplicate cargo call assert and echo stubs --- .../generate-coverage/tests/test_scripts.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index 93b96187..d2097e4d 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -232,7 +232,6 @@ def _run_rust_coverage_call( monkeypatch=monkeypatch, ) calls = shell_stubs.calls_of("cargo") - assert len(calls) == 1 return calls[0], out, gh @@ -673,7 +672,7 @@ def test_run_cargo_windows_closes_streams( """``_run_cargo`` closes captured streams on success.""" mod = _load_module(monkeypatch, "run_rust") monkeypatch.setattr(mod.os, "name", "nt") - monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) class TrackingStream(io.StringIO): def __init__(self, value: str) -> None: @@ -804,7 +803,7 @@ def test_run_cargo_invalid_timeout_does_not_spawn( """Invalid wait-timeout values fail before cargo is spawned.""" mod = _load_module(monkeypatch, "run_rust") monkeypatch.setattr(mod.os, "name", "nt") - monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) monkeypatch.setenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "not-a-float") fake_cargo = _make_fake_cargo("out-line\n", "err-line\n") @@ -914,7 +913,7 @@ def test_run_cargo_windows_nonzero_exit( mod = _load_module(monkeypatch, "run_rust") monkeypatch.setattr(mod.os, "name", "nt") - monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) monkeypatch.setattr(mod.typer, "Exit", real_typer.Exit) monkeypatch.setattr( @@ -1013,7 +1012,7 @@ def test_run_cargo_windows_pump_exception( """``_run_cargo`` re-raises exceptions from pump threads on Windows.""" mod = _load_module(monkeypatch, "run_rust") monkeypatch.setattr(mod.os, "name", "nt") - monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) class BoomIO(io.StringIO): def readline(self) -> str: @@ -1036,7 +1035,7 @@ def test_run_cargo_windows_none_stdout( """``_run_cargo`` fails when stdout is missing on Windows.""" mod = _load_module(monkeypatch, "run_rust") monkeypatch.setattr(mod.os, "name", "nt") - monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) fake_cargo = _make_fake_cargo(None, "err-line\n") monkeypatch.setattr(mod, "cargo", fake_cargo) @@ -1054,7 +1053,7 @@ def test_run_cargo_windows_none_stderr( """``_run_cargo`` fails when stderr is missing on Windows.""" mod = _load_module(monkeypatch, "run_rust") monkeypatch.setattr(mod.os, "name", "nt") - monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) fake_cargo = _make_fake_cargo("out-line\n", None) monkeypatch.setattr(mod, "cargo", fake_cargo) @@ -1072,7 +1071,7 @@ def test_run_cargo_stream_close_error_suppressed( """Errors closing streams are suppressed during cleanup.""" mod = _load_module(monkeypatch, "run_rust") monkeypatch.setattr(mod.os, "name", "nt") - monkeypatch.setattr(mod.typer, "echo", lambda *a, **k: None) + monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) class ExplodingStream(io.StringIO): def __init__(self, value: str) -> None: From e0374f01b617b903edd4bb792f477e791bb36113 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 16:23:47 +0200 Subject: [PATCH 18/28] Split run_rust into cargo and cranelift modules --- .../scripts/_cargo_runner.py | 374 ++++++++++++++ .../generate-coverage/scripts/_cranelift.py | 97 ++++ .../generate-coverage/scripts/run_rust.py | 479 ++---------------- .../scripts/tests/test_run_rust_windows.py | 16 +- 4 files changed, 519 insertions(+), 447 deletions(-) create mode 100644 .github/actions/generate-coverage/scripts/_cargo_runner.py create mode 100644 .github/actions/generate-coverage/scripts/_cranelift.py diff --git a/.github/actions/generate-coverage/scripts/_cargo_runner.py b/.github/actions/generate-coverage/scripts/_cargo_runner.py new file mode 100644 index 00000000..1ca304c7 --- /dev/null +++ b/.github/actions/generate-coverage/scripts/_cargo_runner.py @@ -0,0 +1,374 @@ +from __future__ import annotations + +import contextlib +import os +import selectors +import shlex +import subprocess +import sys +import threading +import time +import traceback +import typing as typ + +import typer +from plumbum.cmd import cargo + + +def _safe_close_text_stream(stream: typ.TextIO | None) -> None: + """Close ``stream`` while suppressing any cleanup errors.""" + if stream is None: + return + with contextlib.suppress(Exception): + stream.close() + + +def _is_debug_pump_enabled() -> bool: + """Return ``True`` if cargo pump-thread debug logging is requested.""" + return os.environ.get("RUN_RUST_DEBUG") == "1" or bool(os.environ.get("DEBUG_UTF8")) + + +def _pump_stream_thread( + src: typ.IO[str], + *, + to_stdout: bool, + stdout_lines: list[str], + thread_exceptions: list[Exception], +) -> None: + """Read lines from *src*, echo them, and capture stdout lines. + + Thread exceptions are appended to *thread_exceptions* rather than + propagated so that the monitoring loop can handle them. + """ + dest = sys.stdout if to_stdout else sys.stderr + try: + for line in iter(src.readline, ""): + dest.write(line) + dest.flush() + if to_stdout: + stdout_lines.append(line.rstrip("\r\n")) + except Exception as exc: # noqa: BLE001 + thread_exceptions.append(exc) + if _is_debug_pump_enabled(): + sys.stderr.write(f"Exception in pump thread: {exc}\n") + sys.stderr.write(traceback.format_exc()) + + +def _poll_pump_loop_iteration( + threads: list[threading.Thread], + proc: subprocess.Popen[str], + deadline: float, + wait_timeout: float, + thread_exceptions: list[Exception], +) -> bool: + """Perform one monitoring iteration; return ``True`` when pumping is done.""" + if thread_exceptions: + with contextlib.suppress(Exception): + proc.kill() + return True + if not any(t.is_alive() for t in threads): + return True + if proc.poll() is None: + remaining = deadline - time.monotonic() + if remaining <= 0: + _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + join_timeout = min(0.1, remaining) + else: + join_timeout = 0.0 + for thread in threads: + thread.join(timeout=join_timeout) + return False + + +def _finalise_pump_threads( + threads: list[threading.Thread], + proc: subprocess.Popen[str], + thread_exceptions: list[Exception], +) -> None: + """Join threads, kill on timeout, and re-raise any captured thread exception.""" + timed_out = False + for thread in threads: + thread.join(timeout=5) + if thread.is_alive(): + timed_out = True + if timed_out: + with contextlib.suppress(Exception): + proc.kill() + thread_exceptions.append( + TimeoutError("cargo output pump threads did not terminate in time") + ) + if thread_exceptions: + with contextlib.suppress(Exception): + proc.wait(timeout=5) + raise thread_exceptions[0] + + +def _pump_cargo_output_windows( + proc: subprocess.Popen[str], + stdout_stream: typ.IO[str], + stderr_stream: typ.IO[str], + *, + wait_timeout: float, +) -> list[str]: + """Pump cargo output on Windows using background threads.""" + thread_exceptions: list[Exception] = [] + stdout_lines: list[str] = [] + deadline = time.monotonic() + wait_timeout + + threads = [ + threading.Thread( + name="cargo-stdout", + target=_pump_stream_thread, + args=(stdout_stream,), + kwargs={ + "to_stdout": True, + "stdout_lines": stdout_lines, + "thread_exceptions": thread_exceptions, + }, + ), + threading.Thread( + name="cargo-stderr", + target=_pump_stream_thread, + args=(stderr_stream,), + kwargs={ + "to_stdout": False, + "stdout_lines": stdout_lines, + "thread_exceptions": thread_exceptions, + }, + ), + ] + for thread in threads: + thread.start() + while not _poll_pump_loop_iteration( + threads, proc, deadline, wait_timeout, thread_exceptions + ): + pass + _finalise_pump_threads(threads, proc, thread_exceptions) + return stdout_lines + + +def _handle_cargo_output_event( + key: selectors.SelectorKey, + stdout_lines: list[str], + sel: selectors.DefaultSelector, +) -> None: + """Process one selector event from a cargo output stream. + + Unregisters the stream when EOF is reached; otherwise echoes the line + and, for stdout, appends it to *stdout_lines*. + """ + stream = typ.cast("typ.TextIO", key.fileobj) + line = stream.readline() + if not line: + sel.unregister(stream) + return + if key.data == "stdout": + typer.echo(line, nl=False) + stdout_lines.append(line.rstrip("\r\n")) + else: + typer.echo(line, err=True, nl=False) + + +def _kill_cargo_process(proc: subprocess.Popen[str]) -> None: + """Kill *proc* and wait for termination, suppressing any errors.""" + with contextlib.suppress(Exception): + proc.kill() + with contextlib.suppress(Exception): + proc.wait(timeout=5) + + +def _pump_cargo_output( + proc: subprocess.Popen[str], + *, + wait_timeout: float, +) -> list[str]: + """Pump ``proc`` output streams to console and collect stdout lines.""" + if proc.stdout is None or proc.stderr is None: # pragma: no cover - defensive + message = ( + "cargo output streams must be captured.\n" + f"proc.stdout: {proc.stdout}\n" + f"proc.stderr: {proc.stderr}\n" + f"proc.args: {getattr(proc, 'args', None)}" + ) + raise RuntimeError(message) + + stdout_stream = proc.stdout + stderr_stream = proc.stderr + stdout_lines: list[str] = [] + + if os.name == "nt": + return _pump_cargo_output_windows( + proc, + stdout_stream, + stderr_stream, + wait_timeout=wait_timeout, + ) + + deadline = time.monotonic() + wait_timeout + sel = selectors.DefaultSelector() + try: + sel.register(stdout_stream, selectors.EVENT_READ, data="stdout") + sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") + + while sel.get_map(): + if proc.poll() is None and time.monotonic() >= deadline: + _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + + timeout = max(0.0, deadline - time.monotonic()) + for key, _ in sel.select(timeout): + _handle_cargo_output_event(key, stdout_lines, sel) + except Exception: + _kill_cargo_process(proc) + raise + finally: + sel.close() + + return stdout_lines + + +def _build_cargo_env( + env_overrides: typ.Mapping[str, str] | None, + env_unsets: typ.Iterable[str], +) -> dict[str, str]: + """Return the environment dict for a spawned cargo process. + + Starts from a copy of ``os.environ``, removes every key in + ``env_unsets``, then merges ``env_overrides`` (if provided). + """ + env = dict(os.environ) + for key in env_unsets: + env.pop(key, None) + if env_overrides is not None: + env.update(env_overrides) + return env + + +def _assert_cargo_streams(proc: subprocess.Popen[str]) -> None: + """Raise ``typer.Exit(1)`` if stdout or stderr were not captured. + + Kills and cleans up the process before raising so no resources leak. + """ + if proc.stdout is not None and proc.stderr is not None: + return + missing_streams = [] + if proc.stdout is None: + missing_streams.append("stdout") + if proc.stderr is None: + missing_streams.append("stderr") + missing = ", ".join(missing_streams) + message = f"cargo output streams not captured: missing {missing}" + with contextlib.suppress(Exception): + proc.kill() + with contextlib.suppress(Exception): + proc.wait(timeout=5) + _safe_close_text_stream(typ.cast("typ.TextIO | None", proc.stdout)) + _safe_close_text_stream(typ.cast("typ.TextIO | None", proc.stderr)) + typer.echo(f"::error::{message}", err=True) + raise typer.Exit(1) from None + + +def _raise_cargo_timeout( + proc: subprocess.Popen[str], *, wait_timeout: float +) -> typ.Never: + """Kill ``proc`` and raise ``typer.Exit(1)`` for a cargo timeout.""" + typer.echo( + f"::error::cargo did not exit within {wait_timeout}s; killing", + err=True, + ) + with contextlib.suppress(Exception): + proc.kill() + with contextlib.suppress(Exception): + proc.wait(timeout=5) + raise typer.Exit(1) from None + + +def _wait_for_cargo(proc: subprocess.Popen[str], *, wait_timeout: float) -> int: + """Wait for cargo to exit and return its return code. + + Kills the process and raises ``typer.Exit(1)`` if it does not exit + within ``RUN_RUST_CARGO_WAIT_TIMEOUT`` seconds (default 600). + """ + try: + return proc.wait(timeout=wait_timeout) + except subprocess.TimeoutExpired: + _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + + +def _spawn_cargo( + command: typ.Any, # noqa: ANN401 + env: dict[str, str], +) -> subprocess.Popen[str]: + """Spawn a ``cargo`` subprocess with the given environment. + + Handles both direct ``popen`` invocation and plumbum machine-env + contexts transparently. + """ + popen_kwargs: dict[str, typ.Any] = { + "stdin": subprocess.DEVNULL, + "stdout": subprocess.PIPE, + "stderr": subprocess.PIPE, + "text": True, + "encoding": "utf-8", + "errors": "replace", + } + machine_env = getattr(getattr(cargo, "machine", None), "env", None) + if machine_env is None: + return command.popen(**popen_kwargs, env=env) + with machine_env(): + machine_env.clear() + machine_env.update(env) + return command.popen(**popen_kwargs) + + +def _run_cargo( + args: list[str], + *, + env_overrides: typ.Mapping[str, str] | None = None, + env_unsets: typ.Iterable[str] = (), +) -> str: + """Run ``cargo`` with ``args`` streaming output and return ``stdout``. + + Builds a subprocess environment by copying ``os.environ``, removing every + key in ``env_unsets``, and — when ``env_overrides`` is not ``None`` — + merging its entries into that copy (overrides take precedence). The + resulting environment is passed to the spawned ``cargo`` process, whose + stdout and stderr are streamed to the current process. + + Parameters + ---------- + args : list[str] + Arguments forwarded verbatim to ``cargo``. + env_overrides : Mapping[str, str] | None, optional + Extra or replacement environment variables. When ``None`` (the + default), the environment is inherited unchanged except for any + ``env_unsets`` removals. + env_unsets : Iterable[str], optional + Variable names to remove from the inherited environment before + ``env_overrides`` are applied. Missing keys are silently ignored. + Unsets are performed before overrides, so ``env_overrides`` can + unconditionally set a variable that may or may not have been + inherited. + + Returns + ------- + str + Captured stdout from the ``cargo`` invocation. + """ + typer.echo(f"$ cargo {shlex.join(args)}") + env = _build_cargo_env(env_overrides, env_unsets) + wait_timeout = float(os.getenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "600")) + proc = _spawn_cargo(cargo[args], env) + try: + _assert_cargo_streams(proc) + stdout_lines = _pump_cargo_output(proc, wait_timeout=wait_timeout) + retcode = _wait_for_cargo(proc, wait_timeout=wait_timeout) + if retcode != 0: + typer.echo( + f"cargo {shlex.join(args)} failed with code {retcode}", + err=True, + ) + raise typer.Exit(code=retcode or 1) + return "\n".join(stdout_lines) + finally: + _safe_close_text_stream(proc.stdout) + _safe_close_text_stream(proc.stderr) diff --git a/.github/actions/generate-coverage/scripts/_cranelift.py b/.github/actions/generate-coverage/scripts/_cranelift.py new file mode 100644 index 00000000..e7e31ec6 --- /dev/null +++ b/.github/actions/generate-coverage/scripts/_cranelift.py @@ -0,0 +1,97 @@ +from __future__ import annotations + +import re +import typing as typ + +if typ.TYPE_CHECKING: + from pathlib import Path + +_CARGO_COVERAGE_ENV_UNSETS = ( + "CARGO_PROFILE_DEV_CODEGEN_BACKEND", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND", +) + + +def _uses_cranelift_backend(manifest_path: Path) -> bool: + """Return ``True`` when the project configures the Cranelift codegen backend. + + Searches from the manifest directory upward for ``.cargo/config.toml`` + (or ``.cargo/config``) and checks whether any profile sets + ``codegen-backend = "cranelift"``. + """ + search_dir = manifest_path.resolve().parent + while True: + for name in ("config.toml", "config"): + candidate = search_dir / ".cargo" / name + if candidate.is_file(): + try: + content = candidate.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + if re.search( + r'^[ \t]*codegen-backend\s*=\s*["\']cranelift["\']', + content, + flags=re.MULTILINE, + ): + return True + parent = search_dir.parent + if parent == search_dir: + break + search_dir = parent + return False + + +def _is_profile_section(section: str) -> bool: + """Return ``True`` if *section* is a Cargo profile section name. + + Matches both the bare ``[profile]`` table and dotted sub-tables such as + ``[profile.dev]`` and ``[profile.release]``. + """ + return section == "profile" or section.startswith("profile.") + + +def _manifest_uses_cranelift_backend(manifest_path: Path) -> bool: + """Return ``True`` when ``manifest_path`` configures Cranelift in profiles. + + Parameters + ---------- + manifest_path : Path + Path to the ``Cargo.toml`` manifest to inspect. + + Returns + ------- + bool + ``True`` if any ``[profile.*]`` section sets + ``codegen-backend = "cranelift"``; ``False`` otherwise. + """ + try: + content = manifest_path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return False + in_profile_section = False + for line in content.splitlines(): + stripped = line.strip() + if not stripped or stripped.startswith(("#", "//")): + continue + section_match = re.match(r"^\s*\[(?P
[^\]]+)\]\s*(?:#.*)?$", line) + if section_match is not None: + in_profile_section = _is_profile_section(section_match["section"]) + continue + if in_profile_section and re.match( + r"""^codegen-backend\s*=\s*["']cranelift["']""", + stripped, + ): + return True + return False + + +def get_cargo_coverage_env(manifest_path: Path) -> dict[str, str]: + """Return coverage-specific cargo env overrides for Cranelift projects.""" + if not _uses_cranelift_backend( + manifest_path + ) and not _manifest_uses_cranelift_backend(manifest_path): + return {} + return { + "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", + } diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index 3088dc1d..09d77ad4 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -18,12 +18,14 @@ import sys import threading import time -import traceback import typing as typ from decimal import ROUND_HALF_UP, Decimal from pathlib import Path +import _cargo_runner import typer +from _cargo_runner import _run_cargo +from _cranelift import _CARGO_COVERAGE_ENV_UNSETS, get_cargo_coverage_env from cmd_utils_loader import run_cmd from coverage_parsers import get_line_coverage_percent_from_lcov from plumbum.cmd import cargo @@ -31,6 +33,7 @@ from shared_utils import read_previous_coverage logger = logging.getLogger(__name__) +_cargo_runner_run_cargo = _run_cargo try: # runtime import for graceful fallback from lxml import etree @@ -99,95 +102,50 @@ global-timeout = "10m" """ -_CARGO_COVERAGE_ENV_UNSETS = ( - "CARGO_PROFILE_DEV_CODEGEN_BACKEND", - "CARGO_PROFILE_TEST_CODEGEN_BACKEND", -) - - -def _uses_cranelift_backend(manifest_path: Path) -> bool: - """Return ``True`` when the project configures the Cranelift codegen backend. - - Searches from the manifest directory upward for ``.cargo/config.toml`` - (or ``.cargo/config``) and checks whether any profile sets - ``codegen-backend = "cranelift"``. - """ - search_dir = manifest_path.resolve().parent - while True: - for name in ("config.toml", "config"): - candidate = search_dir / ".cargo" / name - if candidate.is_file(): - try: - content = candidate.read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError): - continue - if re.search( - r'^[ \t]*codegen-backend\s*=\s*["\']cranelift["\']', - content, - flags=re.MULTILINE, - ): - return True - parent = search_dir.parent - if parent == search_dir: - break - search_dir = parent - return False - - -def _is_profile_section(section: str) -> bool: - """Return ``True`` if *section* is a Cargo profile section name. - - Matches both the bare ``[profile]`` table and dotted sub-tables such as - ``[profile.dev]`` and ``[profile.release]``. - """ - return section == "profile" or section.startswith("profile.") +def _run_cargo( + args: list[str], + *, + env_overrides: typ.Mapping[str, str] | None = None, + env_unsets: typ.Iterable[str] = (), +) -> str: + """Run ``cargo`` with ``args`` streaming output and return ``stdout``. -def _manifest_uses_cranelift_backend(manifest_path: Path) -> bool: - """Return ``True`` when ``manifest_path`` configures Cranelift in profiles. + Builds a subprocess environment by copying ``os.environ``, removing every + key in ``env_unsets``, and — when ``env_overrides`` is not ``None`` — + merging its entries into that copy (overrides take precedence). The + resulting environment is passed to the spawned ``cargo`` process, whose + stdout and stderr are streamed to the current process. Parameters ---------- - manifest_path : Path - Path to the ``Cargo.toml`` manifest to inspect. + args : list[str] + Arguments forwarded verbatim to ``cargo``. + env_overrides : Mapping[str, str] | None, optional + Extra or replacement environment variables. When ``None`` (the + default), the environment is inherited unchanged except for any + ``env_unsets`` removals. + env_unsets : Iterable[str], optional + Variable names to remove from the inherited environment before + ``env_overrides`` are applied. Missing keys are silently ignored. + Unsets are performed before overrides, so ``env_overrides`` can + unconditionally set a variable that may or may not have been + inherited. Returns ------- - bool - ``True`` if any ``[profile.*]`` section sets - ``codegen-backend = "cranelift"``; ``False`` otherwise. + str + Captured stdout from the ``cargo`` invocation. """ - try: - content = manifest_path.read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError): - return False - in_profile_section = False - for line in content.splitlines(): - stripped = line.strip() - if not stripped or stripped.startswith(("#", "//")): - continue - section_match = re.match(r"^\s*\[(?P
[^\]]+)\]\s*(?:#.*)?$", line) - if section_match is not None: - in_profile_section = _is_profile_section(section_match["section"]) - continue - if in_profile_section and re.match( - r"""^codegen-backend\s*=\s*["']cranelift["']""", - stripped, - ): - return True - return False - - -def get_cargo_coverage_env(manifest_path: Path) -> dict[str, str]: - """Return coverage-specific cargo env overrides for Cranelift projects.""" - if not _uses_cranelift_backend( - manifest_path - ) and not _manifest_uses_cranelift_backend(manifest_path): - return {} - return { - "CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", - "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm", - } + _cargo_runner.cargo = cargo + _cargo_runner.selectors = selectors + _cargo_runner.threading = threading + _cargo_runner.time = time + return _cargo_runner_run_cargo( + args, + env_overrides=env_overrides, + env_unsets=env_unsets, + ) def get_cargo_coverage_cmd( @@ -266,365 +224,6 @@ def get_line_coverage_percent_from_cobertura(xml_file: Path) -> str: return _format_percent(covered, total) -def _safe_close_text_stream(stream: typ.TextIO | None) -> None: - """Close ``stream`` while suppressing any cleanup errors.""" - if stream is None: - return - with contextlib.suppress(Exception): - stream.close() - - -def _is_debug_pump_enabled() -> bool: - """Return ``True`` if cargo pump-thread debug logging is requested.""" - return os.environ.get("RUN_RUST_DEBUG") == "1" or bool(os.environ.get("DEBUG_UTF8")) - - -def _pump_stream_thread( - src: typ.IO[str], - *, - to_stdout: bool, - stdout_lines: list[str], - thread_exceptions: list[Exception], -) -> None: - """Read lines from *src*, echo them, and capture stdout lines. - - Thread exceptions are appended to *thread_exceptions* rather than - propagated so that the monitoring loop can handle them. - """ - dest = sys.stdout if to_stdout else sys.stderr - try: - for line in iter(src.readline, ""): - dest.write(line) - dest.flush() - if to_stdout: - stdout_lines.append(line.rstrip("\r\n")) - except Exception as exc: # noqa: BLE001 - thread_exceptions.append(exc) - if _is_debug_pump_enabled(): - sys.stderr.write(f"Exception in pump thread: {exc}\n") - sys.stderr.write(traceback.format_exc()) - - -def _poll_pump_loop_iteration( - threads: list[threading.Thread], - proc: subprocess.Popen[str], - deadline: float, - wait_timeout: float, - thread_exceptions: list[Exception], -) -> bool: - """Perform one monitoring iteration; return ``True`` when pumping is done.""" - if thread_exceptions: - with contextlib.suppress(Exception): - proc.kill() - return True - if not any(t.is_alive() for t in threads): - return True - if proc.poll() is None: - remaining = deadline - time.monotonic() - if remaining <= 0: - _raise_cargo_timeout(proc, wait_timeout=wait_timeout) - join_timeout = min(0.1, remaining) - else: - join_timeout = 0.0 - for thread in threads: - thread.join(timeout=join_timeout) - return False - - -def _finalise_pump_threads( - threads: list[threading.Thread], - proc: subprocess.Popen[str], - thread_exceptions: list[Exception], -) -> None: - """Join threads, kill on timeout, and re-raise any captured thread exception.""" - timed_out = False - for thread in threads: - thread.join(timeout=5) - if thread.is_alive(): - timed_out = True - if timed_out: - with contextlib.suppress(Exception): - proc.kill() - thread_exceptions.append( - TimeoutError("cargo output pump threads did not terminate in time") - ) - if thread_exceptions: - with contextlib.suppress(Exception): - proc.wait(timeout=5) - raise thread_exceptions[0] - - -def _pump_cargo_output_windows( - proc: subprocess.Popen[str], - stdout_stream: typ.IO[str], - stderr_stream: typ.IO[str], - *, - wait_timeout: float, -) -> list[str]: - """Pump cargo output on Windows using background threads.""" - thread_exceptions: list[Exception] = [] - stdout_lines: list[str] = [] - deadline = time.monotonic() + wait_timeout - - threads = [ - threading.Thread( - name="cargo-stdout", - target=_pump_stream_thread, - args=(stdout_stream,), - kwargs={ - "to_stdout": True, - "stdout_lines": stdout_lines, - "thread_exceptions": thread_exceptions, - }, - ), - threading.Thread( - name="cargo-stderr", - target=_pump_stream_thread, - args=(stderr_stream,), - kwargs={ - "to_stdout": False, - "stdout_lines": stdout_lines, - "thread_exceptions": thread_exceptions, - }, - ), - ] - for thread in threads: - thread.start() - while not _poll_pump_loop_iteration( - threads, proc, deadline, wait_timeout, thread_exceptions - ): - pass - _finalise_pump_threads(threads, proc, thread_exceptions) - return stdout_lines - - -def _handle_cargo_output_event( - key: selectors.SelectorKey, - stdout_lines: list[str], - sel: selectors.DefaultSelector, -) -> None: - """Process one selector event from a cargo output stream. - - Unregisters the stream when EOF is reached; otherwise echoes the line - and, for stdout, appends it to *stdout_lines*. - """ - stream = typ.cast("typ.TextIO", key.fileobj) - line = stream.readline() - if not line: - sel.unregister(stream) - return - if key.data == "stdout": - typer.echo(line, nl=False) - stdout_lines.append(line.rstrip("\r\n")) - else: - typer.echo(line, err=True, nl=False) - - -def _kill_cargo_process(proc: subprocess.Popen[str]) -> None: - """Kill *proc* and wait for termination, suppressing any errors.""" - with contextlib.suppress(Exception): - proc.kill() - with contextlib.suppress(Exception): - proc.wait(timeout=5) - - -def _pump_cargo_output( - proc: subprocess.Popen[str], - *, - wait_timeout: float, -) -> list[str]: - """Pump ``proc`` output streams to console and collect stdout lines.""" - if proc.stdout is None or proc.stderr is None: # pragma: no cover - defensive - message = ( - "cargo output streams must be captured.\n" - f"proc.stdout: {proc.stdout}\n" - f"proc.stderr: {proc.stderr}\n" - f"proc.args: {getattr(proc, 'args', None)}" - ) - raise RuntimeError(message) - - stdout_stream = proc.stdout - stderr_stream = proc.stderr - stdout_lines: list[str] = [] - - if os.name == "nt": - return _pump_cargo_output_windows( - proc, - stdout_stream, - stderr_stream, - wait_timeout=wait_timeout, - ) - - deadline = time.monotonic() + wait_timeout - sel = selectors.DefaultSelector() - try: - sel.register(stdout_stream, selectors.EVENT_READ, data="stdout") - sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") - - while sel.get_map(): - if proc.poll() is None and time.monotonic() >= deadline: - _raise_cargo_timeout(proc, wait_timeout=wait_timeout) - - timeout = max(0.0, deadline - time.monotonic()) - for key, _ in sel.select(timeout): - _handle_cargo_output_event(key, stdout_lines, sel) - except Exception: - _kill_cargo_process(proc) - raise - finally: - sel.close() - - return stdout_lines - - -def _build_cargo_env( - env_overrides: typ.Mapping[str, str] | None, - env_unsets: typ.Iterable[str], -) -> dict[str, str]: - """Return the environment dict for a spawned cargo process. - - Starts from a copy of ``os.environ``, removes every key in - ``env_unsets``, then merges ``env_overrides`` (if provided). - """ - env = dict(os.environ) - for key in env_unsets: - env.pop(key, None) - if env_overrides is not None: - env.update(env_overrides) - return env - - -def _assert_cargo_streams(proc: subprocess.Popen[str]) -> None: - """Raise ``typer.Exit(1)`` if stdout or stderr were not captured. - - Kills and cleans up the process before raising so no resources leak. - """ - if proc.stdout is not None and proc.stderr is not None: - return - missing_streams = [] - if proc.stdout is None: - missing_streams.append("stdout") - if proc.stderr is None: - missing_streams.append("stderr") - missing = ", ".join(missing_streams) - message = f"cargo output streams not captured: missing {missing}" - with contextlib.suppress(Exception): - proc.kill() - with contextlib.suppress(Exception): - proc.wait(timeout=5) - _safe_close_text_stream(typ.cast("typ.TextIO | None", proc.stdout)) - _safe_close_text_stream(typ.cast("typ.TextIO | None", proc.stderr)) - typer.echo(f"::error::{message}", err=True) - raise typer.Exit(1) from None - - -def _raise_cargo_timeout( - proc: subprocess.Popen[str], *, wait_timeout: float -) -> typ.Never: - """Kill ``proc`` and raise ``typer.Exit(1)`` for a cargo timeout.""" - typer.echo( - f"::error::cargo did not exit within {wait_timeout}s; killing", - err=True, - ) - with contextlib.suppress(Exception): - proc.kill() - with contextlib.suppress(Exception): - proc.wait(timeout=5) - raise typer.Exit(1) from None - - -def _wait_for_cargo(proc: subprocess.Popen[str], *, wait_timeout: float) -> int: - """Wait for cargo to exit and return its return code. - - Kills the process and raises ``typer.Exit(1)`` if it does not exit - within ``RUN_RUST_CARGO_WAIT_TIMEOUT`` seconds (default 600). - """ - try: - return proc.wait(timeout=wait_timeout) - except subprocess.TimeoutExpired: - _raise_cargo_timeout(proc, wait_timeout=wait_timeout) - - -def _spawn_cargo( - command: typ.Any, # noqa: ANN401 - env: dict[str, str], -) -> subprocess.Popen[str]: - """Spawn a ``cargo`` subprocess with the given environment. - - Handles both direct ``popen`` invocation and plumbum machine-env - contexts transparently. - """ - popen_kwargs: dict[str, typ.Any] = { - "stdin": subprocess.DEVNULL, - "stdout": subprocess.PIPE, - "stderr": subprocess.PIPE, - "text": True, - "encoding": "utf-8", - "errors": "replace", - } - machine_env = getattr(getattr(cargo, "machine", None), "env", None) - if machine_env is None: - return command.popen(**popen_kwargs, env=env) - with machine_env(): - machine_env.clear() - machine_env.update(env) - return command.popen(**popen_kwargs) - - -def _run_cargo( - args: list[str], - *, - env_overrides: typ.Mapping[str, str] | None = None, - env_unsets: typ.Iterable[str] = (), -) -> str: - """Run ``cargo`` with ``args`` streaming output and return ``stdout``. - - Builds a subprocess environment by copying ``os.environ``, removing every - key in ``env_unsets``, and — when ``env_overrides`` is not ``None`` — - merging its entries into that copy (overrides take precedence). The - resulting environment is passed to the spawned ``cargo`` process, whose - stdout and stderr are streamed to the current process. - - Parameters - ---------- - args : list[str] - Arguments forwarded verbatim to ``cargo``. - env_overrides : Mapping[str, str] | None, optional - Extra or replacement environment variables. When ``None`` (the - default), the environment is inherited unchanged except for any - ``env_unsets`` removals. - env_unsets : Iterable[str], optional - Variable names to remove from the inherited environment before - ``env_overrides`` are applied. Missing keys are silently ignored. - Unsets are performed before overrides, so ``env_overrides`` can - unconditionally set a variable that may or may not have been - inherited. - - Returns - ------- - str - Captured stdout from the ``cargo`` invocation. - """ - typer.echo(f"$ cargo {shlex.join(args)}") - env = _build_cargo_env(env_overrides, env_unsets) - wait_timeout = float(os.getenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "600")) - proc = _spawn_cargo(cargo[args], env) - try: - _assert_cargo_streams(proc) - stdout_lines = _pump_cargo_output(proc, wait_timeout=wait_timeout) - retcode = _wait_for_cargo(proc, wait_timeout=wait_timeout) - if retcode != 0: - typer.echo( - f"cargo {shlex.join(args)} failed with code {retcode}", - err=True, - ) - raise typer.Exit(code=retcode or 1) - return "\n".join(stdout_lines) - finally: - _safe_close_text_stream(proc.stdout) - _safe_close_text_stream(proc.stderr) - - def _merge_lcov(base: Path, extra: Path) -> None: """Merge two lcov files ensuring they end with ``end_of_record``.""" try: diff --git a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py index be6ad369..17d9e15f 100644 --- a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py +++ b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py @@ -11,20 +11,22 @@ import pytest from syspath_hack import prepend_to_syspath -MODULE_PATH = Path(__file__).resolve().parent.parent / "run_rust.py" +MODULE_PATH = Path(__file__).resolve().parent.parent / "_cargo_runner.py" SCRIPT_DIR = MODULE_PATH.parent prepend_to_syspath(SCRIPT_DIR) -spec = importlib.util.spec_from_file_location("run_rust_module", MODULE_PATH) +spec = importlib.util.spec_from_file_location("cargo_runner_module", MODULE_PATH) if spec is None or spec.loader is None: # pragma: no cover - defensive import guard - load_error_message = "Unable to load run_rust module for testing" + load_error_message = "Unable to load _cargo_runner module for testing" raise RuntimeError(load_error_message) -run_rust_module = importlib.util.module_from_spec(spec) -if not isinstance(run_rust_module, ModuleType): # pragma: no cover - importlib contract +cargo_runner_module = importlib.util.module_from_spec(spec) +if not isinstance( + cargo_runner_module, ModuleType +): # pragma: no cover - importlib contract type_error_message = "module_from_spec did not return a ModuleType" raise TypeError(type_error_message) -spec.loader.exec_module(run_rust_module) -run_rust = run_rust_module +spec.loader.exec_module(cargo_runner_module) +run_rust = cargo_runner_module class _SupportsKillWait(typ.Protocol): From 553c2675e51485e8794320b5d8840604b186087c Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 16:28:23 +0200 Subject: [PATCH 19/28] Harden cargo timeout parsing and enforcement --- .../scripts/_cargo_runner.py | 22 +++++++++++-------- .../generate-coverage/scripts/run_rust.py | 1 + .../generate-coverage/tests/test_scripts.py | 12 ++++++++-- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/_cargo_runner.py b/.github/actions/generate-coverage/scripts/_cargo_runner.py index 1ca304c7..d3099e82 100644 --- a/.github/actions/generate-coverage/scripts/_cargo_runner.py +++ b/.github/actions/generate-coverage/scripts/_cargo_runner.py @@ -68,13 +68,10 @@ def _poll_pump_loop_iteration( return True if not any(t.is_alive() for t in threads): return True - if proc.poll() is None: - remaining = deadline - time.monotonic() - if remaining <= 0: - _raise_cargo_timeout(proc, wait_timeout=wait_timeout) - join_timeout = min(0.1, remaining) - else: - join_timeout = 0.0 + remaining = deadline - time.monotonic() + if remaining <= 0: + _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + join_timeout = min(0.1, remaining) for thread in threads: thread.join(timeout=join_timeout) return False @@ -211,7 +208,7 @@ def _pump_cargo_output( sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") while sel.get_map(): - if proc.poll() is None and time.monotonic() >= deadline: + if time.monotonic() >= deadline: _raise_cargo_timeout(proc, wait_timeout=wait_timeout) timeout = max(0.0, deadline - time.monotonic()) @@ -356,7 +353,14 @@ def _run_cargo( """ typer.echo(f"$ cargo {shlex.join(args)}") env = _build_cargo_env(env_overrides, env_unsets) - wait_timeout = float(os.getenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "600")) + try: + wait_timeout = float(os.getenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "600")) + except ValueError as exc: + typer.echo( + "::error::RUN_RUST_CARGO_WAIT_TIMEOUT must be a number", + err=True, + ) + raise typer.Exit(1) from exc proc = _spawn_cargo(cargo[args], env) try: _assert_cargo_streams(proc) diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index 09d77ad4..ac878aee 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -141,6 +141,7 @@ def _run_cargo( _cargo_runner.selectors = selectors _cargo_runner.threading = threading _cargo_runner.time = time + typ.cast("typ.Any", _cargo_runner).typer = typer return _cargo_runner_run_cargo( args, env_overrides=env_overrides, diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index d2097e4d..d9c93bed 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -802,17 +802,25 @@ def test_run_cargo_invalid_timeout_does_not_spawn( ) -> None: """Invalid wait-timeout values fail before cargo is spawned.""" mod = _load_module(monkeypatch, "run_rust") + messages: list[tuple[str, bool]] = [] + + def fake_echo(message: str, *, err: bool = False, nl: bool = True) -> None: + _ = nl + messages.append((message, err)) + monkeypatch.setattr(mod.os, "name", "nt") - monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) + monkeypatch.setattr(mod.typer, "echo", fake_echo) monkeypatch.setenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "not-a-float") fake_cargo = _make_fake_cargo("out-line\n", "err-line\n") monkeypatch.setattr(mod, "cargo", fake_cargo) - with pytest.raises(ValueError, match="could not convert string to float"): + with pytest.raises(mod.typer.Exit) as excinfo: mod._run_cargo(["llvm-cov"]) + assert _exit_code(excinfo.value) == 1 assert fake_cargo.last_proc is None + assert ("::error::RUN_RUST_CARGO_WAIT_TIMEOUT must be a number", True) in messages def _make_cucumber_spy( From 4aed49e90917b27e2fbd7e1ef440cbb40aac9bc5 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 19:25:41 +0200 Subject: [PATCH 20/28] Share cargo timeout deadline across phases --- .../scripts/_cargo_runner.py | 30 +++++++++++++------ .../scripts/tests/test_run_rust_windows.py | 2 ++ .../generate-coverage/tests/test_scripts.py | 21 +++++++------ 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/_cargo_runner.py b/.github/actions/generate-coverage/scripts/_cargo_runner.py index d3099e82..21a3bde9 100644 --- a/.github/actions/generate-coverage/scripts/_cargo_runner.py +++ b/.github/actions/generate-coverage/scripts/_cargo_runner.py @@ -68,7 +68,7 @@ def _poll_pump_loop_iteration( return True if not any(t.is_alive() for t in threads): return True - remaining = deadline - time.monotonic() + remaining = max(0.0, deadline - time.time()) if remaining <= 0: _raise_cargo_timeout(proc, wait_timeout=wait_timeout) join_timeout = min(0.1, remaining) @@ -105,12 +105,12 @@ def _pump_cargo_output_windows( stdout_stream: typ.IO[str], stderr_stream: typ.IO[str], *, + deadline: float, wait_timeout: float, ) -> list[str]: """Pump cargo output on Windows using background threads.""" thread_exceptions: list[Exception] = [] stdout_lines: list[str] = [] - deadline = time.monotonic() + wait_timeout threads = [ threading.Thread( @@ -177,6 +177,7 @@ def _kill_cargo_process(proc: subprocess.Popen[str]) -> None: def _pump_cargo_output( proc: subprocess.Popen[str], *, + deadline: float, wait_timeout: float, ) -> list[str]: """Pump ``proc`` output streams to console and collect stdout lines.""" @@ -198,20 +199,20 @@ def _pump_cargo_output( proc, stdout_stream, stderr_stream, + deadline=deadline, wait_timeout=wait_timeout, ) - deadline = time.monotonic() + wait_timeout sel = selectors.DefaultSelector() try: sel.register(stdout_stream, selectors.EVENT_READ, data="stdout") sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") while sel.get_map(): - if time.monotonic() >= deadline: + if time.time() >= deadline: _raise_cargo_timeout(proc, wait_timeout=wait_timeout) - timeout = max(0.0, deadline - time.monotonic()) + timeout = max(0.0, deadline - time.time()) for key, _ in sel.select(timeout): _handle_cargo_output_event(key, stdout_lines, sel) except Exception: @@ -279,14 +280,16 @@ def _raise_cargo_timeout( raise typer.Exit(1) from None -def _wait_for_cargo(proc: subprocess.Popen[str], *, wait_timeout: float) -> int: +def _wait_for_cargo( + proc: subprocess.Popen[str], *, deadline: float, wait_timeout: float +) -> int: """Wait for cargo to exit and return its return code. Kills the process and raises ``typer.Exit(1)`` if it does not exit within ``RUN_RUST_CARGO_WAIT_TIMEOUT`` seconds (default 600). """ try: - return proc.wait(timeout=wait_timeout) + return proc.wait(timeout=max(0.0, deadline - time.time())) except subprocess.TimeoutExpired: _raise_cargo_timeout(proc, wait_timeout=wait_timeout) @@ -361,11 +364,20 @@ def _run_cargo( err=True, ) raise typer.Exit(1) from exc + deadline = time.time() + wait_timeout proc = _spawn_cargo(cargo[args], env) try: _assert_cargo_streams(proc) - stdout_lines = _pump_cargo_output(proc, wait_timeout=wait_timeout) - retcode = _wait_for_cargo(proc, wait_timeout=wait_timeout) + stdout_lines = _pump_cargo_output( + proc, + deadline=deadline, + wait_timeout=wait_timeout, + ) + retcode = _wait_for_cargo( + proc, + deadline=deadline, + wait_timeout=wait_timeout, + ) if retcode != 0: typer.echo( f"cargo {shlex.join(args)} failed with code {retcode}", diff --git a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py index 17d9e15f..9799920d 100644 --- a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py +++ b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py @@ -48,6 +48,7 @@ def _pump_cargo_output_windows( stdout_stream: typ.IO[str], stderr_stream: typ.IO[str], *, + deadline: float, wait_timeout: float, ) -> list[str]: """Mirror of the helper under test.""" @@ -103,6 +104,7 @@ def test_pump_cargo_output_windows_streams( dummy_proc, stdout_stream, stderr_stream, + deadline=1.0, wait_timeout=1.0, ) diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index d9c93bed..22977b6b 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -177,7 +177,7 @@ def _run_rust_coverage_test( shell_stubs: StubManager, config: RustCoverageConfig, *, - monkeypatch: pytest.MonkeyPatch | None = None, + monkeypatch: pytest.MonkeyPatch, ) -> tuple[list[str], Path, Path]: """Run ``run_rust.py`` with shared setup and return cargo argv + paths.""" out = tmp_path / "cov.lcov" @@ -203,8 +203,7 @@ def _run_rust_coverage_test( "GITHUB_OUTPUT": str(gh), } - if monkeypatch is not None: - monkeypatch.chdir(tmp_path) + monkeypatch.chdir(tmp_path) script = Path(__file__).resolve().parents[1] / "scripts" / "run_rust.py" returncode, stdout, _ = run_script(script, env) @@ -222,7 +221,7 @@ def _run_rust_coverage_call( shell_stubs: StubManager, config: RustCoverageConfig, *, - monkeypatch: pytest.MonkeyPatch | None = None, + monkeypatch: pytest.MonkeyPatch, ) -> tuple[Call, Path, Path]: """Run ``run_rust.py`` and return the recorded cargo call + output paths.""" _cargo_args, out, gh = _run_rust_coverage_test( @@ -251,6 +250,7 @@ def test_run_rust_success( features="fast", with_default_features=False, ), + monkeypatch=monkeypatch, ) cargo_args = cargo_call.argv expected_args = [ @@ -303,13 +303,16 @@ def test_run_rust_nextest_command( def test_run_rust_uses_detected_manifest_path( - tmp_path: Path, shell_stubs: StubManager + tmp_path: Path, + shell_stubs: StubManager, + monkeypatch: pytest.MonkeyPatch, ) -> None: """Detected manifest path is propagated to cargo llvm-cov.""" cargo_args, _out, _gh = _run_rust_coverage_test( tmp_path, shell_stubs, RustCoverageConfig(use_nextest=False, manifest_path="rust-toy-app/Cargo.toml"), + monkeypatch=monkeypatch, ) assert "llvm-cov" in cargo_args mp_idx = cargo_args.index("--manifest-path") @@ -785,8 +788,8 @@ def __getitem__(self, _args: list[str]) -> FakeRunner: monkeypatch.setattr(mod, "cargo", FakeCargoCommand()) monkeypatch.setattr(mod.selectors, "DefaultSelector", FakeSelector) - monotonic_values = iter([0.0, 0.0, 0.5, 1.0]) - monkeypatch.setattr(mod.time, "monotonic", lambda: next(monotonic_values)) + time_values = iter([0.0, 0.0, 0.5, 1.0]) + monkeypatch.setattr(mod.time, "time", lambda: next(time_values)) with pytest.raises(mod.typer.Exit) as excinfo: mod._run_cargo(["llvm-cov"]) @@ -972,8 +975,8 @@ def join(self, timeout: float | None = None) -> None: type(self).join_calls += 1 monkeypatch.setattr(mod.threading, "Thread", FakeThread) - monotonic_values = iter([0.0, 0.0, 1.0]) - monkeypatch.setattr(mod.time, "monotonic", lambda: next(monotonic_values)) + time_values = iter([0.0, 0.0, 1.0]) + monkeypatch.setattr(mod.time, "time", lambda: next(time_values)) class FakeProc: def __init__(self) -> None: From 112fbc766052238a564d7437097f95f60e4085ec Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 19:42:25 +0200 Subject: [PATCH 21/28] Introduce cargo process context for pump helpers --- .../scripts/_cargo_runner.py | 48 +++++++++---------- .../scripts/tests/test_run_rust_windows.py | 17 +++---- 2 files changed, 30 insertions(+), 35 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/_cargo_runner.py b/.github/actions/generate-coverage/scripts/_cargo_runner.py index 21a3bde9..016d0c2d 100644 --- a/.github/actions/generate-coverage/scripts/_cargo_runner.py +++ b/.github/actions/generate-coverage/scripts/_cargo_runner.py @@ -1,6 +1,7 @@ from __future__ import annotations import contextlib +import dataclasses import os import selectors import shlex @@ -15,6 +16,15 @@ from plumbum.cmd import cargo +@dataclasses.dataclass +class _CargoProcCtx: + """Cargo process handle together with its timing constraints.""" + + proc: subprocess.Popen[str] + deadline: float + wait_timeout: float + + def _safe_close_text_stream(stream: typ.TextIO | None) -> None: """Close ``stream`` while suppressing any cleanup errors.""" if stream is None: @@ -56,21 +66,19 @@ def _pump_stream_thread( def _poll_pump_loop_iteration( threads: list[threading.Thread], - proc: subprocess.Popen[str], - deadline: float, - wait_timeout: float, + ctx: _CargoProcCtx, thread_exceptions: list[Exception], ) -> bool: """Perform one monitoring iteration; return ``True`` when pumping is done.""" if thread_exceptions: with contextlib.suppress(Exception): - proc.kill() + ctx.proc.kill() return True if not any(t.is_alive() for t in threads): return True - remaining = max(0.0, deadline - time.time()) + remaining = max(0.0, ctx.deadline - time.time()) if remaining <= 0: - _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + _raise_cargo_timeout(ctx.proc, wait_timeout=ctx.wait_timeout) join_timeout = min(0.1, remaining) for thread in threads: thread.join(timeout=join_timeout) @@ -101,12 +109,9 @@ def _finalise_pump_threads( def _pump_cargo_output_windows( - proc: subprocess.Popen[str], stdout_stream: typ.IO[str], stderr_stream: typ.IO[str], - *, - deadline: float, - wait_timeout: float, + ctx: _CargoProcCtx, ) -> list[str]: """Pump cargo output on Windows using background threads.""" thread_exceptions: list[Exception] = [] @@ -136,11 +141,9 @@ def _pump_cargo_output_windows( ] for thread in threads: thread.start() - while not _poll_pump_loop_iteration( - threads, proc, deadline, wait_timeout, thread_exceptions - ): + while not _poll_pump_loop_iteration(threads, ctx, thread_exceptions): pass - _finalise_pump_threads(threads, proc, thread_exceptions) + _finalise_pump_threads(threads, ctx.proc, thread_exceptions) return stdout_lines @@ -193,15 +196,10 @@ def _pump_cargo_output( stdout_stream = proc.stdout stderr_stream = proc.stderr stdout_lines: list[str] = [] + ctx = _CargoProcCtx(proc=proc, deadline=deadline, wait_timeout=wait_timeout) if os.name == "nt": - return _pump_cargo_output_windows( - proc, - stdout_stream, - stderr_stream, - deadline=deadline, - wait_timeout=wait_timeout, - ) + return _pump_cargo_output_windows(stdout_stream, stderr_stream, ctx) sel = selectors.DefaultSelector() try: @@ -209,14 +207,14 @@ def _pump_cargo_output( sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") while sel.get_map(): - if time.time() >= deadline: - _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + if time.time() >= ctx.deadline: + _raise_cargo_timeout(ctx.proc, wait_timeout=ctx.wait_timeout) - timeout = max(0.0, deadline - time.time()) + timeout = max(0.0, ctx.deadline - time.time()) for key, _ in sel.select(timeout): _handle_cargo_output_event(key, stdout_lines, sel) except Exception: - _kill_cargo_process(proc) + _kill_cargo_process(ctx.proc) raise finally: sel.close() diff --git a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py index 9799920d..60234aea 100644 --- a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py +++ b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py @@ -4,6 +4,8 @@ import importlib.util import io +import sys +import time import typing as typ from pathlib import Path from types import ModuleType @@ -25,6 +27,7 @@ ): # pragma: no cover - importlib contract type_error_message = "module_from_spec did not return a ModuleType" raise TypeError(type_error_message) +sys.modules[spec.name] = cargo_runner_module spec.loader.exec_module(cargo_runner_module) run_rust = cargo_runner_module @@ -44,12 +47,9 @@ class _RunRustModule(typ.Protocol): def _pump_cargo_output_windows( self, - proc: _SupportsKillWait, stdout_stream: typ.IO[str], stderr_stream: typ.IO[str], - *, - deadline: float, - wait_timeout: float, + ctx: object, ) -> list[str]: """Mirror of the helper under test.""" @@ -100,13 +100,10 @@ def test_pump_cargo_output_windows_streams( stdout_stream = io.StringIO(stdout_payload) stderr_stream = io.StringIO(stderr_payload) - lines = run_rust_typed._pump_cargo_output_windows( - dummy_proc, - stdout_stream, - stderr_stream, - deadline=1.0, - wait_timeout=1.0, + ctx = run_rust._CargoProcCtx( + proc=dummy_proc, deadline=time.time() + 1.0, wait_timeout=1.0 ) + lines = run_rust_typed._pump_cargo_output_windows(stdout_stream, stderr_stream, ctx) assert lines == expected assert captured_stdout.getvalue() == stdout_payload From 2996a802b29cc1048323365802a83e8d661de50d Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 19:44:25 +0200 Subject: [PATCH 22/28] Extract POSIX cargo pump dispatcher path --- .../scripts/_cargo_runner.py | 83 ++++++++++++++----- 1 file changed, 60 insertions(+), 23 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/_cargo_runner.py b/.github/actions/generate-coverage/scripts/_cargo_runner.py index 016d0c2d..4544204f 100644 --- a/.github/actions/generate-coverage/scripts/_cargo_runner.py +++ b/.github/actions/generate-coverage/scripts/_cargo_runner.py @@ -177,13 +177,64 @@ def _kill_cargo_process(proc: subprocess.Popen[str]) -> None: proc.wait(timeout=5) +def _pump_cargo_output_posix( + proc: subprocess.Popen[str], + stdout_stream: typ.IO[str], + stderr_stream: typ.IO[str], + *, + deadline: float, + wait_timeout: float, +) -> list[str]: + """Pump cargo output on POSIX using a selector-based event loop. + + Parameters + ---------- + proc: + The running cargo process. + stdout_stream: + The captured stdout text stream from *proc*. + stderr_stream: + The captured stderr text stream from *proc*. + deadline: + Absolute ``time.time()`` value after which the pump is aborted. + wait_timeout: + Original timeout in seconds, used only in the error message. + + Returns + ------- + list[str] + Lines collected from stdout (newlines stripped). + """ + stdout_lines: list[str] = [] + sel = selectors.DefaultSelector() + try: + sel.register(stdout_stream, selectors.EVENT_READ, data="stdout") + sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") + while sel.get_map(): + if time.time() >= deadline: + _raise_cargo_timeout(proc, wait_timeout=wait_timeout) + timeout = max(0.0, deadline - time.time()) + for key, _ in sel.select(timeout): + _handle_cargo_output_event(key, stdout_lines, sel) + except Exception: + _kill_cargo_process(proc) + raise + finally: + sel.close() + return stdout_lines + + def _pump_cargo_output( proc: subprocess.Popen[str], *, deadline: float, wait_timeout: float, ) -> list[str]: - """Pump ``proc`` output streams to console and collect stdout lines.""" + """Pump ``proc`` output streams to console and collect stdout lines. + + Delegates to ``_pump_cargo_output_windows`` on Windows and + ``_pump_cargo_output_posix`` elsewhere. + """ if proc.stdout is None or proc.stderr is None: # pragma: no cover - defensive message = ( "cargo output streams must be captured.\n" @@ -195,31 +246,17 @@ def _pump_cargo_output( stdout_stream = proc.stdout stderr_stream = proc.stderr - stdout_lines: list[str] = [] - ctx = _CargoProcCtx(proc=proc, deadline=deadline, wait_timeout=wait_timeout) if os.name == "nt": + ctx = _CargoProcCtx(proc=proc, deadline=deadline, wait_timeout=wait_timeout) return _pump_cargo_output_windows(stdout_stream, stderr_stream, ctx) - - sel = selectors.DefaultSelector() - try: - sel.register(stdout_stream, selectors.EVENT_READ, data="stdout") - sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") - - while sel.get_map(): - if time.time() >= ctx.deadline: - _raise_cargo_timeout(ctx.proc, wait_timeout=ctx.wait_timeout) - - timeout = max(0.0, ctx.deadline - time.time()) - for key, _ in sel.select(timeout): - _handle_cargo_output_event(key, stdout_lines, sel) - except Exception: - _kill_cargo_process(ctx.proc) - raise - finally: - sel.close() - - return stdout_lines + return _pump_cargo_output_posix( + proc, + stdout_stream, + stderr_stream, + deadline=deadline, + wait_timeout=wait_timeout, + ) def _build_cargo_env( From 057856c1cdb195b11d859c3b5770773868747733 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 19:46:12 +0200 Subject: [PATCH 23/28] Extract cargo config cranelift probe --- .../generate-coverage/scripts/_cranelift.py | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/_cranelift.py b/.github/actions/generate-coverage/scripts/_cranelift.py index e7e31ec6..ba392367 100644 --- a/.github/actions/generate-coverage/scripts/_cranelift.py +++ b/.github/actions/generate-coverage/scripts/_cranelift.py @@ -12,6 +12,24 @@ ) +def _cargo_config_contains_cranelift(candidate: Path) -> bool: + """Return ``True`` if *candidate* is a cargo config file that sets Cranelift. + + Returns ``False`` on any read or decode error. + """ + try: + content = candidate.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return False + return bool( + re.search( + r'^[ \t]*codegen-backend\s*=\s*["\']cranelift["\']', + content, + flags=re.MULTILINE, + ) + ) + + def _uses_cranelift_backend(manifest_path: Path) -> bool: """Return ``True`` when the project configures the Cranelift codegen backend. @@ -23,17 +41,8 @@ def _uses_cranelift_backend(manifest_path: Path) -> bool: while True: for name in ("config.toml", "config"): candidate = search_dir / ".cargo" / name - if candidate.is_file(): - try: - content = candidate.read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError): - continue - if re.search( - r'^[ \t]*codegen-backend\s*=\s*["\']cranelift["\']', - content, - flags=re.MULTILINE, - ): - return True + if candidate.is_file() and _cargo_config_contains_cranelift(candidate): + return True parent = search_dir.parent if parent == search_dir: break From 2e6b0e9c11af40701219f6ce30abc7582582cd23 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 19:49:01 +0200 Subject: [PATCH 24/28] Use monotonic cargo timeout deadlines --- .../generate-coverage/scripts/_cargo_runner.py | 12 ++++++------ .../scripts/tests/test_run_rust_windows.py | 2 +- .../actions/generate-coverage/tests/test_scripts.py | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/_cargo_runner.py b/.github/actions/generate-coverage/scripts/_cargo_runner.py index 4544204f..5d6ce9f0 100644 --- a/.github/actions/generate-coverage/scripts/_cargo_runner.py +++ b/.github/actions/generate-coverage/scripts/_cargo_runner.py @@ -76,7 +76,7 @@ def _poll_pump_loop_iteration( return True if not any(t.is_alive() for t in threads): return True - remaining = max(0.0, ctx.deadline - time.time()) + remaining = max(0.0, ctx.deadline - time.monotonic()) if remaining <= 0: _raise_cargo_timeout(ctx.proc, wait_timeout=ctx.wait_timeout) join_timeout = min(0.1, remaining) @@ -196,7 +196,7 @@ def _pump_cargo_output_posix( stderr_stream: The captured stderr text stream from *proc*. deadline: - Absolute ``time.time()`` value after which the pump is aborted. + Absolute ``time.monotonic()`` value after which the pump is aborted. wait_timeout: Original timeout in seconds, used only in the error message. @@ -211,9 +211,9 @@ def _pump_cargo_output_posix( sel.register(stdout_stream, selectors.EVENT_READ, data="stdout") sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") while sel.get_map(): - if time.time() >= deadline: + if time.monotonic() >= deadline: _raise_cargo_timeout(proc, wait_timeout=wait_timeout) - timeout = max(0.0, deadline - time.time()) + timeout = max(0.0, deadline - time.monotonic()) for key, _ in sel.select(timeout): _handle_cargo_output_event(key, stdout_lines, sel) except Exception: @@ -324,7 +324,7 @@ def _wait_for_cargo( within ``RUN_RUST_CARGO_WAIT_TIMEOUT`` seconds (default 600). """ try: - return proc.wait(timeout=max(0.0, deadline - time.time())) + return proc.wait(timeout=max(0.0, deadline - time.monotonic())) except subprocess.TimeoutExpired: _raise_cargo_timeout(proc, wait_timeout=wait_timeout) @@ -399,7 +399,7 @@ def _run_cargo( err=True, ) raise typer.Exit(1) from exc - deadline = time.time() + wait_timeout + deadline = time.monotonic() + wait_timeout proc = _spawn_cargo(cargo[args], env) try: _assert_cargo_streams(proc) diff --git a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py index 60234aea..3bd77c46 100644 --- a/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py +++ b/.github/actions/generate-coverage/scripts/tests/test_run_rust_windows.py @@ -101,7 +101,7 @@ def test_pump_cargo_output_windows_streams( stderr_stream = io.StringIO(stderr_payload) ctx = run_rust._CargoProcCtx( - proc=dummy_proc, deadline=time.time() + 1.0, wait_timeout=1.0 + proc=dummy_proc, deadline=time.monotonic() + 1.0, wait_timeout=1.0 ) lines = run_rust_typed._pump_cargo_output_windows(stdout_stream, stderr_stream, ctx) diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index 22977b6b..aea6a474 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -789,7 +789,7 @@ def __getitem__(self, _args: list[str]) -> FakeRunner: monkeypatch.setattr(mod, "cargo", FakeCargoCommand()) monkeypatch.setattr(mod.selectors, "DefaultSelector", FakeSelector) time_values = iter([0.0, 0.0, 0.5, 1.0]) - monkeypatch.setattr(mod.time, "time", lambda: next(time_values)) + monkeypatch.setattr(mod.time, "monotonic", lambda: next(time_values)) with pytest.raises(mod.typer.Exit) as excinfo: mod._run_cargo(["llvm-cov"]) @@ -976,7 +976,7 @@ def join(self, timeout: float | None = None) -> None: monkeypatch.setattr(mod.threading, "Thread", FakeThread) time_values = iter([0.0, 0.0, 1.0]) - monkeypatch.setattr(mod.time, "time", lambda: next(time_values)) + monkeypatch.setattr(mod.time, "monotonic", lambda: next(time_values)) class FakeProc: def __init__(self) -> None: From f811709d4262536d1817eb22a3d04971fa5c4357 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 19:53:22 +0200 Subject: [PATCH 25/28] Add generate-coverage docstrings --- .../scripts/_cargo_runner.py | 20 ++++++ .../generate-coverage/scripts/_cranelift.py | 51 +++++++++++++- .../generate-coverage/scripts/run_rust.py | 1 + .../generate-coverage/tests/test_scripts.py | 67 +++++++++++++++++-- 4 files changed, 133 insertions(+), 6 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/_cargo_runner.py b/.github/actions/generate-coverage/scripts/_cargo_runner.py index 5d6ce9f0..875bb76d 100644 --- a/.github/actions/generate-coverage/scripts/_cargo_runner.py +++ b/.github/actions/generate-coverage/scripts/_cargo_runner.py @@ -1,3 +1,23 @@ +"""Cargo subprocess plumbing for the generate-coverage action. + +This module encapsulates spawning ``cargo`` subprocesses with live console +streaming and captured stdout. It provides: + +- Environment construction (``_build_cargo_env``) — copies ``os.environ``, + applies ``env_unsets``, then merges ``env_overrides``. +- Output pumping — selector-based on POSIX (``_pump_cargo_output``) and + thread-based on Windows (``_pump_cargo_output_windows``), both bounded by + ``RUN_RUST_CARGO_WAIT_TIMEOUT``. +- Process lifecycle helpers — stream assertion, timeout enforcement, and + cleanup (``_assert_cargo_streams``, ``_raise_cargo_timeout``, + ``_wait_for_cargo``, ``_kill_cargo_process``). +- The primary entry point ``_run_cargo``, which orchestrates all of the + above and returns captured stdout as a string. + +All public symbols in this module are prefixed with ``_`` and are intended +for use only by ``run_rust.py``. +""" + from __future__ import annotations import contextlib diff --git a/.github/actions/generate-coverage/scripts/_cranelift.py b/.github/actions/generate-coverage/scripts/_cranelift.py index ba392367..252febce 100644 --- a/.github/actions/generate-coverage/scripts/_cranelift.py +++ b/.github/actions/generate-coverage/scripts/_cranelift.py @@ -1,3 +1,30 @@ +"""Cranelift codegen-backend detection for the generate-coverage action. + +This module provides lightweight text-based detection of the Cranelift +codegen backend in a Cargo project and computes the environment variable +overrides required to force LLVM during coverage runs. + +Exported symbols: + +- ``_CARGO_COVERAGE_ENV_UNSETS`` — tuple of env-var names to strip from the + inherited environment before applying coverage overrides. +- ``get_cargo_coverage_env(manifest_path)`` — returns a dict of + ``CARGO_PROFILE_*_CODEGEN_BACKEND=llvm`` overrides when Cranelift is + detected; returns an empty dict otherwise. + +Detection strategy (in priority order): + +1. Search upward from the manifest directory for ``.cargo/config.toml`` or + ``.cargo/config`` and scan for ``codegen-backend = "cranelift"``. +2. Parse ``[profile.*]`` sections in the given ``Cargo.toml`` for the same + key. + +Known limitation: when ``manifest_path`` points to a workspace member, +profile settings in the workspace-root ``Cargo.toml`` are not scanned. +Use ``.cargo/config.toml`` at the workspace root to ensure detection in +that case. +""" + from __future__ import annotations import re @@ -95,7 +122,29 @@ def _manifest_uses_cranelift_backend(manifest_path: Path) -> bool: def get_cargo_coverage_env(manifest_path: Path) -> dict[str, str]: - """Return coverage-specific cargo env overrides for Cranelift projects.""" + """Return coverage-specific cargo env overrides for Cranelift projects. + + Detects whether the project identified by *manifest_path* uses the + Cranelift codegen backend (via ``.cargo/config*`` or ``Cargo.toml`` + profile sections). When Cranelift is detected, returns a dict that + forces LLVM for coverage-instrumented builds. Returns an empty dict + for non-Cranelift projects. + + Parameters + ---------- + manifest_path : Path + Path to the ``Cargo.toml`` manifest for the crate or workspace + root being instrumented. When this points to a workspace member, + only that member's manifest is scanned; the workspace-root profile + is not inspected via this path. + + Returns + ------- + dict[str, str] + ``{"CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm", + "CARGO_PROFILE_TEST_CODEGEN_BACKEND": "llvm"}`` when Cranelift is + detected; ``{}`` otherwise. + """ if not _uses_cranelift_backend( manifest_path ) and not _manifest_uses_cranelift_backend(manifest_path): diff --git a/.github/actions/generate-coverage/scripts/run_rust.py b/.github/actions/generate-coverage/scripts/run_rust.py index ac878aee..72336570 100644 --- a/.github/actions/generate-coverage/scripts/run_rust.py +++ b/.github/actions/generate-coverage/scripts/run_rust.py @@ -185,6 +185,7 @@ def extract_percent(output: str) -> str: def _format_percent(covered: int, total: int) -> str: + """Format coverage counts as a two-decimal-place percentage string.""" pct = Decimal(covered) * Decimal(100) / Decimal(total) return str(pct.quantize(Decimal("0.01"), rounding=ROUND_HALF_UP)) diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index aea6a474..b7e412f1 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -96,7 +96,10 @@ def _make_fake_cargo( """Return a fake ``cargo`` object yielding the given streams.""" class FakeProc: + """Minimal subprocess test double for cargo invocations.""" + def __init__(self) -> None: + """Initialise the stub with the configured streams.""" self.stdout = ( stdout if hasattr(stdout, "readline") @@ -115,32 +118,42 @@ def __init__(self) -> None: self.waited = False def poll(self) -> None: - return None + """Report that the process is still running.""" def kill(self) -> None: + """Record that the process was killed when lifecycle tracking is enabled.""" if track_lifecycle: self.killed = True def wait(self, timeout: float | None = None) -> int: + """Return the configured return code immediately.""" if track_lifecycle: self.waited = True return returncode class FakeCargo: + """Minimal cargo command stub with plumbum-like behaviour.""" + def __init__(self) -> None: + """Initialise call-tracking state for the fake cargo command.""" self.last_proc: FakeProc | None = None self.last_popen_kwargs: dict[str, object] | None = None self.bound_env: dict[str, str] | None = None def with_env(self, **env: str) -> FakeCargo: + """Bind environment overrides and return the fake command.""" self.bound_env = dict(env) return self def __getitem__(self, _args: list[str]) -> object: + """Return a runner object for the given cargo arguments.""" cargo = self class Runner: + """Minimal runner object exposing ``popen``.""" + def popen(self, **_kw: object) -> FakeProc: + """Spawn and return the configured fake process.""" kwargs = dict(_kw) if cargo.bound_env is not None: kwargs["env"] = dict(cargo.bound_env) @@ -678,11 +691,15 @@ def test_run_cargo_windows_closes_streams( monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) class TrackingStream(io.StringIO): + """String stream that counts close calls.""" + def __init__(self, value: str) -> None: + """Initialise the stream with the provided text.""" super().__init__(value) self.close_calls = 0 def close(self) -> None: + """Close the stream and record the close call.""" self.close_calls += 1 super().close() @@ -742,36 +759,47 @@ def fake_echo(message: str, *, err: bool = False, nl: bool = True) -> None: monkeypatch.setenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "1") class FakeSelector: + """Selector stub that never yields readable events.""" + def __init__(self) -> None: + """Initialise the selector with a non-empty registration map.""" self._map = {"stdout": object()} def register(self, fileobj: object, event: object, data: str) -> None: + """Accept a selector registration without storing it.""" _ = (fileobj, event, data) def get_map(self) -> dict[str, object]: + """Return the current selector registration map.""" return self._map def select(self, timeout: float | None = None) -> list[tuple[object, object]]: + """Return no ready events for the given timeout.""" _ = timeout return [] def close(self) -> None: - return None + """Close the selector stub.""" class FakeProc: + """Minimal subprocess test double for timeout handling.""" + def __init__(self) -> None: + """Initialise the stub with empty captured streams.""" self.stdout = io.StringIO("") self.stderr = io.StringIO("") self.killed = False self.waited = False def poll(self) -> None: - return None + """Report that the process is still running.""" def kill(self) -> None: + """Record that the process was killed.""" self.killed = True def wait(self, timeout: float | None = None) -> int: + """Record the wait and return success.""" _ = timeout self.waited = True return 0 @@ -779,11 +807,17 @@ def wait(self, timeout: float | None = None) -> int: fake_proc = FakeProc() class FakeRunner: + """Runner stub that always returns the fake process.""" + def popen(self, **_kw: object) -> FakeProc: + """Return the pre-created fake process.""" return fake_proc class FakeCargoCommand: + """Cargo command stub that returns the fake runner.""" + def __getitem__(self, _args: list[str]) -> FakeRunner: + """Return a runner for the requested cargo arguments.""" return FakeRunner() monkeypatch.setattr(mod, "cargo", FakeCargoCommand()) @@ -952,6 +986,8 @@ def fake_echo(message: str, *, err: bool = False, nl: bool = True) -> None: monkeypatch.setenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "1") class FakeThread: + """Thread stub that stays alive for a fixed number of joins.""" + join_calls = 0 def __init__( @@ -962,15 +998,18 @@ def __init__( args: tuple[object, ...], kwargs: dict[str, object], ) -> None: + """Initialise the thread stub with ignored thread metadata.""" _ = (name, target, args, kwargs) def start(self) -> None: - return None + """Pretend to start the thread.""" def is_alive(self) -> bool: + """Report liveness until enough joins have occurred.""" return self.join_calls < 4 def join(self, timeout: float | None = None) -> None: + """Record a join call without blocking.""" _ = timeout type(self).join_calls += 1 @@ -979,19 +1018,24 @@ def join(self, timeout: float | None = None) -> None: monkeypatch.setattr(mod.time, "monotonic", lambda: next(time_values)) class FakeProc: + """Minimal subprocess test double for Windows timeout handling.""" + def __init__(self) -> None: + """Initialise the stub with captured output streams.""" self.stdout = io.StringIO("out-line\n") self.stderr = io.StringIO("err-line\n") self.killed = False self.waited = False def poll(self) -> None: - return None + """Report that the process is still running.""" def kill(self) -> None: + """Record that the process was killed.""" self.killed = True def wait(self, timeout: float | None = None) -> int: + """Record the wait and return success.""" _ = timeout self.waited = True return 0 @@ -999,11 +1043,17 @@ def wait(self, timeout: float | None = None) -> int: fake_proc = FakeProc() class FakeRunner: + """Runner stub that returns the fake Windows process.""" + def popen(self, **_kw: object) -> FakeProc: + """Return the pre-created fake process.""" return fake_proc class FakeCargoCommand: + """Cargo command stub that yields the fake Windows runner.""" + def __getitem__(self, _args: list[str]) -> FakeRunner: + """Return a runner for the requested cargo arguments.""" return FakeRunner() monkeypatch.setattr(mod, "cargo", FakeCargoCommand()) @@ -1026,7 +1076,10 @@ def test_run_cargo_windows_pump_exception( monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) class BoomIO(io.StringIO): + """Stream stub whose ``readline`` always raises.""" + def readline(self) -> str: + """Raise a runtime error when the pump reads a line.""" message = "boom in pump" raise RuntimeError(message) @@ -1085,11 +1138,15 @@ def test_run_cargo_stream_close_error_suppressed( monkeypatch.setattr(mod.typer, "echo", lambda *_args, **_kwargs: None) class ExplodingStream(io.StringIO): + """String stream that raises after being closed.""" + def __init__(self, value: str) -> None: + """Initialise the stream with the provided text.""" super().__init__(value) self.close_calls = 0 def close(self) -> None: + """Close the stream, then raise a cleanup error.""" self.close_calls += 1 super().close() message = "close failure" From bf0fa07a8fd3727f0e10e7bee71199f71d61f449 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 20:24:07 +0200 Subject: [PATCH 26/28] Tighten cargo pump cleanup and test helpers --- .../scripts/_cargo_runner.py | 8 +- .../generate-coverage/tests/test_scripts.py | 161 ++++++++++-------- 2 files changed, 93 insertions(+), 76 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/_cargo_runner.py b/.github/actions/generate-coverage/scripts/_cargo_runner.py index 875bb76d..dd3a3538 100644 --- a/.github/actions/generate-coverage/scripts/_cargo_runner.py +++ b/.github/actions/generate-coverage/scripts/_cargo_runner.py @@ -161,9 +161,11 @@ def _pump_cargo_output_windows( ] for thread in threads: thread.start() - while not _poll_pump_loop_iteration(threads, ctx, thread_exceptions): - pass - _finalise_pump_threads(threads, ctx.proc, thread_exceptions) + try: + while not _poll_pump_loop_iteration(threads, ctx, thread_exceptions): + pass + finally: + _finalise_pump_threads(threads, ctx.proc, thread_exceptions) return stdout_lines diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index b7e412f1..0c049748 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -125,7 +125,11 @@ def kill(self) -> None: if track_lifecycle: self.killed = True - def wait(self, timeout: float | None = None) -> int: + def wait( + self, + _timeout: float | None = None, + **_kwargs: float | None, + ) -> int: """Return the configured return code immediately.""" if track_lifecycle: self.waited = True @@ -167,6 +171,82 @@ def popen(self, **_kw: object) -> FakeProc: return FakeCargo() +class _WindowsPumpTimeoutFakeThread: + """Thread stub that stays alive for a fixed number of joins.""" + + def __init__( + self, + *, + name: str, + target: typ.Callable[..., object], + args: tuple[object, ...], + kwargs: dict[str, object], + ) -> None: + """Initialise the thread stub with ignored thread metadata.""" + _ = (name, target, args, kwargs) + self.join_calls = 0 + + def start(self) -> None: + """Pretend to start the thread.""" + + def is_alive(self) -> bool: + """Report liveness until enough joins have occurred.""" + return self.join_calls < 2 + + def join(self, timeout: float | None = None) -> None: + """Record a join call without blocking.""" + _ = timeout + self.join_calls += 1 + + +class _WindowsPumpTimeoutFakeProc: + """Minimal subprocess test double for Windows timeout handling.""" + + def __init__(self) -> None: + """Initialise the stub with captured output streams.""" + self.stdout = io.StringIO("out-line\n") + self.stderr = io.StringIO("err-line\n") + self.killed = False + self.waited = False + + def poll(self) -> None: + """Report that the process is still running.""" + + def kill(self) -> None: + """Record that the process was killed.""" + self.killed = True + + def wait(self, timeout: float | None = None) -> int: + """Record the wait and return success.""" + _ = timeout + self.waited = True + return 0 + + +class _WindowsPumpTimeoutFakeRunner: + """Runner stub that returns the fake Windows process.""" + + def __init__(self, proc: _WindowsPumpTimeoutFakeProc) -> None: + """Initialise the runner with the fake process to return.""" + self._proc = proc + + def popen(self, **_kw: object) -> _WindowsPumpTimeoutFakeProc: + """Return the pre-created fake process.""" + return self._proc + + +class _WindowsPumpTimeoutFakeCargoCommand: + """Cargo command stub that yields the fake Windows runner.""" + + def __init__(self, proc: _WindowsPumpTimeoutFakeProc) -> None: + """Initialise the cargo command with the fake process.""" + self._proc = proc + + def __getitem__(self, _args: list[str]) -> _WindowsPumpTimeoutFakeRunner: + """Return a runner for the requested cargo arguments.""" + return _WindowsPumpTimeoutFakeRunner(self._proc) + + @dataclasses.dataclass(frozen=True, slots=True) class RustCoverageConfig: """Configuration for rust coverage test runs.""" @@ -906,6 +986,7 @@ def test_main_reuses_cargo_env_for_cucumber( github_output = tmp_path / "gh.txt" cargo_env = {"CARGO_PROFILE_DEV_CODEGEN_BACKEND": "llvm"} env_calls: list[Path] = [] + run_cargo_env_calls: list[typ.Mapping[str, str] | None] = [] cucumber_calls: list[dict[str, object]] = [] def fake_get_cargo_coverage_env(manifest_path: Path) -> dict[str, str]: @@ -918,7 +999,8 @@ def fake_run_cargo( env_overrides: typ.Mapping[str, str] | None = None, env_unsets: typ.Iterable[str] = (), ) -> str: - _ = (args, env_overrides, env_unsets) + _ = (args, env_unsets) + run_cargo_env_calls.append(env_overrides) return "Coverage: 100%" cucumber_spy = _make_cucumber_spy(cucumber_calls) @@ -946,6 +1028,7 @@ def fake_run_cargo( ) assert env_calls == [manifest_path] + assert run_cargo_env_calls == [cargo_env] assert len(cucumber_calls) == 1 assert cucumber_calls[0]["cargo_env"] == cargo_env @@ -984,79 +1067,11 @@ def fake_echo(message: str, *, err: bool = False, nl: bool = True) -> None: monkeypatch.setattr(mod.typer, "echo", fake_echo) monkeypatch.setenv("RUN_RUST_CARGO_WAIT_TIMEOUT", "1") - - class FakeThread: - """Thread stub that stays alive for a fixed number of joins.""" - - join_calls = 0 - - def __init__( - self, - *, - name: str, - target: typ.Callable[..., object], - args: tuple[object, ...], - kwargs: dict[str, object], - ) -> None: - """Initialise the thread stub with ignored thread metadata.""" - _ = (name, target, args, kwargs) - - def start(self) -> None: - """Pretend to start the thread.""" - - def is_alive(self) -> bool: - """Report liveness until enough joins have occurred.""" - return self.join_calls < 4 - - def join(self, timeout: float | None = None) -> None: - """Record a join call without blocking.""" - _ = timeout - type(self).join_calls += 1 - - monkeypatch.setattr(mod.threading, "Thread", FakeThread) + monkeypatch.setattr(mod.threading, "Thread", _WindowsPumpTimeoutFakeThread) time_values = iter([0.0, 0.0, 1.0]) monkeypatch.setattr(mod.time, "monotonic", lambda: next(time_values)) - - class FakeProc: - """Minimal subprocess test double for Windows timeout handling.""" - - def __init__(self) -> None: - """Initialise the stub with captured output streams.""" - self.stdout = io.StringIO("out-line\n") - self.stderr = io.StringIO("err-line\n") - self.killed = False - self.waited = False - - def poll(self) -> None: - """Report that the process is still running.""" - - def kill(self) -> None: - """Record that the process was killed.""" - self.killed = True - - def wait(self, timeout: float | None = None) -> int: - """Record the wait and return success.""" - _ = timeout - self.waited = True - return 0 - - fake_proc = FakeProc() - - class FakeRunner: - """Runner stub that returns the fake Windows process.""" - - def popen(self, **_kw: object) -> FakeProc: - """Return the pre-created fake process.""" - return fake_proc - - class FakeCargoCommand: - """Cargo command stub that yields the fake Windows runner.""" - - def __getitem__(self, _args: list[str]) -> FakeRunner: - """Return a runner for the requested cargo arguments.""" - return FakeRunner() - - monkeypatch.setattr(mod, "cargo", FakeCargoCommand()) + fake_proc = _WindowsPumpTimeoutFakeProc() + monkeypatch.setattr(mod, "cargo", _WindowsPumpTimeoutFakeCargoCommand(fake_proc)) with pytest.raises(mod.typer.Exit) as excinfo: mod._run_cargo(["llvm-cov"]) From 88dd7164eae78545e79cc19029d5b35de7cd2321 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 20:26:58 +0200 Subject: [PATCH 27/28] Extract Windows pump proc test double --- .../generate-coverage/tests/test_scripts.py | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/.github/actions/generate-coverage/tests/test_scripts.py b/.github/actions/generate-coverage/tests/test_scripts.py index 0c049748..e9ab890b 100644 --- a/.github/actions/generate-coverage/tests/test_scripts.py +++ b/.github/actions/generate-coverage/tests/test_scripts.py @@ -199,38 +199,14 @@ def join(self, timeout: float | None = None) -> None: self.join_calls += 1 -class _WindowsPumpTimeoutFakeProc: - """Minimal subprocess test double for Windows timeout handling.""" - - def __init__(self) -> None: - """Initialise the stub with captured output streams.""" - self.stdout = io.StringIO("out-line\n") - self.stderr = io.StringIO("err-line\n") - self.killed = False - self.waited = False - - def poll(self) -> None: - """Report that the process is still running.""" - - def kill(self) -> None: - """Record that the process was killed.""" - self.killed = True - - def wait(self, timeout: float | None = None) -> int: - """Record the wait and return success.""" - _ = timeout - self.waited = True - return 0 - - class _WindowsPumpTimeoutFakeRunner: """Runner stub that returns the fake Windows process.""" - def __init__(self, proc: _WindowsPumpTimeoutFakeProc) -> None: + def __init__(self, proc: _WindowsPumpFakeProc) -> None: """Initialise the runner with the fake process to return.""" self._proc = proc - def popen(self, **_kw: object) -> _WindowsPumpTimeoutFakeProc: + def popen(self, **_kw: object) -> _WindowsPumpFakeProc: """Return the pre-created fake process.""" return self._proc @@ -238,7 +214,7 @@ def popen(self, **_kw: object) -> _WindowsPumpTimeoutFakeProc: class _WindowsPumpTimeoutFakeCargoCommand: """Cargo command stub that yields the fake Windows runner.""" - def __init__(self, proc: _WindowsPumpTimeoutFakeProc) -> None: + def __init__(self, proc: _WindowsPumpFakeProc) -> None: """Initialise the cargo command with the fake process.""" self._proc = proc @@ -1053,6 +1029,30 @@ def test_run_cargo_windows_nonzero_exit( assert _exit_code(excinfo.value) == 1 +class _WindowsPumpFakeProc: + """Minimal subprocess test double for Windows timeout handling.""" + + def __init__(self) -> None: + """Initialise the stub with captured output streams.""" + self.stdout = io.StringIO("out-line\n") + self.stderr = io.StringIO("err-line\n") + self.killed = False + self.waited = False + + def poll(self) -> None: + """Report that the process is still running.""" + + def kill(self) -> None: + """Record that the process was killed.""" + self.killed = True + + def wait(self, timeout: float | None = None) -> int: + """Record the wait and return success.""" + _ = timeout + self.waited = True + return 0 + + def test_run_cargo_windows_pump_timeout( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -1070,7 +1070,7 @@ def fake_echo(message: str, *, err: bool = False, nl: bool = True) -> None: monkeypatch.setattr(mod.threading, "Thread", _WindowsPumpTimeoutFakeThread) time_values = iter([0.0, 0.0, 1.0]) monkeypatch.setattr(mod.time, "monotonic", lambda: next(time_values)) - fake_proc = _WindowsPumpTimeoutFakeProc() + fake_proc = _WindowsPumpFakeProc() monkeypatch.setattr(mod, "cargo", _WindowsPumpTimeoutFakeCargoCommand(fake_proc)) with pytest.raises(mod.typer.Exit) as excinfo: From 3f4d6449e96a05945d341627720cf62a46905964 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 17 Apr 2026 20:28:56 +0200 Subject: [PATCH 28/28] Use shared cargo context for POSIX pump --- .../scripts/_cargo_runner.py | 35 ++++++------------- 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/.github/actions/generate-coverage/scripts/_cargo_runner.py b/.github/actions/generate-coverage/scripts/_cargo_runner.py index dd3a3538..e38c20bb 100644 --- a/.github/actions/generate-coverage/scripts/_cargo_runner.py +++ b/.github/actions/generate-coverage/scripts/_cargo_runner.py @@ -200,27 +200,20 @@ def _kill_cargo_process(proc: subprocess.Popen[str]) -> None: def _pump_cargo_output_posix( - proc: subprocess.Popen[str], stdout_stream: typ.IO[str], stderr_stream: typ.IO[str], - *, - deadline: float, - wait_timeout: float, + ctx: _CargoProcCtx, ) -> list[str]: """Pump cargo output on POSIX using a selector-based event loop. Parameters ---------- - proc: - The running cargo process. stdout_stream: - The captured stdout text stream from *proc*. + The captured stdout text stream from the cargo process. stderr_stream: - The captured stderr text stream from *proc*. - deadline: - Absolute ``time.monotonic()`` value after which the pump is aborted. - wait_timeout: - Original timeout in seconds, used only in the error message. + The captured stderr text stream from the cargo process. + ctx: + Shared context holding the process handle, deadline, and timeout. Returns ------- @@ -233,13 +226,13 @@ def _pump_cargo_output_posix( sel.register(stdout_stream, selectors.EVENT_READ, data="stdout") sel.register(stderr_stream, selectors.EVENT_READ, data="stderr") while sel.get_map(): - if time.monotonic() >= deadline: - _raise_cargo_timeout(proc, wait_timeout=wait_timeout) - timeout = max(0.0, deadline - time.monotonic()) + if time.monotonic() >= ctx.deadline: + _raise_cargo_timeout(ctx.proc, wait_timeout=ctx.wait_timeout) + timeout = max(0.0, ctx.deadline - time.monotonic()) for key, _ in sel.select(timeout): _handle_cargo_output_event(key, stdout_lines, sel) except Exception: - _kill_cargo_process(proc) + _kill_cargo_process(ctx.proc) raise finally: sel.close() @@ -269,16 +262,10 @@ def _pump_cargo_output( stdout_stream = proc.stdout stderr_stream = proc.stderr + ctx = _CargoProcCtx(proc=proc, deadline=deadline, wait_timeout=wait_timeout) if os.name == "nt": - ctx = _CargoProcCtx(proc=proc, deadline=deadline, wait_timeout=wait_timeout) return _pump_cargo_output_windows(stdout_stream, stderr_stream, ctx) - return _pump_cargo_output_posix( - proc, - stdout_stream, - stderr_stream, - deadline=deadline, - wait_timeout=wait_timeout, - ) + return _pump_cargo_output_posix(stdout_stream, stderr_stream, ctx) def _build_cargo_env(