Skip to content

refactor: modularize vinca recipe and pipeline generation - #135

Open
wolfv wants to merge 5 commits into
RoboStack:masterfrom
wolfv:refactor/modular-cleanup
Open

refactor: modularize vinca recipe and pipeline generation#135
wolfv wants to merge 5 commits into
RoboStack:masterfrom
wolfv:refactor/modular-cleanup

Conversation

@wolfv

@wolfv wolfv commented Aug 20, 2026

Copy link
Copy Markdown
Member

Why

main.py had grown to ~1400 lines and mixed config loading, platform detection, mutex handling, source generation and recipe output generation together. generate_output alone was 347 lines. Finding anything was painful, and testing any of it meant going through the CLI or reaching into the middle of a huge function.

The GitHub Actions and Azure generators had also drifted into near-copies of each other, with the dependency-graph and batching helpers duplicated between them.

What changed

main.py goes from 1466 to 779 lines. Configuration loading, platform detection, mutex recipes, source generation and output generation move into configuration.py, platforms.py, mutex.py, sources.py and recipes.py. generate_output is now a handful of named helpers. The shared CI graph and batching helpers live in pipeline.py.

The Azure generator is gone, along with the vinca-azure entry point; vinca-migrate now shells out to vinca-gha instead. The packaged CI scripts moved from azure_templates/ to ci_templates/ and were renamed to match.

Compatibility entry points stay in vinca.main, so existing imports keep working. One thing to watch out for: anything that patches vinca.main.resolve_pkgname in a test will silently stop having any effect, because the call site now lives in vinca.recipes. That bit the test suite here and it will bite downstream suites the same way.

Behavior changes

Mostly behavior-preserving, but not entirely. These are deliberate fixes, found by diffing generated output against master over a corpus of package.xml fixtures:

  • cmake in run requirements. master removed only the first occurrence before appending the target_platform != 'emscripten-wasm32' selector. catkin_pkg reports run_depends and exec_depends separately, so a single <exec_depend>cmake</exec_depend> already produces two entries — which meant an unconditional cmake survived next to the selector and defeated it. All occurrences are replaced now.
  • <prefix>-mimick-vendor in host requirements. Same bug, same fix.
  • cyclonedds cross-compilation. master evaluated the check inside the build-tool loop, which continues past tools that don't resolve. A package depending on cyclonedds got no build-platform copy if none of its build tools resolved. The check no longer hangs off that loop.
  • Transitive CI dependencies. get_all_ancestors always indexed the starting node instead of the node it was visiting, so it only ever returned direct dependencies. It now walks the full closure and handles cycles. Expect CI stages to get bigger.
  • CI stage batching. batch_stages emitted individually-built packages before flushing the pending stage, and sized stages using lengths computed before those packages were removed. Both fixed, and it no longer mutates the list it was handed.
  • Patch paths. os.path.commonprefix compares paths as strings and only looked at the first patch. Now os.path.commonpath over all of them, with posix separators so Windows output matches everyone else.

build_in_own_azure_stage still works — build_in_own_stage is just the new name.

Needs a real test run

The packaged unix CI script exported CI=azure and read BUILD_SOURCEBRANCHNAME / BUILD_REPOSITORY_NAME. Those are Azure DevOps variables and are empty on GitHub Actions, so it now exports CI=github and reads the GITHUB_* equivalents. CI is consumed by the conda-forge .scripts/build_unix.sh that lives in the robostack/ros-* repos rather than here, so I can't verify the downstream effect from this repo. This is the part worth trying in an actual ros-* repo before merging (@traversaro).

Testing

210 tests, up from 157 on master. recipes.py gained a golden-output test plus targeted tests for each cross-compilation rule — that is what surfaced the cmake and mimick-vendor bugs above. pipeline.py, configuration.py, sources.py, platforms.py and mutex.py all got their own tests.

pytest, ruff check, ruff format --check and pyrefly check pass on every commit in the series, on Linux, macOS and Windows.

Type checking is new. pyrefly runs in CI at its basic preset, which is clean today. Turning on check-unannotated-defs reports ~129 findings, mostly test doubles and pre-existing dynamic code, so that's left for a follow-up.

Not covered: the vinca-migrate path, and the CI=github change above.

@traversaro

Copy link
Copy Markdown
Member

This is quite nice, but I think it would be nice to test this in an actual robostack/ros-* repos.

@baszalmstra
baszalmstra force-pushed the refactor/modular-cleanup branch 2 times, most recently from 0c35276 to 9107f14 Compare September 1, 2026 08:52
@baszalmstra
baszalmstra force-pushed the refactor/modular-cleanup branch from 9107f14 to 8f1a3f9 Compare September 1, 2026 08:58
@baszalmstra

