[codex] Fix cross nightly toolchain resolution#255
Merged
Conversation
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Cross Nightly Toolchain Resolution FixThis PR removes the action’s practice of injecting a "+toolchain" CLI override into cross invocations to avoid conflicts with rustup’s environment-based resolution (RUSTUP_TOOLCHAIN=nightly-YYYY-MM-DD) and simplifies toolchain handling for cross builds. Root cause
Solution
Key code changes
Behavioural impact
Validation
Documentation / Execplans
WalkthroughRemove target-triple sanitisation and channel-derivation logic; stop injecting Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main (action)
participant Env as Environment
participant Cross as cross CLI
participant Rustup as rustup
Main->>Main: determine toolchain (repo / override)
alt explicit RBR_TOOLCHAIN provided
Main->>Env: set RUSTUP_TOOLCHAIN=<resolved_toolchain>
Main->>Cross: invoke `cross build` (no +toolchain arg)
Cross->>Rustup: consult RUSTUP_TOOLCHAIN from env
else no explicit override
Main->>Cross: invoke `cross build` (no +toolchain arg)
Cross->>Rustup: use default rustup toolchain
end
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideAlign cross build behavior with rustup toolchain resolution by removing cross-specific toolchain overrides and sanitization logic, so cross now relies on the repository or environment toolchain, with tests updated to enforce the new behavior. Sequence diagram for updated cross and cargo toolchain resolutionsequenceDiagram
actor Developer
participant GithubAction as rust_build_release_action
participant Decision as _decide_cross_usage
participant Cargo as cargo
participant Cross as cross
Developer->>GithubAction: Trigger rust-build-release
GithubAction->>Decision: _decide_cross_usage(toolchain_name, installed_names, rustup_exec, target, host_target)
Decision-->>GithubAction: _CrossDecision(cargo_toolchain_spec=+toolchain_name,
Decision-->>GithubAction: use_cross, cross_path, use_cross_local_backend)
alt use_cross is true
GithubAction->>Cross: cross build --manifest-path ... --target target
Note over Cross: No +toolchain spec passed
Cross->>Cross: Resolve toolchain from RUSTUP_TOOLCHAIN or rust-toolchain.toml
else use_cross is false
GithubAction->>Cargo: cargo +toolchain_name build --manifest-path ... --target target
end
Updated class diagram for _CrossDecision toolchain fieldsclassDiagram
class _CrossDecision {
str cross_path
str cross_version
bool use_cross
str cargo_toolchain_spec
bool use_cross_local_backend
bool docker_present
}
class ToolchainDecisionLogic {
+_decide_cross_usage(toolchain_name, installed_names, rustup_exec, target, host_target) _CrossDecision
+_build_cross_command(decision, manifest_path, target, features, release) void
}
ToolchainDecisionLogic ..> _CrossDecision : returns
class CargoInvocation {
+build_command: cargo cargo_toolchain_spec build ...
}
class CrossInvocation {
+build_command: cross build ...
}
ToolchainDecisionLogic ..> CargoInvocation : uses
ToolchainDecisionLogic ..> CrossInvocation : uses
class EnvironmentToolchainResolution {
+RUSTUP_TOOLCHAIN
+rust_toolchain_toml
}
CrossInvocation ..> EnvironmentToolchainResolution : resolves toolchain from
CargoInvocation ..> _CrossDecision : uses cargo_toolchain_spec
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 344ffe9e62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/rust-build-release/src/main.py:
- Around line 341-347: The function _decide_cross_usage currently includes
unused parameters _installed_names and _rustup_exec; remove those parameters
from _decide_cross_usage's signature and update the call site that invokes
_decide_cross_usage (the call around the noted call site) to stop passing those
two arguments, ensuring the function's type annotations and any callers are
adjusted accordingly; if this function is exported or used elsewhere search for
references to _decide_cross_usage and update them the same way to preserve
compatibility.
In @.github/actions/rust-build-release/tests/test_features.py:
- Around line 259-261: The duplicated assertion logic that checks cross commands
(parts[1] == "build" and all(not part.startswith("+") for part in parts[1:]))
should be extracted into a shared test helper (e.g., assert_cross_command(parts)
or validate_cross_command_parts) and used from both test_features.py and
test_manifest_path.py; implement the helper in a common test utilities module or
at the top of the test package, update the tests to call that helper instead of
repeating the two assertions, and ensure the helper name matches references so
test discovery and readability improve.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92dd719f-abea-471d-adab-94ab80fceffc
📒 Files selected for processing (6)
.github/actions/rust-build-release/src/main.py.github/actions/rust-build-release/tests/conftest.py.github/actions/rust-build-release/tests/test_features.py.github/actions/rust-build-release/tests/test_manifest_path.py.github/actions/rust-build-release/tests/test_target_install.py.github/actions/rust-build-release/tests/test_toolchain_sanitize.py
💤 Files with no reviewable changes (2)
- .github/actions/rust-build-release/tests/test_target_install.py
- .github/actions/rust-build-release/tests/conftest.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
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/src/main.py (1)
625-635: 🧹 Nitpick | 🔵 TrivialCollapse
_resolve_toolchain()to a scalar return.Drop the dead list return. Line 625 now discards the second value immediately, and
_decide_cross_usage()no longer consumes any installed-toolchain state. Shrink the private helper contract tostrand remove the tuple plumbing from the test stubs as well.♻️ Proposed change
- toolchain_name, _installed_names = _resolve_toolchain( + toolchain_name = _resolve_toolchain( rustup_exec, requested_toolchain, target_to_build )# Then update _resolve_toolchain() to return only `toolchain_name` # and simplify the patched return values in the tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/rust-build-release/src/main.py around lines 625 - 635, The call to _resolve_toolchain currently unpacks a dead second value; change _resolve_toolchain to return only the toolchain_name (str) and update its call sites (e.g., replace "toolchain_name, _installed_names = _resolve_toolchain(...)" with "toolchain_name = _resolve_toolchain(...)") and any other unpacking uses, adjust _decide_cross_usage callers if they expected the removed value, and update test stubs/mocks to return a simple string instead of a tuple so the function signature and all call sites consistently use a single str return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/actions/rust-build-release/src/main.py:
- Around line 625-635: The call to _resolve_toolchain currently unpacks a dead
second value; change _resolve_toolchain to return only the toolchain_name (str)
and update its call sites (e.g., replace "toolchain_name, _installed_names =
_resolve_toolchain(...)" with "toolchain_name = _resolve_toolchain(...)") and
any other unpacking uses, adjust _decide_cross_usage callers if they expected
the removed value, and update test stubs/mocks to return a simple string instead
of a tuple so the function signature and all call sites consistently use a
single str return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bb69a98-b081-4bec-a3bd-c5df1904fd67
📒 Files selected for processing (5)
.github/actions/rust-build-release/src/main.py.github/actions/rust-build-release/tests/conftest.py.github/actions/rust-build-release/tests/test_features.py.github/actions/rust-build-release/tests/test_manifest_path.py.github/actions/rust-build-release/tests/test_target_install.py
💤 Files with no reviewable changes (1)
- .github/actions/rust-build-release/tests/test_target_install.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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_target_install.py (1)
74-79:⚠️ Potential issue | 🟠 MajorUpdate this assertion to the new cross contract.
Stop asserting a
+toolchainargv fragment here. The implementation no longer injects one forcross, so this test is still pinned to the pre-PR command shape and no longer validates the regression that this change is meant to fix. Assertbuildas the subcommand, assert that no later arg starts with+, and recordRUSTUP_TOOLCHAINfrom the command wrapper’s environment instead.🧪 Proposed fix
- app_env.patch_run_cmd(run_cmd_side_effect) + captured: dict[str, object] = {} + + def fake_run(cmd: object) -> None: + parts = list(cmd.formulate()) + run_cmd_side_effect(parts) + captured["parts"] = parts + captured["env"] = getattr(cmd, "env", {}) + + app_env.patch_attr("run_cmd", fake_run) @@ - build_cmd = app_env.calls[-1] + build_cmd = typ.cast("list[str]", captured["parts"]) assert Path(build_cmd[0]).name == "cross" - assert build_cmd[1] == f"+{default_toolchain}" + assert build_cmd[1] == "build" + assert all(not part.startswith("+") for part in build_cmd[1:]) + assert captured["env"] == {"RUSTUP_TOOLCHAIN": default_toolchain}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/rust-build-release/tests/test_target_install.py around lines 74 - 79, The test currently asserts a "+toolchain" argv fragment; update it to match the new cross contract by replacing the app_env.patch_run_cmd usage with a fake_run that captures command parts and env (use names from the diff: fake_run, captured, run_cmd_side_effect), call main_module.main("aarch64-pc-windows-gnu", default_toolchain) as before, then set build_cmd = captured["parts"] and assert Path(build_cmd[0]).name == "cross", assert build_cmd[1] == "build", assert all(not part.startswith("+") for part in build_cmd[1:]), and assert captured["env"] == {"RUSTUP_TOOLCHAIN": default_toolchain}; remove the old assertion that build_cmd[1] == f"+{default_toolchain}".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/rust-build-release/src/main.py:
- Around line 666-669: The cross environment variable uses the raw user input
`explicit_toolchain` instead of the canonicalised `toolchain_name` returned by
`_resolve_toolchain()` (which itself uses `_resolve_toolchain_name()` and
`_fallback_toolchain_name()`); update the branch where `explicit_toolchain` is
applied so that `build_cmd.with_env(RUSTUP_TOOLCHAIN=...)` uses `toolchain_name`
instead, ensuring `build_cmd` (the `_SupportsEnvFormulate` object) receives the
resolved toolchain that matches cargo and target installation paths and avoids
unrecognised identifiers when fallback resolution was used.
In @.github/actions/rust-build-release/tests/test_manifest_path.py:
- Around line 14-15: The test imports the helper directly as "from conftest
import assert_no_toolchain_override", which relies on pytest path injection and
is brittle; change the import to a package-relative import (e.g., "from
.conftest import assert_no_toolchain_override") or move the helper into a proper
test utility module and import it (e.g., from tests.utils import
assert_no_toolchain_override), and update the test_manifest_path.py import to
use that package-relative or utility-module import so it no longer depends on
top-level conftest resolution.
---
Outside diff comments:
In @.github/actions/rust-build-release/tests/test_target_install.py:
- Around line 74-79: The test currently asserts a "+toolchain" argv fragment;
update it to match the new cross contract by replacing the app_env.patch_run_cmd
usage with a fake_run that captures command parts and env (use names from the
diff: fake_run, captured, run_cmd_side_effect), call
main_module.main("aarch64-pc-windows-gnu", default_toolchain) as before, then
set build_cmd = captured["parts"] and assert Path(build_cmd[0]).name == "cross",
assert build_cmd[1] == "build", assert all(not part.startswith("+") for part in
build_cmd[1:]), and assert captured["env"] == {"RUSTUP_TOOLCHAIN":
default_toolchain}; remove the old assertion that build_cmd[1] ==
f"+{default_toolchain}".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 075ee775-0967-4348-a003-56ed598d2298
📒 Files selected for processing (4)
.github/actions/rust-build-release/src/main.py.github/actions/rust-build-release/tests/conftest.py.github/actions/rust-build-release/tests/test_manifest_path.py.github/actions/rust-build-release/tests/test_target_install.py
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 609
Comment 1 — Remove unused parameters from decision = _decide_cross_usage(toolchain_name, target_to_build, host_target)Comment 2 — Extract shared helper for cross command shape assertion: |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplication.github/actions/rust-build-release/tests/test_manifest_path.py: What lead to degradation?The module contains 2 functions with similar structure: test_main_does_not_export_cross_toolchain_without_explicit_override,test_main_exports_explicit_toolchain_to_cross_command Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/actions/rust-build-release/tests/test_manifest_path.py (1)
329-393: 🧹 Nitpick | 🔵 TrivialCollapse these two cross toolchain-env tests into one parametrized case.
Merge this pair into a single
@pytest.mark.parametrizetest. Both functions patch the same attributes, capture the same fields, and assert the same command invariant; only the explicit input and expected env differ, so keeping them separate invites drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/rust-build-release/tests/test_manifest_path.py around lines 329 - 393, Collapse the two tests test_main_exports_explicit_toolchain_to_cross_command and test_main_does_not_export_cross_toolchain_without_explicit_override into one pytest.mark.parametrize test that reuses the same setup (harness.patch_attr, captured dict, decision = context.cross_decision_factory, fake_run patch of run_cmd, and assert_no_toolchain_override on parts) and only varies the inputs and expected env: parameterize the call to context.main_module.main (pass toolchain="nightly-2026-03-26" for the explicit case and omit/None for the ambient case) and the expected captured["env"] ({"RUSTUP_TOOLCHAIN": "nightly-2026-03-26-x86_64-unknown-linux-gnu"} vs {}). Keep the same assertions on parts and use descriptive param ids; remove the original two test functions and ensure the new test still patches _resolve_target_argument, _resolve_toolchain/resolve_requested_toolchain, and _decide_cross_usage as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/rust-build-release/src/main.py:
- Around line 625-631: The code currently strips whitespace into
explicit_toolchain but then calls resolve_requested_toolchain(...) with the
original toolchain variable, allowing a whitespace-only override to bypass
fallback; change the resolve_requested_toolchain call to pass explicit_toolchain
instead of toolchain so the normalized (stripped) value is used when computing
requested_toolchain (update the call site that constructs requested_toolchain
which references resolve_requested_toolchain, keeping project_dir, manifest_path
and fallback DEFAULT_TOOLCHAIN the same).
In @.github/actions/rust-build-release/tests/conftest.py:
- Around line 209-213: The with_env method currently discards positional args
(args) and only merges kwargs into the wrapped _DummyCommand, which narrows the
API compared to the real command; update with_env to accept and parse
mapping-style positional environment arguments from *args (e.g., dict or Mapping
instances), merge them in order before applying **kwargs into wrapped.env, and
ensure wrapped.env = {**self.env, **merged_from_args, **kwargs}; also validate
or raise if any arg is not a mapping so _DummyCommand's with_env contract
mirrors the production wrapper.
In @.github/actions/rust-build-release/tests/test_target_install.py:
- Around line 83-85: Replace the two open-coded assertions about the build
command with the shared helper: call assert_no_toolchain_override(build_cmd)
instead of asserting build_cmd[1] == "build" and checking parts for '+'; keep
the existing env check (assert captured["env"] == {"RUSTUP_TOOLCHAIN":
default_toolchain}) as-is. This consolidates the command-shape invariant into
the helper (referencing build_cmd, default_toolchain, and the helper
assert_no_toolchain_override).
---
Duplicate comments:
In @.github/actions/rust-build-release/tests/test_manifest_path.py:
- Around line 329-393: Collapse the two tests
test_main_exports_explicit_toolchain_to_cross_command and
test_main_does_not_export_cross_toolchain_without_explicit_override into one
pytest.mark.parametrize test that reuses the same setup (harness.patch_attr,
captured dict, decision = context.cross_decision_factory, fake_run patch of
run_cmd, and assert_no_toolchain_override on parts) and only varies the inputs
and expected env: parameterize the call to context.main_module.main (pass
toolchain="nightly-2026-03-26" for the explicit case and omit/None for the
ambient case) and the expected captured["env"] ({"RUSTUP_TOOLCHAIN":
"nightly-2026-03-26-x86_64-unknown-linux-gnu"} vs {}). Keep the same assertions
on parts and use descriptive param ids; remove the original two test functions
and ensure the new test still patches _resolve_target_argument,
_resolve_toolchain/resolve_requested_toolchain, and _decide_cross_usage as
before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6576ab4-2a82-4d0d-a65e-6232ffd3384a
📒 Files selected for processing (6)
.github/actions/rust-build-release/src/main.py.github/actions/rust-build-release/tests/conftest.py.github/actions/rust-build-release/tests/rust_build_release_test_helpers.py.github/actions/rust-build-release/tests/test_features.py.github/actions/rust-build-release/tests/test_manifest_path.py.github/actions/rust-build-release/tests/test_target_install.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/rust-build-release/tests/test_manifest_path.py:
- Around line 346-385: Add a test case exercising main() with a repo-declared
nightly manifest by adding a NIGHTLY_CRANELIFT_PROJECT / dated-nightly scenario
to test_main_cross_toolchain_environment (or a small companion test) where
explicit_toolchain is None: arrange resolved_toolchain to represent the nightly
manifest, ensure setup_manifest provides the dated-nightly path, call
context.main_module.main("aarch64-unknown-linux-gnu") (no toolchain arg), and
assert that captured["env"] contains the expected NIGHTLY_CRANELIFT_PROJECT (and
still no toolchain override) — this ensures the dated-nightly branch exercised
by _build_cross_command() is covered end-to-end by main().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10ee12a8-fe1e-4ade-a5db-667499f2e23d
📒 Files selected for processing (1)
.github/actions/rust-build-release/tests/test_manifest_path.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/actions/rust-build-release/tests/test_manifest_path.py (1)
13-13:⚠️ Potential issue | 🟡 MinorSwitch this helper import to a package-relative path.
Replace the absolute import on Line 13 with
from .rust_build_release_test_helpers import assert_no_toolchain_overrideto avoid accidental module shadowing fromsys.pathprecedence.#!/bin/bash set -euo pipefail # Verify whether helper module names can collide and where this import style is used. fd -a 'rust_build_release_test_helpers.py$' . rg -n 'from rust_build_release_test_helpers import|from \.rust_build_release_test_helpers import' .github/actions/rust-build-release/tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/rust-build-release/tests/test_manifest_path.py at line 13, Replace the top-level import of the test helper with a package-relative import so the local helper module can't be shadowed; change the import of rust_build_release_test_helpers to a relative import (referencing the module rust_build_release_test_helpers and the symbol assert_no_toolchain_override) by using from .rust_build_release_test_helpers import assert_no_toolchain_override in the test file that imports assert_no_toolchain_override.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/actions/rust-build-release/tests/test_manifest_path.py:
- Line 13: Replace the top-level import of the test helper with a
package-relative import so the local helper module can't be shadowed; change the
import of rust_build_release_test_helpers to a relative import (referencing the
module rust_build_release_test_helpers and the symbol
assert_no_toolchain_override) by using from .rust_build_release_test_helpers
import assert_no_toolchain_override in the test file that imports
assert_no_toolchain_override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 266f3c01-57f3-4009-8392-2064c7f8b71b
📒 Files selected for processing (1)
.github/actions/rust-build-release/tests/test_manifest_path.py
Owner
Author
|
@coderabbitai pause |
Contributor
✅ Actions performedReviews paused. |
Owner
Author
|
@coderabbitai please create a new GitHub issue for the following: Verified this finding against the current code and did not change it.
I also confirmed the broader issue: importing this test module directly outside pytest already fails with |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 51
It captures the fragile bare-import pattern, the root cause (no |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
+toolchainoverride fromcross buildinrust-build-release+...argumentWhy
When a target repository already pins a dated nightly in
rust-toolchain.toml, rustup exportsRUSTUP_TOOLCHAIN=nightly-YYYY-MM-DD. Passingcross +nightly-YYYY-MM-DD build ...at the same time causes cross/rustup to combine both sources into an invalid toolchain name with the date repeated, which rustup rejects.Impact
crossnow resolves its toolchain solely fromRUSTUP_TOOLCHAINor the repository'srust-toolchain.toml, while cargo keeps its existing explicit toolchain override behavior.Validation
make check-fmtmake typecheckmake lintmake testrust-build-releasepytest run covering the command shape regressionSummary by Sourcery
Rely on the repository’s or rustup’s toolchain resolution for cross builds instead of injecting an explicit toolchain override, and adjust tests accordingly.
Bug Fixes:
Enhancements:
Tests: