refactor: modularize vinca recipe and pipeline generation - #135
Conversation
|
This is quite nice, but I think it would be nice to test this in an actual |
0c35276 to
9107f14
Compare
9107f14 to
8f1a3f9
Compare
|
Rebased this onto current @traversaro, on testing in a real Rebase notes. The "byte-for-byte identical output" claim didn't hold. I built a differential harness running
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. Other fixes while in here. Restored a few things the extraction changed by accident: Added 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: One last thing: |
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 |
|
Opened RoboStack/ros-jazzy#273 as a draft test build against this branch. Two caveats on reading it: ros-jazzy pins vinca at Separately, on the 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 I also corrected my earlier comment: I had claimed the |
|
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. |
Why
main.pyhad grown to ~1400 lines and mixed config loading, platform detection, mutex handling, source generation and recipe output generation together.generate_outputalone 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.pygoes from 1466 to 779 lines. Configuration loading, platform detection, mutex recipes, source generation and output generation move intoconfiguration.py,platforms.py,mutex.py,sources.pyandrecipes.py.generate_outputis now a handful of named helpers. The shared CI graph and batching helpers live inpipeline.py.The Azure generator is gone, along with the
vinca-azureentry point;vinca-migratenow shells out tovinca-ghainstead. The packaged CI scripts moved fromazure_templates/toci_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 patchesvinca.main.resolve_pkgnamein a test will silently stop having any effect, because the call site now lives invinca.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
masterover a corpus of package.xml fixtures:cmakein run requirements.masterremoved only the first occurrence before appending thetarget_platform != 'emscripten-wasm32'selector.catkin_pkgreportsrun_dependsandexec_dependsseparately, so a single<exec_depend>cmake</exec_depend>already produces two entries — which meant an unconditionalcmakesurvived next to the selector and defeated it. All occurrences are replaced now.<prefix>-mimick-vendorin host requirements. Same bug, same fix.masterevaluated the check inside the build-tool loop, whichcontinues 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.get_all_ancestorsalways 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.batch_stagesemitted 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.os.path.commonprefixcompares paths as strings and only looked at the first patch. Nowos.path.commonpathover all of them, with posix separators so Windows output matches everyone else.build_in_own_azure_stagestill works —build_in_own_stageis just the new name.Needs a real test run
The packaged unix CI script exported
CI=azureand readBUILD_SOURCEBRANCHNAME/BUILD_REPOSITORY_NAME. Those are Azure DevOps variables and are empty on GitHub Actions, so it now exportsCI=githuband reads theGITHUB_*equivalents.CIis consumed by the conda-forge.scripts/build_unix.shthat lives in therobostack/ros-*repos rather than here, so I can't verify the downstream effect from this repo. This is the part worth trying in an actualros-*repo before merging (@traversaro).Testing
210 tests, up from 157 on
master.recipes.pygained a golden-output test plus targeted tests for each cross-compilation rule — that is what surfaced thecmakeand mimick-vendor bugs above.pipeline.py,configuration.py,sources.py,platforms.pyandmutex.pyall got their own tests.pytest,ruff check,ruff format --checkandpyrefly checkpass on every commit in the series, on Linux, macOS and Windows.Type checking is new.
pyreflyruns in CI at itsbasicpreset, which is clean today. Turning oncheck-unannotated-defsreports ~129 findings, mostly test doubles and pre-existing dynamic code, so that's left for a follow-up.Not covered: the
vinca-migratepath, and theCI=githubchange above.