baszalmstra commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Rebased this onto current master (it had drifted behind #134, #141, #142) and went over it fairly carefully. Pushed the result to this branch. Summary of what changed beyond the rebase, and one thing that needs your eyes:

@traversaro, on testing in a real robostack/ros-* repo — there is one change here I can't verify from this repo. The packaged unix CI script used to export CI=azure and read BUILD_SOURCEBRANCHNAME / BUILD_REPOSITORY_NAME, which are Azure DevOps variables and are empty under GitHub Actions. It now exports CI=github and reads the GITHUB_* equivalents. The old values were plainly broken, but CI is read by the conda-forge .scripts/build_unix.sh that lives in the ros-* repos rather than here, so I don't know what flipping it from azure to github does downstream. That is the piece I'd want a real build to confirm.

Rebase notes. dependencies_only (added to generate_output by #134 for the pinning code) had to be ported into recipes.generate_output. test_snapshot_metadata.py patches vinca.main.resolve_pkgname, and after the split the call site is in vinca.recipes, so the patch silently stopped applying and the test failed — repointed. Worth flagging generally: any downstream test patching vinca.main.* will break the same quiet way.

The "byte-for-byte identical output" claim didn't hold. I built a differential harness running generate_output on both master and this branch over a corpus of package.xml fixtures. Four output divergences, all of which look like genuine bugs on master that this PR happens to fix:

  • cmake in run requirements: master removes only the first occurrence before adding the emscripten selector, but catkin_pkg double-counts run_depends/exec_depends, so an unconditional cmake can survive alongside the selector and defeat it. Correction to an earlier version of this comment: I first wrote that this had a wide blast radius. It does not. Generating all of ros-jazzy shows cmake reaching run exactly once, via buildtool_export_depend, so the single .remove() on master was already enough. Triggering it needs <exec_depend>cmake</exec_depend> or <depend>cmake</depend>, which no jazzy package uses. The bug is real but latent.
  • <prefix>-mimick-vendor in host: same bug.
  • cyclonedds: master evaluates the cross-compile check inside the build-tool loop, which skips past unresolvable tools, so the build-platform copy was missed when no build tool resolved.
  • get_all_ancestors returned only direct dependencies on master (it indexed the start node instead of the node being visited), so CI stages will grow now that it actually walks the closure.

I've kept the new behavior and documented all of it in the PR description rather than reverting, but it's worth a look in case any of it is load-bearing somewhere.

Coverage. recipes.py is the core of this refactor and had one test covering _dummy_constraint, with generate_output itself untested — which is why the above went unnoticed. Added a golden-output test plus targeted tests for each cross-compilation rule. 157 → 210 tests.

Other fixes while in here. Restored a few things the extraction changed by accident: get_conda_subdir had narrowed to an exact (system, machine) table, which would hard-fail on Windows-on-ARM (platform.machine() returns ARM64) where master returned win-64; build_in_own_azure_stage now still works alongside the new build_in_own_stage name; _discover_tests is back to splitting on the first dot; the "not found" warning in get_all_ancestors is back. Also guarded the new os.path.commonpath call, which raises ValueError across drives on Windows where commonprefix never did.

Added pyrefly to CI at its basic preset (clean today). It found one real thing: a dead global selected_platform in parse_command_line with no such module global — the state actually lives on config.selected_platform. Cranking it up to check unannotated functions reports ~129 findings, mostly test doubles, so I left that for later.

Restored explanatory comments the extraction dropped — the "git goes in build for cross-compilation", "cmake is build-only on Emscripten" and cyclonedds rationale — and documented module responsibilities plus the two ownership rules that weren't written down anywhere: vinca_conf is read-only, and the unsatisfied set is caller-owned and mutated in place.

One last thing: fix: normalize recipe patch paths was a two-line fix to a file the first commit created, and that same commit shipped the test needing it, so the first three commits failed on Windows (green on Linux CI, which is why nobody noticed). Squashed it into the commit that introduced the code. All commits now pass tests, lint, format and typecheck on all three platforms.

@traversaro

Copy link
Copy Markdown
Member

The old values were plainly broken, but CI is read by the conda-forge .scripts/build_unix.sh that lives in the ros-* repos rather than here, so I don't know what flipping it from azure to github does downstream. That is the piece I'd want a real build to confirm.

Yes, I would just open a draft full rebuilds PR on one of the ros-* repo pointing to this branch, and check if at least the testpr build works fine.

@baszalmstra

Copy link
Copy Markdown
Collaborator

Opened RoboStack/ros-jazzy#273 as a draft test build against this branch.

Two caveats on reading it: ros-jazzy pins vinca at 7c7eae93, 42 commits behind master, so that PR bumps vinca by master + this branch rather than by this branch alone — a failure there needs a master-pinned control run to attribute. And the lockfile diff is large but inert (v6→v7 format rewrite; 355 package refs before and after, none added or removed).

Separately, on the CI=azureCI=github question I raised earlier: it turns out to be a non-issue. .scripts/build_unix.sh uses only CURRENT_RECIPES, FEEDSTOCK_ROOT, CONDA_BLD_PATH and target — no CI, GIT_BRANCH or FEEDSTOCK_NAME — and that holds for ros-jazzy, ros-humble, ros-noetic, ros-kilted and ros-rolling. A code search across those repos for all three names returns nothing. Worth noting testpr wouldn't have covered it anyway: it builds via rattler-build directly and never executes the packaged script.

What testpr does cover is generation, so I ran that comparison locally first: generating all of ros-jazzy with master's vinca and with this branch, for linux-64, win-64, osx-arm64 and emscripten-wasm32, gives byte-identical output across ~19,000 files apart from the intended \\/ patch-path fix. vinca-gha batching shifts as expected from the batch_stages fixes, but still yields 297 jobs / 1400 recipes / 53 stages with no dependency-ordering inversions on either side.

I also corrected my earlier comment: I had claimed the cmake duplicate-removal change had a wide blast radius. It doesn't — on real jazzy data cmake reaches run exactly once via buildtool_export_depend, so master's single .remove() was already sufficient. The bug is real but latent.

baszalmstra pushed a commit to baszalmstra/ros-jazzy that referenced this pull request Sep 1, 2026
@traversaro

Copy link
Copy Markdown
Member

RoboStack/ros-jazzy#273 seems to be working fine, so that is good to go for merge for me, but probably it would be a good idea to have a thumbs up from @Tobias-Fischer as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants