ci: unbreak the three red jobs (windows runner pin, rolling base pin, renamed test binary) - #28
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCI settings update Windows dependency and test commands, upgrade Pixi tooling, disable matrix fail-fast, and pin the ROS 2 Rolling Ubuntu codename to ChangesCI compatibility
Possibly related issues
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dv-picknik
changed the title
…st binary) Three jobs were red. Two are external drift; the third had been masked by fail-fast and is our own fork-rename fallout. cmake Windows and Pixi (conda) both broke on the same runner-image change -- windows-latest is now windows-2025-vs2026, with Visual Studio 2026 (MSVC 19.51, tools 14.51) as the only Visual Studio installed -- but they broke on different things, so they get different fixes. Both stay on windows-latest. cmake Windows is not a generator problem. Conan already selects the "Visual Studio 18 2026" generator and gtest configures and builds clean under it. The break is libsodium, the one package in the graph built by msbuild rather than cmake: its hand-written .vcxproj gets PlatformToolset v145 from compiler.version=195 while conanvcvars lands on VCToolsVersion 14.44.35207, and msbuild rejects the pair (MSB8052). Passing -o "zeromq/*:encryption=tweetnacl" uses the copy of tweetnacl bundled in libzmq and drops the libsodium dependency entirely. CURVE support is unchanged, and nothing here touches it -- the ZMQ_CURVE_* references in src/loggers/zmq.hpp are cppzmq's own ifdef'd option wrappers. Pixi is a generator problem. cmake picks its default generator from a hardcoded list, and the conda-forge cmake 3.31.5 in the lock predates VS 2026 entirely, so it asked for "Visual Studio 17 2022" and found nothing. That generator exists only in cmake >= 4.2, hence the dependency floor and the win-64 build task that names it explicitly. Relocking to cmake 4.4.2 moves pixi.lock to format v7, which the pinned pixi v0.40.3 cannot read, so setup-pixi and pixi move up with it. ros2-rolling is unrelated: ROS 2 Rolling migrated to Ubuntu 26.04 (resolute), and packages.ros.org publishes no ros-rolling-* debs for resolute yet, so ros-rolling-ros-environment isn't there to install. Pin OS_CODE_NAME to noble until it is. Pixi (conda) on ubuntu was never actually passing -- fail-fast cancelled it mid-build once windows died, hiding its own failure. The rename in #26 changed PROJECT_NAME to behaviortree_cpp_picknik, so tests build behaviortree_cpp_picknik_test while pixi.toml and cmake_windows.yml still ran behaviortree_cpp_test. Fixed, and fail-fast disabled so one job's failure stops hiding its sibling's. cmake_ubuntu.yml needed nothing -- it runs ctest, not the binary by name. Verified locally where possible: industrial_ci in Docker with ROS_DISTRO= rolling ROS_REPO=main OS_CODE_NAME=noble builds and passes 207 tests, and pixi run build && pixi run test under cmake 4.4.2 builds clean and passes 207 tests, so the CMake 4 policy bump costs nothing (the one pre-4 declaration left is lexy's cmake_minimum_required(VERSION 3.8), which warns rather than errors). The windows halves are CI-testable only; all six jobs are green.
e522c43 to
7b39030
Compare
griswaldbrooks
left a comment
griswaldbrooks
left a comment
There was a problem hiding this comment.
i mean, if it works in ci...?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/cmake_windows.yml (1)
42-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the non-Pixi Windows generator explicit.
The Conan workaround assumes Visual Studio 18 2026, but the configure command at Line 54 still omits
-G. Without-G, CMake can useCMAKE_GENERATORor its internal default. A runner image change can select a generator that does not match the Conan toolchain. Pass-G "Visual Studio 18 2026"to Line 54 and verify CMake is at least 4.2. CMake added this generator in 4.2 and documents-Gas the explicit selector. (cmake.org)Proposed configure change
- run: cmake $GITHUB_WORKSPACE -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} -DCMAKE_TOOLCHAIN_FILE=conan_toolchain.cmake + run: cmake $GITHUB_WORKSPACE -G "Visual Studio 18 2026" -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} -DCMAKE_TOOLCHAIN_FILE=conan_toolchain.cmake🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cmake_windows.yml around lines 42 - 49, Update the Windows CMake configure command to pass the explicit generator option -G "Visual Studio 18 2026", matching the Conan workaround’s toolchain. Also ensure the workflow uses CMake version 4.2 or newer so this generator is supported..github/workflows/pixi.yaml (1)
13-15: 🚀 Performance & Scalability | 🔵 TrivialConfirm the runner-cost trade-off of disabling fail-fast.
fail-fast: falsekeeps the sibling matrix job running after one job fails. This supports independent diagnostics, but it can consume another runner and extend the workflow. Confirm that this cost is acceptable for the repository. GitHub documents that fail-fast cancellation applies to in-progress and queued matrix jobs. (docs.github.com)As per coding guidelines, CI/CD changes that can affect runner costs require a human review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pixi.yaml around lines 13 - 15, Confirm with a human reviewer that the increased runner usage and workflow duration from fail-fast: false are acceptable for this repository; retain the setting only after approving this CI cost trade-off.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/cmake_windows.yml:
- Around line 42-49: Update the Windows CMake configure command to pass the
explicit generator option -G "Visual Studio 18 2026", matching the Conan
workaround’s toolchain. Also ensure the workflow uses CMake version 4.2 or newer
so this generator is supported.
In @.github/workflows/pixi.yaml:
- Around line 13-15: Confirm with a human reviewer that the increased runner
usage and workflow duration from fail-fast: false are acceptable for this
repository; retain the setting only after approving this CI cost trade-off.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01a0a3a5-d705-4585-b0b7-88b494d0608f
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/cmake_windows.yml.github/workflows/pixi.yamlpixi.toml
2 participants
Gets this fork's CI matrix fully green. Two of the failures are external drift; digging in surfaced a third, previously-masked one that is our own fork-rename fallout.
What broke
cmake Windowswindows-latestis now thewindows-2025-vs2026image — Visual Studio 2026 only (MSVC 19.51, tools 14.51).conan profile detectpicks upcompiler.version=195, no prebuilt binaries match, so everything builds from source.libsodium/1.0.20(pulled in byzeromq) is the one package built by msbuild rather than cmake: its hand-written.vcxprojgetsPlatformToolset v145fromcompiler.version=195, whileconanvcvarslands onVCToolsVersion 14.44.35207, and msbuild rejects the pair.error MSB8052Pixi (conda)(windows)cmake 3.31.5inpixi.lockpredates VS 2026, so it asked forVisual Studio 17 2022and found nothing.could not find any instance of Visual Studioros2-rollingpackages.ros.orgpublishes zeroros-rolling-*debs for resolute so far, soros-rolling-ros-environmentisn't there to install.Pixi (conda)(ubuntu) — not in the issuePROJECT_NAMEtobehaviortree_cpp_picknik, sotests/CMakeLists.txtbuildsbehaviortree_cpp_picknik_test.pixi.tomlandcmake_windows.ymlstill ranbehaviortree_cpp_test.Three corrections to the issue's guesses, all found by reading the logs rather than trusting the summary:
cmake Windowsfailure is not a cmake generator problem either. Conan already selectsVisual Studio 18 2026andgtestconfigures and builds clean under it. Only the one msbuild-based package in the graph breaks.fail-fastkept cancelling it mid-build once windows died, so its own failure was invisible. It has been broken since the rename.The fix
Adapt to VS 2026 rather than pin away from it, then fix the rename fallout:
cmake_windows.yml:-o "zeromq/*:encryption=tweetnacl"onconan install. tweetnacl ships inside libzmq, so this drops thelibsodiumdependency — and with it the only msbuild-built package in the graph — while leaving CURVE support intact. Nothing in this repo touches CURVE; theZMQ_CURVE_*hits insrc/loggers/zmq.hppare cppzmq's own#ifdef'd option wrappers.pixi.toml:cmake = ">=4.2"(first version with theVisual Studio 18 2026generator) plus a win-64buildtask that names that generator explicitly. Relocking picks up cmake 4.4.2 and movespixi.lockto format v7.pixi.yaml:setup-pixi→v0.10.1,pixi-version→v0.76.1, because the pinnedv0.40.3cannot read a v7 lock. Alsofail-fast: false, so one job's failure stops hiding its sibling's.ros2-rolling.yaml: addOS_CODE_NAME: nobleto the matrix env.pixi.toml(both targets) +cmake_windows.ymltest step:behaviortree_cpp_test→behaviortree_cpp_picknik_test.Both windows runners stay on
windows-latest.cmake_ubuntu.ymlneeded nothing — it runsctest, not the binary by name.Testing
Everything testable on Linux was run locally; CI isolates the rest.
ROS_DISTRO=rolling ROS_REPO=main OS_CODE_NAME=noble ./.industrial_ci/ci.sh): builds,207 tests from 45 test suitespass,colcon test-resultreports208 tests, 0 errors, 0 failures. The run also emits ROS's ownRolling has migrated to Ubuntu 26.04warning — the drift confirmed from the source. Green on CI here too.pixi run build && pixi run testbuilds clean and passes 207 tests, so the CMake 4 policy bump costs nothing. The one pre-4 declaration left in the tree is lexy'scmake_minimum_required(VERSION 3.8), which warns rather than errors.windows-2022push already isolated the two failures to exactly the two causes above:build (windows-2022)got through conan, cmake and link and failed only on the stale test binary name, andpixi_conda_build (windows-2022)was mid-compile with no generator error. Both are addressed here without the pins.pre-commitclean; all workflow YAML parses.Not done
package-name: plotjugglerin both industrial_ci workflows is stale copy-paste, butpackage-nameisn't an inputindustrial_cideclares (not in itsaction.yml) — silently ignored, changes no behavior. Left alone.concurrencygroups,actions/checkout@v7, theconanfile.py/CMake-presets rewrite of the Windows job) — none of it is needed to go green, and each is extra merge surface against upstream.When these come off
encryption=tweetnaclcan go back to the default once conan-center publishes msvc 195 binaries, or once thelibsodiumrecipe stops handing msbuild a toolset/tools mismatch.OS_CODE_NAME: nobleoncepackages.ros.orgstarts publishing rolling debs for resolute.Closes PickNikRobotics/moveit_pro#21028
🤖 Generated with Claude Code
https://claude.ai/code/session_01SFjW9VieunSxySXNKSFEmX