Conversation
Reviewer's GuideIntroduce support for requiring containerized cross builds when targeting FreeBSD from non-FreeBSD hosts by adding detection logic and error handling, extending test coverage, updating CI matrix, and documenting the new behavior. Sequence diagram for FreeBSD cross build error handlingsequenceDiagram
participant "main()"
participant "_requires_cross_container()"
participant "decision"
participant "typer"
"main()"->>"_requires_cross_container()": Check if cross container is required
"_requires_cross_container()"-->>"main()": Return True/False
alt Cross container required and not available
"main()"->>"decision": Check cross_path, use_cross_local_backend, has_container
"main()"->>"typer": Emit error message
"main()"->>"typer": Exit with error
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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
WalkthroughEnforce container-backed cross builds for FreeBSD targets on non-FreeBSD hosts; detect Docker/Podman and export Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Workflow
participant Action as rust-build-release/main.py
participant Host as Host Env
participant Cross as cross Tool
participant CR as Container Runtime
CI->>Action: Run build for target (e.g. x86_64-unknown-freebsd)
Action->>Action: Compute host_target and decide cross usage
alt FreeBSD target on non-FreeBSD host
Action->>Host: Check `cross` installed
Host-->>Action: cross present? (yes/no)
Action->>Host: Probe container runtimes (docker/podman)
Host-->>Action: runtime present? (docker/podman/none)
alt Missing cross or runtime
Action-->>CI: Exit with error (requires cross + container runtime)
else Prerequisites present
Action->>Action: Export `CROSS_CONTAINER_ENGINE`
Action->>Cross: Invoke cross (container-backed)
Cross->>CR: Launch container build
CR-->>Cross: Build completes
Cross-->>Action: Return status
Action-->>CI: Success
end
else No container requirement
Action->>Host: Use cargo/cross as usual
Action-->>CI: Proceed with standard build
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1).github/actions/rust-build-release/src/main.py (2)
🔍 Remote MCP DeepwikiBelow is the key context for PR #150 review:
Summary:
⏰ 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). (6)
🔇 Additional comments (10)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.13.2).github/actions/rust-build-release/src/main.py�[1;31mruff failed�[0m Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Refactor the new _requires_cross_container logic into the existing decision flow (e.g. CrossDecision) to avoid duplicating normalization and make cross support rules more centralized.
- Simplify the error message detail suffix by listing missing prerequisites in a single, concise phrase (using commas instead of a semicolon list) to improve readability.
- Add a positive-path test for FreeBSD targets to verify that builds succeed when both cross and a container runtime are available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Refactor the new _requires_cross_container logic into the existing decision flow (e.g. CrossDecision) to avoid duplicating normalization and make cross support rules more centralized.
- Simplify the error message detail suffix by listing missing prerequisites in a single, concise phrase (using commas instead of a semicolon list) to improve readability.
- Add a positive-path test for FreeBSD targets to verify that builds succeed when both cross and a container runtime are available.
## Individual Comments
### Comment 1
<location> `.github/actions/rust-build-release/src/main.py:408-413` </location>
<code_context>
toolchain_name, installed_names, rustup_exec, target_to_build
)
+ if _requires_cross_container(target_to_build, DEFAULT_HOST_TARGET) and not decision.use_cross:
+ details: list[str] = []
+ if decision.cross_path is None:
+ details.append("cross is not installed")
+ if not decision.use_cross_local_backend and not decision.has_container:
+ details.append("no container runtime detected")
+ detail_suffix = f" ({'; '.join(details)})" if details else ""
+ typer.echo(
+ "::error:: target "
+ f"'{target_to_build}' requires cross with a container runtime on this host"
+ f"{detail_suffix}",
+ err=True,
+ )
+ raise typer.Exit(1)
+
if not target_installed and (
</code_context>
<issue_to_address>
**suggestion:** Error message could clarify which host and target are involved.
Displaying both host and target triples in the error will help users quickly identify configuration issues, particularly in automated environments.
```suggestion
typer.echo(
"::error:: target "
f"'{target_to_build}' requires cross with a container runtime "
f"on host '{DEFAULT_HOST_TARGET}' (target: '{target_to_build}'){detail_suffix}",
err=True,
)
```
</issue_to_address>
### Comment 2
<location> `.github/actions/rust-build-release/src/main.py:142` </location>
<code_context>
return toolchain_name
+def _requires_cross_container(target: str, host_target: str) -> bool:
+ """Return True when *target* needs cross with containers on this host."""
+
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new _requires_cross_container function.
The new function _requires_cross_container introduces logic for determining when a containerized cross build is required. You must add both behavioural and unit tests to verify its correctness under various target/host combinations.
<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 3
<location> `.github/actions/rust-build-release/src/main.py:401` </location>
<code_context>
toolchain_name, installed_names, rustup_exec, target_to_build
)
+ if _requires_cross_container(target_to_build, DEFAULT_HOST_TARGET) and not decision.use_cross:
+ details: list[str] = []
+ if decision.cross_path is None:
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new error handling logic for FreeBSD cross builds.
The new error handling and early exit for FreeBSD cross builds on non-FreeBSD hosts is a behavioural change. You must add tests that demonstrate this behaviour, including cases where the error is triggered and where it is not.
<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 4
<location> `.github/actions/rust-build-release/README.md:5` </location>
<code_context>
Build Rust application release artefacts using the repository's `setup-rust` action, `uv`, and `cross`.
+FreeBSD targets (for example `x86_64-unknown-freebsd`) require `cross` with a
+container runtime when built on non-FreeBSD hosts. The action enforces this so
+that builds fail fast if Docker or Podman are unavailable.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This paragraph exceeds the 80 column wrapping limit for Markdown text.
Please wrap this paragraph so that no line exceeds 80 columns, as per the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 5
<location> `.github/actions/rust-build-release/README.md:6` </location>
<code_context>
Build Rust application release artefacts using the repository's `setup-rust` action, `uv`, and `cross`.
+FreeBSD targets (for example `x86_64-unknown-freebsd`) require `cross` with a
+container runtime when built on non-FreeBSD hosts. The action enforces this so
+that builds fail fast if Docker or Podman are unavailable.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This line is part of a paragraph that is not wrapped to 80 columns.
Ensure all lines in paragraphs and bullet points are wrapped to 80 columns for readability.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 6
<location> `.github/actions/rust-build-release/README.md:7` </location>
<code_context>
+FreeBSD targets (for example `x86_64-unknown-freebsd`) require `cross` with a
+container runtime when built on non-FreeBSD hosts. The action enforces this so
+that builds fail fast if Docker or Podman are unavailable.
+
> [!NOTE]
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This line continues the paragraph and should be wrapped to 80 columns.
Please wrap this paragraph so that each line does not exceed 80 columns.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 7
<location> `.github/actions/rust-build-release/src/main.py:146` </location>
<code_context>
def _requires_cross_container(target: str, host_target: str) -> bool:
"""Return True when *target* needs cross with containers on this host."""
normalized = target.strip().lower()
host_normalized = host_target.strip().lower()
if normalized.endswith("unknown-freebsd"):
return not host_normalized.endswith("unknown-freebsd")
return False
</code_context>
<issue_to_address>
**suggestion (code-quality):** Move assignments closer to their usage ([`move-assign`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/move-assign/))
```suggestion
host_normalized = host_target.strip().lower()
```
</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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/actions/rust-build-release/tests/test_target_install.py (1)
114-146: LGTM! Test validates FreeBSD cross-requirement error path.The test correctly exercises the fail-fast logic when x86_64-unknown-freebsd is requested without cross+container support. The assertion confirms the expected error is raised.
Optionally strengthen the assertion to validate the detailed diagnostic message structure introduced at lines 407-421 of main.py:
err = capsys.readouterr().err assert "requires cross" in err +assert "cross" in err or "container runtime" in err +assert "missing:" in errThis would catch regressions in the diagnostic detail formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/actions/rust-build-release/src/main.py(7 hunks).github/actions/rust-build-release/tests/test_target_install.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
.github/actions/rust-build-release/src/main.py (2)
.github/actions/rust-build-release/src/runtime.py (1)
runtime_available(205-264).github/actions/rust-build-release/src/cross_manager.py (1)
ensure_cross(164-249)
.github/actions/rust-build-release/tests/test_target_install.py (3)
.github/actions/rust-build-release/tests/conftest.py (5)
module_harness(169-178)patch_shutil_which(134-136)patch_platform(138-140)patch_attr(142-144)patch_run_cmd(123-132)conftest.py (8)
cmd_mox(149-153)CmdMox(58-77)_register_rustup_toolchain_stub(115-121)replay(71-73)verify(75-77)_register_cross_version_stub(96-112)_register_docker_info_stub(124-131)_register_podman_info_stub(134-141).github/actions/rust-build-release/src/main.py (1)
main(382-474)
🔍 Remote MCP
Additional Context for FreeBSD Container-Backed Cross Builds
-
The
crosstool requires a Linux host with binfmt_misc support for container-based cross builds. Without binfmt_misc (a kernel facility for interpreting non-native binaries), containerized targets will fail to launch properly in QEMU contexts. -
By default,
crossprobes for Docker first, then Podman, when invoking containerized builds. If neither runtime is found,crosswill error fast. -
You can override which container engine
crossuses by setting theCROSS_CONTAINER_ENGINEenvironment variable. For example,CROSS_CONTAINER_ENGINE=podmanforcescrossto exclusively use Podman.
⏰ 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). (5)
- GitHub Check: python-tests (macos-13)
- GitHub Check: python-tests-windows
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: build-release (windows-latest, x86_64-pc-windows-gnullvm)
- GitHub Check: Sourcery review
🔇 Additional comments (14)
.github/actions/rust-build-release/src/main.py (7)
21-21: LGTM! Import supports host-target detection.The DEFAULT_HOST_TARGET import enables runtime detection of the host platform, which is essential for determining when container-backed cross-compilation is required.
78-79: LGTM! New fields capture container requirements.The
container_engineandrequires_cross_containerfields correctly extend the decision structure to track which container runtime is available and whether the target mandates containerised cross-compilation.
298-302: LGTM! Container engine priority matches cross tool defaults.Docker is correctly prioritised over Podman, aligning with the cross tool's native runtime probing order.
351-352: LGTM! New fields correctly propagated.The
container_engineandrequires_cross_containervalues are correctly returned in the decision structure.
407-421: LGTM! Pre-build validation provides clear diagnostics.The fail-fast check correctly prevents builds when a FreeBSD target is requested without the required cross+container tooling. The error message enumerates missing components and displays both host and target triples, enabling quick diagnosis in CI environments.
435-441: LGTM! Environment management respects user overrides.The logic correctly exports
CROSS_CONTAINER_ENGINEonly when a container runtime is detected and no pre-existing value is set, ensuring user-specified overrides are preserved. The restorer flag correctly tracks whether cleanup is needed.
472-474: LGTM! Cleanup ensures environment hygiene.The finally block correctly removes the exported
CROSS_CONTAINER_ENGINEwhen the script set it, preventing environment pollution for subsequent builds..github/actions/rust-build-release/tests/test_target_install.py (7)
148-195: LGTM! Test validates FreeBSD success path.The test correctly confirms that x86_64-unknown-freebsd builds successfully when cross and a container runtime are available. The assertions verify that cross is invoked and no warnings or errors are emitted.
197-242: LGTM! Test validates Docker environment export and cleanup.The test correctly verifies that
CROSS_CONTAINER_ENGINE=dockeris exported when Docker is detected and removed after the build completes, confirming the environment management logic at lines 435-474 of main.py.
244-289: LGTM! Test validates Podman fallback and cleanup.The test correctly confirms that Podman is selected when Docker is unavailable, and that the environment variable is properly cleaned up, validating the container engine priority logic at lines 298-302 of main.py.
291-336: LGTM! Test validates user override preservation.The test correctly confirms that a pre-existing
CROSS_CONTAINER_ENGINEvalue is not overridden, even when a different container runtime is detected, validating the preservation logic at line 439 of main.py.
425-556: LGTM! Tests validate Windows host container probing logic.The three test functions comprehensively validate that container runtime probing is skipped when a Windows host builds Windows targets (unnecessary), but occurs when cross-compiling to non-Windows targets. The parametrised test covers multiple Windows target suffix variants.
558-707: LGTM! Tests validate timeout handling and error propagation.The five test functions comprehensively validate the
_probe_runtimewrapper's behaviour: successful probes, timeout warnings (with/without duration), unexpected error propagation, and integration with the main build flow. The timeout handling ensures builds can proceed despite slow container runtime probes.
709-819: LGTM! Tests validate Windows linker configuration logic.The two test functions correctly validate that toolchain-provided GCC is preferred for Windows host builds, and that cross-target linkers discovered on PATH are properly exported. The parametrised test covers both GCC and Clang linker variants.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/actions/rust-build-release/src/main.py(9 hunks).github/actions/rust-build-release/tests/test_target_install.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
.github/actions/rust-build-release/src/main.py (2)
.github/actions/rust-build-release/src/runtime.py (1)
runtime_available(205-264).github/actions/rust-build-release/src/cross_manager.py (1)
ensure_cross(164-249)
.github/actions/rust-build-release/tests/test_target_install.py (3)
.github/actions/rust-build-release/tests/conftest.py (7)
main_module(236-245)cross_module(223-232)module_harness(169-178)patch_shutil_which(134-136)patch_platform(138-140)patch_attr(142-144)patch_run_cmd(123-132)conftest.py (8)
cmd_mox(149-153)CmdMox(58-77)_register_rustup_toolchain_stub(115-121)replay(71-73)verify(75-77)_register_cross_version_stub(96-112)_register_docker_info_stub(124-131)_register_podman_info_stub(134-141).github/actions/rust-build-release/src/main.py (2)
main(390-494)_decide_cross_usage(284-361)
⏰ 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). (6)
- GitHub Check: python-tests-windows
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: python-tests (macos-13)
- GitHub Check: build-release (windows-latest, aarch64-pc-windows-gnullvm)
- GitHub Check: build-release (windows-latest, x86_64-pc-windows-gnullvm)
- GitHub Check: Sourcery review
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 613 🏁 Script executed: Length of output: 567 🏁 Script executed: Length of output: 2010 @leynos: Here's the status of the three items: ✅ Item 1: Refactored into CrossDecisionRESOLVED - The
The ❌ Item 2: Error message simplificationNOT RESOLVED - The error message detail suffix still uses semicolons instead of commas. Line 432 shows: detail_suffix = f"; {'; '.join(details)}" if details else ""This produces messages like:
instead of the requested comma-separated format:
✅ Item 3: Positive-path test for FreeBSDRESOLVED - The test Summary: Two of three items are resolved. The error message formatting still needs updating to use commas. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/actions/rust-build-release/src/main.py(9 hunks).github/actions/rust-build-release/tests/test_target_install.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
.github/actions/rust-build-release/tests/test_target_install.py (3)
.github/actions/rust-build-release/tests/conftest.py (7)
main_module(236-245)cross_module(223-232)module_harness(169-178)patch_shutil_which(134-136)patch_attr(142-144)patch_run_cmd(123-132)patch_platform(138-140)conftest.py (8)
cmd_mox(149-153)CmdMox(58-77)_register_rustup_toolchain_stub(115-121)_register_cross_version_stub(96-112)replay(71-73)verify(75-77)_register_docker_info_stub(124-131)_register_podman_info_stub(134-141).github/actions/rust-build-release/src/main.py (2)
main(390-505)_decide_cross_usage(284-361)
.github/actions/rust-build-release/src/main.py (2)
.github/actions/rust-build-release/src/runtime.py (1)
runtime_available(205-264).github/actions/rust-build-release/src/cross_manager.py (1)
ensure_cross(164-249)
⏰ 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). (6)
- GitHub Check: build-release (windows-latest, aarch64-pc-windows-gnullvm)
- GitHub Check: build-release (windows-latest, x86_64-pc-windows-gnullvm)
- GitHub Check: python-tests (macos-13)
- GitHub Check: python-tests-windows
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: Sourcery review
🔇 Additional comments (8)
.github/actions/rust-build-release/tests/test_target_install.py (8)
114-177: Update test assertions once error message formatting is fixed.The test exercises the missing-prerequisites error path but does not validate the separator format (semicolons vs commas). PR objectives note that item 2 (comma-separated error messages) remains unresolved; main.py line ~432 still constructs semicolon-separated messages. Once main.py is updated to use commas, add an assertion here to validate the comma separator format.
Add the following assertion after line 172 once main.py uses commas:
for phrase in expected_phrases: assert phrase in err + # Verify comma-separated format (not semicolons) + if len(expected_phrases) > 1: + assert ", ".join(expected_phrases) in err + assert "; ".join(expected_phrases) not in errThis will catch regressions if semicolons are reintroduced.
179-226: LGTM! Positive-path test correctly validates FreeBSD cross+container support.The test confirms that FreeBSD targets build successfully when both cross and a container runtime (Docker) are available. Mirrors existing test patterns and addresses PR objectives item 3 (positive-path test for FreeBSD targets). Assertions validate that cross was invoked and no errors were emitted.
228-267: LGTM! Test correctly validates CROSS_NO_DOCKER rejection for container-required targets.The test confirms that CROSS_NO_DOCKER=1 is rejected when FreeBSD targets require container-backed cross. Assertions verify the expected error message and correctly exercise the decision logic at main.py lines ~416–427.
269-314: LGTM! Test correctly validates container start failure handling.The test confirms that cross container start failure (exit code 125) for FreeBSD targets emits a clear error without falling back to cargo. Assertions verify the expected error message and exit code, and confirm no cargo fallback was attempted.
317-361: LGTM! Test correctly validates CROSS_CONTAINER_ENGINE export and cleanup.The test confirms that CROSS_CONTAINER_ENGINE is automatically exported when Docker is detected and cleaned up after the build completes. Assertions verify the env var was set to "docker" during cross invocation and removed afterwards.
364-408: LGTM! Test correctly validates Podman fallback for CROSS_CONTAINER_ENGINE.The test confirms that CROSS_CONTAINER_ENGINE is set to "podman" when Docker is unavailable and Podman is detected. Assertions verify the env var was set to "podman" during cross invocation and cleaned up afterwards.
411-455: LGTM! Test correctly validates preservation of pre-existing CROSS_CONTAINER_ENGINE.The test confirms that pre-existing CROSS_CONTAINER_ENGINE values are not overridden by automatic detection. Assertions verify the env var remained "custom" during cross invocation and was not cleaned up afterwards.
458-497: LGTM! Parametrised test provides comprehensive BSD suffix detection coverage.The test validates that BSD targets (FreeBSD, OpenBSD, NetBSD) require container-backed cross when the host suffix does not match. Parametrisation covers all three BSD variants with both matching and non-matching host scenarios, exercising the suffix detection logic at main.py lines ~295–298.
|
@coderabbitai Has the following now been resolved? ❌ Item 2: Error message simplificationNOT RESOLVED - The error message detail suffix still uses semicolons instead of commas. Line 432 shows: detail_suffix = f"; {'; '.join(details)}" if details else ""This produces messages like:
instead of the requested comma-separated format:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 627 @leynos: The change is partially resolved. Looking at line 432, the code now shows: detail_suffix = f"; {', '.join(details)}" if details else ""The items within the list are now comma-separated (
However, the leading separator is still a semicolon (the
Wait—I need to re-examine this. The leading
This produces: If you want the entire detail suffix to use commas consistently (including the leading separator), the code should be: detail_suffix = f", {', '.join(details)}" if details else ""Should the leading separator also be changed from semicolon to comma? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/actions/rust-build-release/src/main.py(9 hunks).github/actions/rust-build-release/tests/test_target_install.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
.github/actions/rust-build-release/tests/test_target_install.py (3)
.github/actions/rust-build-release/tests/conftest.py (7)
main_module(236-245)cross_module(223-232)module_harness(169-178)patch_shutil_which(134-136)patch_attr(142-144)patch_run_cmd(123-132)patch_platform(138-140)conftest.py (8)
cmd_mox(149-153)CmdMox(58-77)_register_rustup_toolchain_stub(115-121)_register_cross_version_stub(96-112)replay(71-73)verify(75-77)_register_docker_info_stub(124-131)_register_podman_info_stub(134-141).github/actions/rust-build-release/src/main.py (2)
main(390-505)_decide_cross_usage(284-361)
.github/actions/rust-build-release/src/main.py (2)
.github/actions/rust-build-release/src/runtime.py (1)
runtime_available(205-264).github/actions/rust-build-release/src/cross_manager.py (1)
ensure_cross(164-249)
⏰ 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). (5)
- GitHub Check: build-release (windows-latest, aarch64-pc-windows-gnullvm)
- GitHub Check: python-tests-windows
- GitHub Check: python-tests (macos-13)
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: Sourcery review
🔇 Additional comments (15)
.github/actions/rust-build-release/tests/test_target_install.py (8)
114-179: LGTM! Comprehensive parametrised test correctly validates error messaging.The test exercises all combinations of missing prerequisites (cross and container runtime) and verifies that:
- The error message includes the required substring "missing:"
- Expected diagnostic phrases appear in the output
- Multiple missing prerequisites are comma-separated
- Unexpected phrases are absent
The parametrisation ensures thorough coverage of the error paths.
181-228: LGTM! Positive-path test correctly validates FreeBSD builds with cross and container.This test resolves follow-up #3 from the PR objectives by verifying that FreeBSD targets build successfully when both cross and a container runtime (Docker) are available. The test:
- Patches the environment to simulate available prerequisites
- Records executed commands
- Validates cross was invoked
- Confirms no errors were emitted
230-269: LGTM! Test correctly rejects CROSS_NO_DOCKER=1 for container-required targets.The test validates that attempting to build a FreeBSD target with CROSS_NO_DOCKER=1 results in an error explaining that the flag is unsupported when a container runtime is required.
271-317: LGTM! Test correctly handles cross container start failures for required targets.The test validates that when cross fails to launch a container runtime for a FreeBSD target (exit code 125), the build:
- Exits with the same error code
- Emits a specific error message identifying the failure
- Does not fall back to cargo (since the target mandates container-backed cross)
319-364: LGTM! Test correctly validates CROSS_CONTAINER_ENGINE export for Docker.The test verifies that when Docker is available and cross is used for a FreeBSD build:
- CROSS_CONTAINER_ENGINE is automatically set to "docker" during the cross invocation
- The environment variable is cleaned up after the build completes
366-411: LGTM! Test correctly validates CROSS_CONTAINER_ENGINE export for Podman.The test verifies that when only Podman is available (Docker is not), CROSS_CONTAINER_ENGINE is set to "podman" during cross invocation and cleaned up afterward.
413-458: LGTM! Test correctly validates preservation of existing CROSS_CONTAINER_ENGINE.The test verifies that when CROSS_CONTAINER_ENGINE is already set in the environment, the build does not override it, even when a container runtime is detected. This respects user configuration.
460-500: LGTM! Comprehensive parametrised test validates BSD target detection logic.The test directly exercises
_decide_cross_usagewith all three BSD variants (FreeBSD, OpenBSD, NetBSD) and confirms:
- BSD targets require cross with containers when the host is a different OS (e.g., BSD target on Linux host)
- BSD targets do not require containers when the host matches the target OS (e.g., OpenBSD target on OpenBSD host)
The parametrisation ensures thorough coverage of the suffix-matching logic introduced in lines 296-299 of main.py.
.github/actions/rust-build-release/src/main.py (7)
21-21: LGTM! Imports required for FreeBSD cross-container support.The additions of
DEFAULT_HOST_TARGET(used at line 410) andCROSS_CONTAINER_ERROR_CODES(used at line 475) are necessary for the new container-backed cross build logic.
38-42: LGTM! BSD target suffixes correctly defined.The tuple covers all three major BSD variants (FreeBSD, OpenBSD, NetBSD) and follows the existing pattern of WINDOWS_TARGET_SUFFIXES. This enables container-requirement detection for all BSD targets.
84-85: LGTM! _CrossDecision fields correctly capture container runtime state.The additions of
container_engine(Docker/Podman/None) andrequires_cross_container(whether the target mandates container-backed cross) centralise the decision logic as requested in follow-up #1 from the PR objectives.
289-310: LGTM! BSD target detection and container engine selection correctly implemented.The changes resolve follow-up #1 from the PR objectives by centralising the container requirement logic in
_CrossDecision:
- The
host_targetparameter enables accurate cross-OS detection- Lines 293-299 correctly identify BSD targets requiring container-backed cross when the host OS differs
- Lines 306-310 select the container engine (preferring Docker over Podman)
- The computed values are propagated in the returned
_CrossDecisionat lines 359-360Also applies to: 359-360
410-424: LGTM! Host target initialisation and CROSS_NO_DOCKER rejection correctly implemented.The code correctly:
- Initialises
host_targettoDEFAULT_HOST_TARGETfor cross-OS detection- Passes
host_targetto_decide_cross_usageto enable BSD target detection- Rejects CROSS_NO_DOCKER=1 when building FreeBSD targets, which require container-backed cross
454-460: LGTM! CROSS_CONTAINER_ENGINE export and restoration correctly implemented.The code correctly manages the CROSS_CONTAINER_ENGINE environment variable:
- Lines 454-460 export the detected container engine (Docker or Podman) when cross is used with a container, but only if the variable is not already set
- Lines 503-505 restore the environment in a
finallyblock, ensuring cleanup even when the build fails- The logic respects pre-existing CROSS_CONTAINER_ENGINE values, as validated by test_preserves_existing_cross_container_engine
Also applies to: 503-505
476-486: LGTM! Container start failure handling correctly distinguishes required vs. optional cross.The code correctly handles cross container start failures:
- For targets that mandate container-backed cross (FreeBSD), the failure is fatal—no fallback to cargo occurs
- The error message at lines 481-484 identifies the target and container engine for debugging
- The original exit code (typically 125) is preserved at line 486
- For other targets, the existing fallback-to-cargo behaviour (lines 488-500) remains intact
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68db910147848322894bed2eac8fbe0f
Summary by Sourcery
Support FreeBSD cross-compilation by enforcing the use of a container-backed cross tool on non-FreeBSD hosts, including detection and clear error messaging, backed by tests, documentation updates, and CI configuration
New Features:
Enhancements:
CI:
Documentation:
Tests:
Chores: