Uh oh!
There was an error while loading. Please reload this page.
fix: close if-block before starting a new one in sort_vinca_lists - #152
Merged
traversaro merged 1 commit intoSep 6, 2026
Conversation
Two "- if:" blocks placed back-to-back with no blank line or comment between them were parsed as a single block, since the "then:" handler never resets in_then between blocks. Sorting then pooled both blocks' then-items into one flat list and redistributed them by original line position, which can move an item across a platform-condition boundary into the wrong block. Reproduced downstream in RoboStack/ros-rolling: adding entries to a "not wasm32 and not win" block right before an adjacent, comment-less "linux and not aarch64" block caused webots_ros2 (linux-only) to swap places with an item that should have stayed unconditional. Close the current if-block whenever a new "- if:" line is seen, the same way a list-level comment already does, so each block sorts within its own scope regardless of adjacency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tobias-Fischer added a commit
to RoboStack/ros-rolling
that referenced
this pull request
Sep 5, 2026
…em_modes_msgs seed rosgraph_monitor, rviz_2d_overlay_plugins, sick_safetyscanners_base, spacenav, system_modes, trac_ik, and zed_msgs were all gated under if: linux (or, for zed_msgs, linux and not aarch64) in this repo's vinca.yaml despite jazzy already building every one of them on osx (its own vinca.yaml selects them under "not win"/"not linux"). Moved them into the not-wasm32-and-not-win block instead of duplicating them, and added a comment separator so vinca-sort-vinca-lists' known adjacent-if-block-merging bug doesn't scramble the boundary on the next sort (see build_feedback.md's vinca-sort-vinca-lists entry, and RoboStack/vinca#152 for the upstream fix). Also added system_modes_msgs as an explicit seed -- system_modes depends on it but it wasn't reachable any other way for this distro. Each of the 7 was individually verified via rebuild before being kept in this platform-loosened state; three needed additional fixes to actually build once genuinely attempted for the first time on macOS: - rosgraph_monitor: same "-Wl,--no-undefined is GNU-ld-only" pattern already fixed for rmw_stats_shim this session. - system_modes: the already-documented Rolling-wide ament_target_dependencies() removal (see build_feedback.md), fixed with the same local link_ament_dependencies() macro pattern. - sick_safetyscanners_base: the largest fix of the three -- Boost.ASIO API removals across 8 files: find_package(Boost COMPONENTS system) dropped (header-only now, matches the rtabmap/web-video-server/ cartographer_ros pattern), boost::asio::io_service -> io_context, io_service::work -> executor_work_guard<io_context::executor_type> (constructed via make_work_guard), boost::asio::deadline_timer -> explicit boost::asio::basic_deadline_timer<boost::posix_time::ptime> (the plain deadline_timer typedef is gated behind a macro this Boost build doesn't define, but the underlying template is still present and just deprecated-not-removed), address_v4::from_string -> make_address_v4, address_v4::to_ulong -> to_uint, plus two missing <boost/date_time/posix_time/posix_time_types.hpp> includes that used to arrive transitively via the now-gated deadline_timer.hpp. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
traversaro
approved these changes
Sep 6, 2026
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
vinca-sort-vinca-listsparsespackages_select_by_deps(etc.) by collecting all- if: ... then: [...]blocks, then sorting each block'sthen:items independently.- if:blocks placed back-to-back, with no blank line or comment between them, were being merged into a single block during parsing: the code only closed the currently-open block when it hit a list-level comment, never when it hit a new- if:line.then:items into one flat list and reassigned them back to their original line positions — which can move an item that belongs to one platform condition into a different, unrelated one.RoboStack/ros-rolling: adding several packages to a- if: not wasm32 and not winblock that was immediately followed (no separating comment) by- if: linux and not aarch64causedwebots_ros2(linux-only, non-aarch64) to swap places with an item from the other block after a routine sort. The existing workaround there was a comment between the two blocks, which happened to close the block correctly as a side effect — this PR fixes the actual boundary detection so that workaround isn't required.Fix
Close the current if-block whenever a new
- if:line is seen (mirroring the existing comment-based close), so each block'sthen:items are sorted within their own scope regardless of whether they're adjacent to another block.Test plan
vinca/test_sort_vinca_lists.pywith a regression test reproducing the bug (adjacent if-blocks, no separator) and a companion test confirming the comment-separator workaround still workspixi run test— 161 passed (was 157; +4 new)pixi run fmt-checkandpixi run lint-check— clean🤖 Generated with Claude Code