Fix rust host detection timeout and Windows bin path#91
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideImplements a timeout for the Rust host triple probe (falling back to the default on timeout) and refactors the Windows setup action to use Join-Path for constructing the llvm-mingw bin path. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdd a 10-second timeout to rustc probing in detect_host_target, explicitly handle subprocess.TimeoutExpired, and maintain default fallback on failure. Add unit tests covering timeout behaviour and argument forwarding. Adjust a Windows action step to compute the llvm-mingw bin path via intermediate variables without changing behaviour. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI as CI Runner
participant RT as detect_host_target
participant RV as run_validated (rustc)
CI->>RT: Invoke detect_host_target(default)
RT->>RV: Execute rustc -vV with timeout=10
alt Success
RV-->>RT: stdout (host: <triple>)
RT-->>CI: Return parsed host triple
else subprocess.TimeoutExpired
RV-->>RT: Raise TimeoutExpired
RT-->>CI: Return default
else CalledProcessError / OSError
RV-->>RT: Raise error
RT-->>CI: Return default
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `.github/actions/rust-build-release/src/runtime.py:105` </location>
<code_context>
capture_output=True,
text=True,
check=True,
+ timeout=10,
)
- except (FileNotFoundError, subprocess.CalledProcessError, OSError):
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests to cover the new timeout parameter in subprocess.run.
The addition of the timeout parameter changes the behaviour of the function. You must provide both behavioural and unit tests to verify that the timeout is handled correctly and that the appropriate exception is raised and caught.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 2
<location> `.github/actions/rust-build-release/src/runtime.py:110` </location>
<code_context>
+ except (
+ FileNotFoundError,
+ subprocess.CalledProcessError,
+ subprocess.TimeoutExpired,
+ OSError,
+ ):
</code_context>
<issue_to_address>
**issue (review_instructions):** Demonstrate handling of subprocess.TimeoutExpired with a test.
If this change is intended to fix a bug where timeouts were not previously handled, you must add a test that demonstrates the bug and verifies the fix.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
Bug fixes *must* be demonstrated by a test.
</details>
</issue_to_address>
### Comment 3
<location> `.github/actions/rust-build-release/src/runtime.py:107-112` </location>
<code_context>
def detect_host_target(
*,
default: str = DEFAULT_HOST_TARGET,
rustc_path: str | Path | None = None,
) -> str:
"""Return the active Rust host triple, defaulting to *default* when unknown."""
candidate = rustc_path or shutil.which("rustc")
if candidate is None:
return default
try:
exec_path = ensure_allowed_executable(candidate, ("rustc", "rustc.exe"))
except UnexpectedExecutableError:
return default
try:
result = run_validated(
exec_path,
["-vV"],
allowed_names=("rustc", "rustc.exe"),
capture_output=True,
text=True,
check=True,
timeout=10,
)
except (
FileNotFoundError,
subprocess.CalledProcessError,
subprocess.TimeoutExpired,
OSError,
):
return default
triple = next(
(
line.partition(":")[2].strip()
for line in (result.stdout or "").splitlines()
if line.startswith("host:")
),
"",
)
return triple or default
</code_context>
<issue_to_address>
**suggestion (code-quality):** Remove redundant exceptions from an except clause ([`remove-redundant-exception`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-exception/))
```suggestion
except (subprocess.CalledProcessError, subprocess.TimeoutExpired, OSError):
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/rust-build-release/tests/test_runtime.py (1)
222-251: Strengthen the timeout‑forwarding test with extra kwargs assertions.Also assert capture flags and allowed_names to lock API contract.
Apply this diff within this hunk:
@@ - call_kwargs: dict[str, object] = {} + call_kwargs: dict[str, object] = {} @@ - def fake_run( + def fake_run( executable: str, args: list[str], *, allowed_names: tuple[str, ...], **kwargs: object, ) -> subprocess.CompletedProcess[str]: - _ = (executable, args, allowed_names) - call_kwargs.update(kwargs) + _ = (executable, args) + call_kwargs.update(kwargs) + call_kwargs["allowed_names"] = allowed_names return subprocess.CompletedProcess( [executable, *args], 0, stdout="host: bounded\n" ) @@ - assert runtime_module.detect_host_target() == "bounded" - assert call_kwargs.get("timeout") == 10 + assert runtime_module.detect_host_target() == "bounded" + assert call_kwargs.get("timeout") == 10 + assert call_kwargs.get("capture_output") is True + assert call_kwargs.get("text") is True + assert call_kwargs.get("check") is True + assert call_kwargs.get("allowed_names") == ("rustc", "rustc.exe")
♻️ Duplicate comments (1)
.github/actions/rust-build-release/tests/test_runtime.py (1)
194-220: Timeout path test: LGTM and addresses prior request.Exercise TimeoutExpired → default fallback as previously requested by the bot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/actions/rust-build-release/src/runtime.py(1 hunks).github/actions/rust-build-release/tests/test_runtime.py(1 hunks).github/actions/setup-windows-gnu/action.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
.github/actions/setup-windows-gnu/action.yml (1)
57-58: Use Join-Path for bin path: LGTM.Eliminate backslash escaping and path concatenation pitfalls. Behaviour unchanged.
* Fix rust host detection timeout and Windows bin path * Add timeout coverage for rust host detection * Unify runtime probe timeout configuration * test: capture rustc probe kwargs in timeout test
* Add toolchain-specific cargo cache key and clean shellstub imports (#88) * Address review feedback for shellstub imports * Fix MSYS2 install comments breaking pacman (#89) * Fix MSYS2 install comments breaking pacman * Fix Windows llvm-mingw metadata export * Handle runtime detection timeouts * Fix rust host detection timeout and Windows bin path (#91) * Fix rust host detection timeout and Windows bin path * Add timeout coverage for rust host detection * Unify runtime probe timeout configuration * test: capture rustc probe kwargs in timeout test * Remove inline comments from Windows GNU MSYS2 package list (#99) * Remove inline MSYS2 install comments * Skip nfpm steps on Windows builds * Handle nfpm packaging only for Linux * Fix Windows invocation of rust-build-release (#102) * Silence type-check import lints for release action * Adds ignore rule for Crush agent artifacts Ignores files generated by the Crush AI agent to prevent them from being tracked in version control. Adds a symlink to AGENTS.md for discovery by Crush * Format code after rebase validation to satisfy repo style checks. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> * fmt: extend fmt target to run ruff check --select D202,I001 --fix; apply resulting fixes 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> --------- Co-authored-by: Payton McIntosh <pmcintosh@df12.net> Co-authored-by: Crush <crush@charm.land>
* Add toolchain-specific cargo cache key and clean shellstub imports (#88) * Address review feedback for shellstub imports * Fix MSYS2 install comments breaking pacman (#89) * Fix MSYS2 install comments breaking pacman * Fix Windows llvm-mingw metadata export * Handle runtime detection timeouts * Fix rust host detection timeout and Windows bin path (#91) * Fix rust host detection timeout and Windows bin path * Add timeout coverage for rust host detection * Unify runtime probe timeout configuration * test: capture rustc probe kwargs in timeout test * Remove inline comments from Windows GNU MSYS2 package list (#99) * Remove inline MSYS2 install comments * Skip nfpm steps on Windows builds * Handle nfpm packaging only for Linux * Fix Windows invocation of rust-build-release (#102) * Silence type-check import lints for release action * Adds ignore rule for Crush agent artifacts Ignores files generated by the Crush AI agent to prevent them from being tracked in version control. Adds a symlink to AGENTS.md for discovery by Crush * Format code after rebase validation to satisfy repo style checks. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> * fmt: extend fmt target to run ruff check --select D202,I001 --fix; apply resulting fixes 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> --------- Co-authored-by: Payton McIntosh <pmcintosh@df12.net> Co-authored-by: Crush <crush@charm.land>
* Add toolchain-specific cargo cache key and clean shellstub imports (#88) * Address review feedback for shellstub imports * Fix MSYS2 install comments breaking pacman (#89) * Fix MSYS2 install comments breaking pacman * Fix Windows llvm-mingw metadata export * Handle runtime detection timeouts * Fix rust host detection timeout and Windows bin path (#91) * Fix rust host detection timeout and Windows bin path * Add timeout coverage for rust host detection * Unify runtime probe timeout configuration * test: capture rustc probe kwargs in timeout test * Remove inline comments from Windows GNU MSYS2 package list (#99) * Remove inline MSYS2 install comments * Skip nfpm steps on Windows builds * Handle nfpm packaging only for Linux * Fix Windows invocation of rust-build-release (#102) * Silence type-check import lints for release action * Adds ignore rule for Crush agent artifacts Ignores files generated by the Crush AI agent to prevent them from being tracked in version control. Adds a symlink to AGENTS.md for discovery by Crush * Format code after rebase validation to satisfy repo style checks. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> * fmt: extend fmt target to run ruff check --select D202,I001 --fix; apply resulting fixes 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> --------- Co-authored-by: Payton McIntosh <pmcintosh@df12.net> Co-authored-by: Crush <crush@charm.land>
* Add toolchain-specific cargo cache key and clean shellstub imports (#88) * Address review feedback for shellstub imports * Fix MSYS2 install comments breaking pacman (#89) * Fix MSYS2 install comments breaking pacman * Fix Windows llvm-mingw metadata export * Handle runtime detection timeouts * Fix rust host detection timeout and Windows bin path (#91) * Fix rust host detection timeout and Windows bin path * Add timeout coverage for rust host detection * Unify runtime probe timeout configuration * test: capture rustc probe kwargs in timeout test * Remove inline comments from Windows GNU MSYS2 package list (#99) * Remove inline MSYS2 install comments * Skip nfpm steps on Windows builds * Handle nfpm packaging only for Linux * Fix Windows invocation of rust-build-release (#102) * Silence type-check import lints for release action * Adds ignore rule for Crush agent artifacts Ignores files generated by the Crush AI agent to prevent them from being tracked in version control. Adds a symlink to AGENTS.md for discovery by Crush * Format code after rebase validation to satisfy repo style checks. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> * fmt: extend fmt target to run ruff check --select D202,I001 --fix; apply resulting fixes 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> --------- Co-authored-by: Payton McIntosh <pmcintosh@df12.net> Co-authored-by: Crush <crush@charm.land>
* Add toolchain-specific cargo cache key and clean shellstub imports (#88) * Address review feedback for shellstub imports * Fix MSYS2 install comments breaking pacman (#89) * Fix MSYS2 install comments breaking pacman * Fix Windows llvm-mingw metadata export * Handle runtime detection timeouts * Fix rust host detection timeout and Windows bin path (#91) * Fix rust host detection timeout and Windows bin path * Add timeout coverage for rust host detection * Unify runtime probe timeout configuration * test: capture rustc probe kwargs in timeout test * Remove inline comments from Windows GNU MSYS2 package list (#99) * Remove inline MSYS2 install comments * Skip nfpm steps on Windows builds * Handle nfpm packaging only for Linux * Fix Windows invocation of rust-build-release (#102) * Silence type-check import lints for release action * Adds ignore rule for Crush agent artifacts Ignores files generated by the Crush AI agent to prevent them from being tracked in version control. Adds a symlink to AGENTS.md for discovery by Crush * Format code after rebase validation to satisfy repo style checks. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> * fmt: extend fmt target to run ruff check --select D202,I001 --fix; apply resulting fixes 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> --------- Co-authored-by: Payton McIntosh <pmcintosh@df12.net> Co-authored-by: Crush <crush@charm.land>
* Add release-to-pypi-uv composite action * Document trusted publishing permissions * Clarify GH_TOKEN handling in README * Fix local usage example * Improve GitHub release error handling * Fix cmd_utils discovery in publish script * Skip more build caches * Fail fast on TOML parse errors * Format release summary output * Add no-tag error test * Test TOML parse failures * Make uv python version configurable * Document concurrency guard in usage * Address review feedback with retries and tests * Improve TOML version validation and test strategy Extends boolean parsing to support more truthy values when validating dynamic version flags in TOML files. Refactors tests to run in-process with better coverage of various flag values and scenarios, replacing subprocess-based tests for efficiency and detail. Updates action to simplify Python version handling with uv and improves path detection for testing. These changes make validation logic more robust and enhance test reliability. * Add cmd-mox users guide * Use cmd-mox to stub rust build command tests (#92) * Use cmd-mox for rust build command tests * Guard cmd-mox integration on Windows * Refactor cmd-mox helpers into shared conftest * Apply cmd-mox fake which simplification * Mark Windows smoke tests as xfail (#94) * Mark Windows smoke tests as xfail * Tighten Windows smoke test expectations * Address reviewer feedback for release-to-pypi-uv action (#98) * Address review comments for release to PyPI action * Add tests and docs for python-version handling * Fix cmd_mox fixture usage and expose dev extra (#101) * Fix cmd_mox fixture usage and expose dev extras * Remove pyproject optional dev extra * Remove Pyright configuration * Add missing docstrings to release-to-pypi-uv modules (#103) * Silence type-check import lints for release action (#104) * Add toolchain-specific cargo cache key and clean shellstub imports (#88) * Address review feedback for shellstub imports * Fix MSYS2 install comments breaking pacman (#89) * Fix MSYS2 install comments breaking pacman * Fix Windows llvm-mingw metadata export * Handle runtime detection timeouts * Fix rust host detection timeout and Windows bin path (#91) * Fix rust host detection timeout and Windows bin path * Add timeout coverage for rust host detection * Unify runtime probe timeout configuration * test: capture rustc probe kwargs in timeout test * Remove inline comments from Windows GNU MSYS2 package list (#99) * Remove inline MSYS2 install comments * Skip nfpm steps on Windows builds * Handle nfpm packaging only for Linux * Fix Windows invocation of rust-build-release (#102) * Silence type-check import lints for release action * Adds ignore rule for Crush agent artifacts Ignores files generated by the Crush AI agent to prevent them from being tracked in version control. Adds a symlink to AGENTS.md for discovery by Crush * Format code after rebase validation to satisfy repo style checks. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> * fmt: extend fmt target to run ruff check --select D202,I001 --fix; apply resulting fixes 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> --------- Co-authored-by: Payton McIntosh <pmcintosh@df12.net> Co-authored-by: Crush <crush@charm.land> * Narrow module fixtures to ModuleType (#105) * Add docstrings for release-to-pypi-uv tests and helpers (#106) * Add missing docstrings for release-to-pypi-uv tests * Simplify cmd-mox typing and streamline docstrings * Apply formatting * chore: remove unused imports across scripts and tests * style(_helpers): alphabetise __all__ tuple * tests: drop superfluous parentheses from @pytest.fixture usage --------- Co-authored-by: Payton McIntosh <pmcintosh@df12.net> * Scope type-checking imports (#111) * Scope type-checking imports * Fix lint warnings across release scripts * Clarify cargo stream capture error * ci: add lint and format checks * Install action-validator in CI lint workflow * Install bun validator packages in CI * Test full success message for matching versions (#113) * Address review comments (#116) * Address review feedback for release-to-pypi-uv actions (#112) * Address review feedback * Add CLI coverage for publish and validate scripts * Add deterministic validation tests and bunx validator checks * Update ci.yml remove unneeded step * Fix formatting * Fix Windows xfail marker removal for pytest 8 (#120) * Fix Windows xfail marker removal for pytest 8 * Tighten Windows xfail marker filtering * Handle runtime probe timeouts and expand release tests (#119) * Handle runtime probe timeouts and expand release tests * Add _probe_runtime coverage and assert timeout warnings * Expand TOML skip directories for release validation (#122) * Expand pyproject skip list handling * Strengthen TOML and runtime timeout tests * Assert cargo fallback logs podman failure * Parameterize skip-directory regression * Reinforce regression coverage for release tooling (#130) * Refine skip directory regression parameterization * Document skip directories and DRY timeout assertions * Parametrize publish index test * Refactor runtime timeout tests * Rebase onto origin/python-lib-release-action and resolve conflicts - Resolve merge in validate_toml_versions tests by standardizing module loading - Register dynamically loaded scripts in sys.modules to support reload semantics - Make toolchain triple test robust to host arch by selecting matching target - Run formatting, lint, typecheck, and tests to validate integration 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> --------- Co-authored-by: Crush <crush@charm.land> * Harden release-to-pypi-uv workflow and regression coverage (#134) * Improve release validation jitter handling * Adjust runtime fallback host triples per platform * Make release validation deterministic and configurable * Rebase python-lib-release-action onto origin/main; resolve conflicts preserving branch intent; drop uv.lock in favor of main; fix tests and typing/lint issues; all tests pass and linters clean * Close cargo pipes when coverage stream missing (#137) * Close cargo pipes when missing * Ensure cargo pipes close on all paths * Ensure guard closes cargo pipes before exiting * Handle release script auth failure and multiline outputs (#136) * Handle release script auth failure and multiline outputs * Harden windows toolchain setup and extend runtime tests * Handle release auth errors and tweak toolchain retries * Fix cross install warning expectation (#139) * Fix cross install warning assertion * Harden cross install harness checks * Sanitize runtime probe timeout and guard actions * Refine runtime probe helpers and add timeout tests * Fix runtime probe lint findings * Share echo recorder fixture across runtime tests * Update fmt target description (#140) * Resolve rebase conflicts and align runtime tests with platform/timeouts from main while preserving branch improvements; ensure formatting passes and all tests/linters are green. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land> --------- Co-authored-by: Payton McIntosh <pmcintosh@df12.net> Co-authored-by: Crush <crush@charm.land>
Summary
Join-Pathto avoid embedded backslashes in the setup-windows-gnu actionTesting
make linthttps://chatgpt.com/codex/tasks/task_e_68cc41dcc0b48322baaf86145d832bab
Summary by Sourcery
Improve reliability of host detection by timing out stalled rustc probes and enhance Windows setup by correctly building the llvm-mingw bin path
Bug Fixes:
Enhancements: