ci: restore blocking Rust package checks by espressolee · Pull Request #684 · ros2-rust/ros2_rust

espressolee · 2026-09-03T16:23:51Z

Fixes #682.

Since colcon-ros-cargo #32 (December 2024), colcon list has reported Rust packages as (ros.ament_cargo). The workflows still filtered for (ament_cargo), so their per-package loops selected zero packages and passed without running their checks.

This PR restores those checks as blocking checks:

selects ros.ament_cargo packages and fails explicitly if the selector returns no target packages;
computes each package-path list once and reuses it;
limits Clippy and rustdoc to packages owned by this repository with --packages-select, rather than linting source imported from separate repositories by the .repos file;
sources the built install/setup.sh overlay so generated ROS message crates are available to Clippy and rustdoc;
keeps action-ros-ci as the Linux build/test runner instead of running the same tests twice;
propagates individual cargo test failures in the Windows batch loop; and
excludes the generated ros_ws/ directory from the root Cargo workspace.

The restored Clippy step exposed the existing rclrs lint backlog. This PR fixes that backlog in a separate automated-fix commit followed by reviewed manual fixes. It also repairs the broken intra-doc links exposed by the restored rustdoc step. The Rolling matrix entry remains allowed to fail at the job level because current Rolling is broken by the upstream change discussed in #658; all non-Rolling matrix entries remain blocking.

Local validation on Ubuntu 22.04 aarch64 with ROS 2 Humble:

selector fixtures: new selector 1 target; old selector 0 targets; repository target selection 1; dependency closure 2;
actionlint 1.7.7: all three edited workflows pass;
nightly rustfmt 1.10.0: 1/1 target package passes;
Rust 1.85.0 and Stable 1.98.1: blocking Clippy and rustdoc both pass for the selected target package;
Rust 1.85.0: 120/120 unit tests and 51/51 doc tests pass;
Stable 1.98.1: 120/120 unit tests and 51/51 doc tests pass on the final successful run; and
colcon build: 13/13 packages pass.

One local parallel Stable run intermittently lost /rosout messages while the test capture handler was active. That race is fixed on current main by #656. A narrower mutex workaround was tested and then removed in a follow-up commit so this PR does not duplicate the upstream fix. Merge-commit CI against current main is therefore the authority for the final cross-distro result.

Assisted-by: Claude Code:claude-fable-5-1

Assisted-by: Codex:gpt-6 cargo-clippy:1.85.0 cargo-clippy:1.98.1 actionlint:1.7.7

Since colcon-ros-cargo ros2-rust#32, colcon lists Rust packages as (ros.ament_cargo). The loops still filtered on (ament_cargo), so the fmt, clippy, test and rustdoc steps selected no packages and passed without running. Update the nine filters and the docs/building.md example. Fixes ros2-rust#682. Assisted-by: Claude Code:claude-fable-5-1

With the fmt loop running again, `cargo +nightly fmt -- --check` stops on rclrs at two import groups. Apply exactly what rustfmt printed so the step can pass and the later steps get to run. Assisted-by: Claude Code:claude-fable-5-1

action-ros-ci creates ros_ws/ inside the checkout, so once the per-package loops run, cargo finds this manifest above every package and stops with "current package believes it's in a workspace when it's not" before any lint runs. The root [workspace] was added in ros2-rust#502, after the loops had already stopped iterating, so CI never showed this. Excluding ros_ws makes the nested packages standalone again. Assisted-by: Claude Code:claude-fable-5-1

The workflow passes the checkout-root package list (rclrs) to action-ros-ci@v0.4, which builds its --packages-up-to closure: rclrs and rosidl_runtime_rs. The per-package loops instead traversed every ament_cargo package in the completed workspace, including the example packages from ros2-rust/examples whose generated message crates are never built here; the first live run died in examples_rclrs_message_demo with 21 E0433s. Select the same closure the build step used. Assisted-by: Claude Code:claude-fable-5-1

espressolee · 2026-09-03T17:52:41Z

The first CI run exposed three follow-up issues:

fmt reported formatting differences in two rclrs import groups, so I applied rustfmt’s output.
clippy failed before reaching any lints. action-ros-ci creates ros_ws/ inside the repository checkout, which places its packages beneath the root Cargo.toml workspace added in build: add release-plz for automating releases #502. Because those packages are not workspace members, Cargo aborts with current package believes it's in a workspace when it's not. The same issue affected the test and rustdoc steps.

