Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion .github/actions/rust-build-release/src/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -501,19 +501,30 @@ def _normalize_features(features: str) -> str:
return ",".join(normalized)


def _assert_cross_command_has_no_toolchain_override(cmd: cabc.Sequence[object]) -> None:
# Cross must not be given a +<toolchain>; rely on rust-toolchain.toml /
# rustup override.
if any(isinstance(arg, str) and arg.startswith("+") for arg in cmd[1:3]):
message = "cross command must not include a +<toolchain> override"
raise ValueError(message)


def _build_cross_command(
decision: _CrossDecision, target_to_build: str, manifest_path: Path, features: str
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
) -> SupportsFormulate:
cross_executable = decision.cross_path or "cross"
executor = local[cross_executable]
build_cmd = executor[
cmd: list[object] = [
cross_executable,
"build",
"--manifest-path",
str(manifest_path),
"--release",
"--target",
target_to_build,
]
_assert_cross_command_has_no_toolchain_override(cmd)
build_cmd = executor[cmd[1:]]
normalized_features = _normalize_features(features)
if normalized_features:
build_cmd = build_cmd["--features", normalized_features]
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Expand Down Expand Up @@ -667,6 +678,7 @@ def main(
build_cmd = typ.cast("_SupportsEnvFormulate", build_cmd).with_env(
RUSTUP_TOOLCHAIN=toolchain_name
)
typer.echo(f"::debug:: cross argv: {build_cmd}")
else:
build_cmd = _build_cargo_command(
decision.cargo_toolchain_spec, target_to_build, manifest_argument, features
Expand Down
145 changes: 145 additions & 0 deletions .github/actions/rust-build-release/tests/test_commands.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
"""Regression tests for rust-build-release command construction."""

from __future__ import annotations

import importlib.util
import os
import typing as typ
from pathlib import Path

import pytest
from plumbum.commands.processes import ProcessExecutionError
from rust_build_release_test_helpers import assert_no_toolchain_override

SRC_DIR = Path(__file__).resolve().parents[1] / "src"
REPO_ROOT = Path(__file__).resolve().parents[4]

if typ.TYPE_CHECKING:
from types import ModuleType


def _make_cross_executable(tmp_path: Path) -> Path:
cross_path = tmp_path / "cross"
cross_path.write_text("#!/bin/sh\n", encoding="utf-8")
cross_path.chmod(cross_path.stat().st_mode | os.X_OK)
return cross_path
Comment thread
coderabbitai[bot] marked this conversation as resolved.


def _load_main_module(monkeypatch: pytest.MonkeyPatch) -> ModuleType:
monkeypatch.setenv("GITHUB_ACTION_PATH", str(REPO_ROOT))
monkeypatch.syspath_prepend(str(SRC_DIR))

import packaging
import packaging.version as packaging_version

spec = importlib.util.spec_from_file_location(
"rbr_main_commands", SRC_DIR / "main.py"
)
if spec is None or spec.loader is None:
msg = f"failed to load main.py from {SRC_DIR}"
raise RuntimeError(msg)

module = importlib.util.module_from_spec(spec)
try:
spec.loader.exec_module(module)
finally:
if getattr(packaging, "version", None) is packaging_version:
delattr(packaging, "version")
return module
Comment thread
coderabbitai[bot] marked this conversation as resolved.


def test_build_cross_command_never_injects_toolchain_override(
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
) -> None:
"""Cross commands start with cross build and omit +toolchain arguments."""
main_module = _load_main_module(monkeypatch)
cross_path = _make_cross_executable(tmp_path)
decision = main_module._CrossDecision(
cross_path=str(cross_path),
cross_version="0.2.5",
Comment thread
leynos marked this conversation as resolved.
use_cross=True,
cargo_toolchain_spec="+bogus-nightly",
use_cross_local_backend=False,
docker_present=True,
podman_present=False,
has_container=True,
container_engine="docker",
requires_cross_container=False,
)

cmd = main_module._build_cross_command(
decision,
"aarch64-unknown-linux-gnu",
tmp_path / "Cargo.toml",
"",
)

parts = list(cmd.formulate())
assert parts[0] == "cross"
assert_no_toolchain_override(parts)


def test_cross_container_fallback_keeps_cargo_toolchain_override(
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
) -> None:
"""The container-error fallback remains cargo-only and keeps +toolchain."""
main_module = _load_main_module(monkeypatch)
calls: list[list[str]] = []

def fake_run_cmd(cmd: object) -> None:
formulated = list(cmd.formulate())
Comment thread
leynos marked this conversation as resolved.
Outdated
if formulated:
formulated[0] = Path(formulated[0]).name
calls.append(formulated)

monkeypatch.setattr(main_module, "run_cmd", fake_run_cmd)
decision = main_module._CrossDecision(
cross_path="/usr/bin/cross",
cross_version="0.2.5",
use_cross=True,
cargo_toolchain_spec="+bogus-nightly",
use_cross_local_backend=False,
docker_present=True,
podman_present=False,
has_container=True,
container_engine="docker",
requires_cross_container=False,
)
exc = ProcessExecutionError(["cross", "build"], 125, "", "")

main_module._handle_cross_container_error(
exc,
decision,
"aarch64-unknown-linux-gnu",
tmp_path / "Cargo.toml",
"",
)

assert calls == [
[
"cargo",
"+bogus-nightly",
"build",
"--manifest-path",
str(tmp_path / "Cargo.toml"),
"--release",
"--target",
"aarch64-unknown-linux-gnu",
]
]


def test_cross_command_guard_rejects_toolchain_override(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""The cross argv guard catches accidental +toolchain injection."""
main_module = _load_main_module(monkeypatch)
with pytest.raises(
ValueError,
match=r"cross command must not include a \+<toolchain> override",
):
main_module._assert_cross_command_has_no_toolchain_override(
["cross", "+bogus-nightly", "build"]
)
16 changes: 10 additions & 6 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ clean: ## Remove transient artefacts
BUILD_JOBS ?=
MDLINT ?= markdownlint
NIXIE ?= nixie
ACTION_VALIDATOR_CANDIDATES := $(wildcard $(HOME)/.cargo/bin/action-validator /usr/local/bin/action-validator /usr/bin/action-validator)
ACTION_VALIDATOR ?= $(if $(ACTION_VALIDATOR_CANDIDATES),$(firstword $(ACTION_VALIDATOR_CANDIDATES)),action-validator)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
RUFF_CANDIDATES := $(wildcard $(CURDIR)/.venv/bin/ruff $(HOME)/.local/bin/ruff /usr/local/bin/ruff /usr/bin/ruff)
RUFF ?= $(if $(RUFF_CANDIDATES),$(firstword $(RUFF_CANDIDATES)),uv tool run ruff)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

uv tool run unpinned package version resolution documentation

💡 Result:

When using uv tool run (or its alias uvx) without specifying a version (i.e., unpinned tool name like ruff), uv resolves and installs the latest compatible version of the tool package and its dependencies into a temporary, cached virtual environment isolated from any project. By default, uv's resolution strategy prefers the highest (latest) compatible version for each package, both direct (the tool) and transitive (its dependencies). This is the standard behavior across uv commands like uv pip install, uv run --with, and uv tool run. For uv tool run specifically: - On first run, it performs full resolution to select the latest versions satisfying the tool's declared requirements. - Subsequent runs reuse the cached environment unless the cache is pruned/refreshed, a different version is requested (e.g., ruff@latest or --from ruff==0.3.0), or --isolated is used. - The cache is disposable (in ~/.cache/uv), unlike installed tools via uv tool install which are persistent. To override the default "highest" resolution (e.g., for testing lowest versions), use the --resolution flag, which applies to uv tool run as it inherits from uv's run interface: - --resolution lowest: Lowest compatible versions for all dependencies. - --resolution lowest-direct: Lowest for direct (tool), highest for transitive. Example: uv tool run --resolution lowest-direct ruff check . Official documentation confirms uv resolves unpinned requirements to latest versions by default, with configurable strategies via --resolution. For reproducible exact versions, specify the tool version explicitly (e.g., uvx ruff@0.3.0) or use uv tool install first. No dedicated lockfile for tools; caching provides reproducibility until refreshed.

Citations:


Pin the Ruff fallback version.

The unpinned uv tool run ruff fallback resolves to the latest compatible version of Ruff on first run and reuses the cached environment thereafter. However, uv's cache is disposable, meaning the fallback can drift to newer Ruff releases across CI and local runs, destabilising lint and format output. Specify an explicit version using --from ruff==<version> to ensure deterministic behaviour.

Apply a deterministic Ruff fallback
+RUFF_VERSION ?= 0.11.13
-RUFF ?= $(if $(RUFF_CANDIDATES),$(firstword $(RUFF_CANDIDATES)),uv tool run ruff)
+RUFF ?= $(if $(RUFF_CANDIDATES),$(firstword $(RUFF_CANDIDATES)),uv tool run --from ruff==$(RUFF_VERSION) ruff)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Makefile` at line 24, Add a deterministic Ruff version and use it for the
fallback: introduce a RUFF_VERSION variable (e.g., RUFF_VERSION ?= 0.11.13) and
update the RUFF fallback assignment so the fallback uses `uv tool run --from
ruff==$(RUFF_VERSION) ruff` instead of unpinned `uv tool run ruff`; update the
existing RUFF ?= ... line and add the RUFF_VERSION ?= ... declaration near it to
ensure consistent lint/format behavior across runs.

RUFF_FIX_RULES ?= D202,I001

test: .venv ## Run tests
Expand All @@ -30,9 +34,9 @@ endif
uv sync --group dev

lint: ## Check test scripts and actions
uvx ruff check
$(RUFF) check
find .github/actions -type f \( -name 'action.yml' -o -name 'action.yaml' \) -print0 \
| xargs -r -0 -n1 action-validator
| xargs -r -0 -n1 $(ACTION_VALIDATOR)

typecheck: .venv ## Run static type checking with Ty
./.venv/bin/ty check \
Expand All @@ -58,12 +62,12 @@ typecheck: .venv ## Run static type checking with Ty
--extra-search-path .github/actions/macos-package/scripts \
.github/actions/macos-package/scripts
fmt: ## Format Python files and auto-fix selected lint rules
uvx ruff format
uvx ruff check --select $(RUFF_FIX_RULES) --fix
$(RUFF) format
$(RUFF) check --select $(RUFF_FIX_RULES) --fix

check-fmt: ## Check Python formatting without modifying files
uvx ruff format --check
uvx ruff check --select $(RUFF_FIX_RULES)
$(RUFF) format --check
$(RUFF) check --select $(RUFF_FIX_RULES)

markdownlint: ## Lint Markdown files
find . -type f -name '*.md' -not -path './target/*' -print0 | xargs -0 -- $(MDLINT)
Expand Down
Loading