I added exclude = ["ros_ws"] to the root workspace to keep the generated ROS workspace outside it.
Once the loops actually ran, clippy also picked up packages from ros2-rust/examples, even though this job’s build step does not include them. This caused message_demo to fail with E0433.

I updated the fmt, clippy, test, and rustdoc loops to use the same --packages-up-to closure as the build step, so all of these checks operate on the same package set.

With these fixes in place, the next CI run should reach the actual rclrs clippy backlog.

With the selector fixed these three per-package loops run for the first time since 2024-12 and surface a pre-existing rclrs backlog (about 122 clippy diagnostics, plus rustdoc link errors). Mark them continue-on-error so this selector fix can land green while the backlog is paid down in follow-ups, as suggested in ros2-rust#682. fmt stays blocking: it had no backlog and is already clean. Assisted-by: Claude Code:claude-fable-5-1

espressolee · 2026-09-05T09:46:53Z

The two remaining failures appear to be a Rolling-only flake in in the existing rclrs test run.

At exact base 60e829b5, Rust Stable and Rust Minimal both reached the 120-test rclrs binary and terminated with SIGSEGV on September 3; later Rolling checks on that same base commit passed. At this PR head, 65ec9ec2, Stable terminates with SIGSEGV and Minimal with SIGILL at the same stage. Humble, Jazzy, Kilted, and, and Windows pass. The only Rust source changes in this PR are rustfmt output.

The corrected selectors are working: the logs show rclrs, and rosidl_runtime_rs where applicable, actually being selected and run. I cannot rerun the failed jobs without repository admin rights.

Could someone with write access rerun the two Rolling jobs? I don't think a code change is justified before seeing this exact head rerun.

azerupi

Thank you for the PR!

My personal opinion is that we shouldn't merge this with continue-on-error: true. We should take the time in this PR to fix all of the clippy lints that make CI fail and land this together. Merging this with continue-on-error: true would now run the steps but hide the failures, this would be only marginally better than now.

We / I can help you get this complete if you need assistance. Some clippy lints can probably automatically be fixed by clippy itself with cargo clippy --fix
. To make review easier, make sure you commit automated fix passes like this separately.

@esteve @maspe36 what are your thoughts on this?

@espressolee about rolling CI, it is broken in main because of an upstream change see #658 (comment). You will not get rolling CI to pass.

azerupi · 2026-09-05T11:31:19Z

@@ -76,7 +76,7 @@ jobs:
          call C:\pixi_ws\ros2-windows\setup.bat
          cd C:\workspace
          for /f "tokens=1,2,3" %%A in ('pixi run --manifest-path C:\pixi_ws\pixi.toml colcon list') do (


From the logs, cargo test fails but the step still succeeds and the failure is silently ignored

Fixed in 71537a4: the Windows batch loop now checks errorlevel immediately after each cargo test, returns a failing step on any package failure, and also fails if the selector matches zero target packages.

Fixed in : the Windows batch loop now checks immediately after each , returns a failing step on any package failure, and also fails if the selector matches zero target packages.

azerupi · 2026-09-05T11:45:16Z

@@ -124,10 +125,11 @@ jobs:
        done

    - name: Run cargo test on Rust packages


I believe the step "Build and test" lines 102-108 already run the tests. At least tests were being run while this was broken. This means that now we are running tests twice. We should check which one we want to keep and avoid running tests twice.

Agreed. I removed the direct Linux cargo test loops. action-ros-ci remains the single Linux build/test runner; the separate per-package steps now cover only Clippy and rustdoc.

azerupi · 2026-09-05T11:46:06Z

        cd ${{ steps.build.outputs.ros-workspace-directory-name }}
        . /opt/ros/${{ matrix.ros_distribution }}/setup.sh
-        for path in $(colcon list | awk '$3 == "(ament_cargo)" { print $2 }'); do
+        for path in $(colcon list --packages-up-to $(echo "${{ steps.list_packages.outputs.package_list }}") | awk '$3 == "(ros.ament_cargo)" { print $2 }'); do


This is being repeated multiple times, is it worth extracting into a step that adds the list of packages in an output that other steps can reuse?

Done in 71537a4. Repository and post-build Rust package paths are each computed once into step outputs and reused. Both selector steps fail explicitly if they produce an empty target set.

I prefer readability over reducing repetition if the solution is too complex or not easy to read, it's only a handful of packages.

Apply the meaning-preserving suggestions from cargo clippy --fix while retaining explicit lifetimes required by the field-access macro. Assisted-by: Codex:gpt-6 cargo-clippy:1.98.1

Clarify discarded promises, factor callback types, make function-pointer equality explicit, scope distro-specific dead code, and repair broken public documentation links. Keep existing public Result shapes where changing them would be an API break. Assisted-by: Codex:gpt-6 cargo-clippy:1.85.0 cargo-clippy:1.98.1

The logging macro test replaces the process-wide rcutils output handler, which could race with the rosout tests and make them receive no messages. Hold one test mutex across all three global-handler tests. Assisted-by: Codex:gpt-6 cargo-clippy:1.85.0 cargo-clippy:1.98.1

Select only ros.ament_cargo targets owned by this repository, fail when selection is empty, reuse the selected path list, and source the built overlay before clippy and rustdoc. Keep action-ros-ci as the Linux test runner and make Windows cargo-test failures propagate. Assisted-by: Codex:gpt-6 actionlint:1.7.7

Upstream ros2-rust#656 now preserves rosout while the test capture handler is installed and isolates captured records by thread. Remove the narrower mutex workaround so this PR does not duplicate that fix. Assisted-by: Codex:gpt-6

espressolee · 2026-09-06T06:45:13Z

Thanks for the detailed review. I reworked the PR around the blocking checks rather than keeping the step-level continue-on-error flags.

9a5cdda is the separate cargo clippy --fix pass.
d3b3144 resolves the remaining Clippy diagnostics under both Rust 1.85.0 and Stable 1.98.1, and fixes the rustdoc link failures.
71537a4 removes the duplicate Linux cargo test loops, keeps action-ros-ci as the Linux test runner, sources the built overlay for Clippy/rustdoc, computes package paths once, fails on an empty selector, and makes Windows cargo test failures propagate.
The post-build checks use --packages-select for this repository's target packages. Using the dependency closure also linted the separately imported rosidl_runtime_rs repository, whose current main has an unrelated unnecessary_cast lint. That external source is still built/tested by action-ros-ci; it is not silently allowed in this PR.

The only remaining continue-on-error is the matrix-level expression for Rolling, following the known upstream breakage you pointed out. All non-Rolling matrix entries remain blocking.

Local Humble validation is recorded in the updated PR description. In particular, both Rust toolchains pass blocking Clippy and rustdoc; Rust 1.85.0 and Stable each completed 120/120 unit tests and 51/51 doc tests. actionlint passes all three edited workflows. The selector fixture gives 1 target for (ros.ament_cargo) and 0 for the old (ament_cargo) spelling.

Local stress also reproduced the /rosout race fixed on current main by #656. I briefly tested a mutex workaround, then fully reverted it in a2948d1 so the final diff does not duplicate #656. The new CI run will provide the merge-commit result against current main.

Container jobs otherwise default to sh, which rejects the mapfile and process-substitution syntax used by the fail-closed package selection steps. Assisted-by: Codex:gpt-6 actionlint:1.7.7

esteve · 2026-09-06T06:50:52Z


+defaults:
+  run:
+    shell: bash


esteve · 2026-09-06T06:57:28Z

@espressolee please run the tests locally before pushing any more commits.

Compare C characters after lossless promotion to i32 so the code compiles when c_char is unsigned on aarch64 and signed on x86_64. Assisted-by: Codex:gpt-6 cargo-clippy:1.85.0 cargo-clippy:1.98.1

espressolee · 2026-09-06T07:08:57Z

You're right. I did run the Humble checks locally on aarch64 (Rust 1.85 and Stable Clippy/rustdoc, 120 unit tests, 51 doc tests, and colcon build/test), but that did not cover GitHub's x86_64 c_char or the container job's default shell. I should not have treated that as sufficient coverage.

I will not push any more commits until I have reproduced the current PR merge against main in an x86_64 container and run the workflow checks locally. Sorry for the churn.

esteve · 2026-09-06T07:14:01Z

@espressolee are we talking to a person or to an AI agent? Just don't be a meat proxy, you're putting the burden of AI-generated code and issues on humans without even checking what you're submitting.

espressolee · 2026-09-06T07:39:31Z

Local x86_64 results, with no further commits pushed after your request:

I reconstructed the PR merge against current main. Its tree is c443a9b8ad3f2b2492c1af1276388f85b41ab628, which matches the tree of GitHub's merge commit 6dc9629 exactly.
Environment: linux/amd64, Ubuntu 22.04, ROS 2 Humble, Rust 1.85.0 and Stable 1.98.1.
colcon build --packages-up-to rclrs: 13/13 packages passed.
Blocking Clippy and rustdoc: passed with both Rust 1.85.0 and Stable 1.98.1.
Parallel rclrs unit tests on the PR merge: 0/5 passed; all five processes ended with SIGSEGV.
The same parallel test command on current main (2766eab) in the same container setup: 0/5 passed with the same SIGSEGV.
PR merge with --test-threads=1: 120/120 passed.
test_logging_macros alone passed, the three logging tests passed 10/10, and excluding test_logging_macros still crashed in 4/5 runs.

So the native parallel crash is reproducible on current main and is not introduced by #684. I am keeping the PR at remote head f4044d5 while I finish separating that base failure from the checks this PR restores.

espressolee · 2026-09-06T07:40:48Z

This is my account, and I am using Codex as an AI coding assistant. Some of the code and text are AI-assisted, as disclosed in the commit messages and PR description. I am responsible for what was pushed.

Your criticism is fair. I allowed iterative generated fixes to reach CI before adequate x86_64 local validation, which shifted review and debugging burden onto maintainers. I have stopped pushing and will not ask you to diagnose it. I will review and validate the work offline; if I cannot reduce it to one minimal, locally verified update, I will close the PR.

espressolee added 4 commits September 4, 2026 00:23

style: apply rustfmt to the two rclrs files it flags

a86ee30

With the fmt loop running again, `cargo +nightly fmt -- --check` stops on rclrs at two import groups. Apply exactly what rustfmt printed so the step can pass and the later steps get to run. Assisted-by: Claude Code:claude-fable-5-1

azerupi reviewed Sep 5, 2026

View reviewed changes

espressolee added 5 commits September 6, 2026 14:03

Fix auto-fixable clippy warnings

9a5cdda

Apply the meaning-preserving suggestions from cargo clippy --fix while retaining explicit lifetimes required by the field-access macro. Assisted-by: Codex:gpt-6 cargo-clippy:1.98.1

Drop logging lock superseded by upstream fix

a2948d1

Upstream ros2-rust#656 now preserves rosout while the test capture handler is installed and isolates captured records by thread. Remove the narrower mutex workaround so this PR does not duplicate that fix. Assisted-by: Codex:gpt-6

espressolee changed the title ~~ci: match colcon's (ros.ament_cargo) type in the per-package loops~~ ci: restore blocking Rust package checks Sep 6, 2026

Run Linux workflow scripts with Bash

0ecc15c

Container jobs otherwise default to sh, which rejects the mapfile and process-substitution syntax used by the fail-closed package selection steps. Assisted-by: Codex:gpt-6 actionlint:1.7.7

esteve requested changes Sep 6, 2026

View reviewed changes

Comment thread .github/workflows/rust-minimal.yml

defaults:

run:

shell: bash

esteve Sep 6, 2026

Copy link
Copy Markdown

Collaborator

Choose a reason for hiding this comment

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

Why?

Make parameter separator comparison portable

f4044d5

Compare C characters after lossless promotion to i32 so the code compiles when c_char is unsigned on aarch64 and signed on x86_64. Assisted-by: Codex:gpt-6 cargo-clippy:1.85.0 cargo-clippy:1.98.1

		@@ -124,10 +125,11 @@ jobs:
		done

		- name: Run cargo test on Rust packages

Uh oh!

Conversation

espressolee commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

espressolee commented Sep 3, 2026

Uh oh!

espressolee commented Sep 5, 2026

Uh oh!

azerupi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

espressolee commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esteve commented Sep 6, 2026

Uh oh!

espressolee commented Sep 6, 2026

Uh oh!

esteve commented Sep 6, 2026

Uh oh!

espressolee commented Sep 6, 2026

Uh oh!

espressolee commented Sep 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

espressolee commented Sep 3, 2026 •

edited

Loading

azerupi left a comment •

edited

Loading

espressolee commented Sep 6, 2026 •

edited

Loading