Add an optional platform-finalize publish mode#118
Add an optional platform-finalize publish mode#118han-xudong wants to merge 2 commits intoRoboStack:masterfrom
Conversation
|
One clarification on scope: although the PR text focuses on GitHub Actions, the commit also updates the Azure generator and adds matching tests there. I kept those changes in the same PR because the publish model is the same in both generators:
If you would prefer to review GitHub Actions and Azure separately, I can split that in a follow-up. |
|
I am not sure if the azure pipelines generator actually are tested or currently work, so I would leave them aside. |
|
Thanks a lot for the contribution @han-xudong , can you check the CI failures? fyi @wep21 |
| if publish_mode == "platform-finalize" and prev_batch_keys: | ||
| add_publish_job(azure_template, "win-64", vm_imagename, prev_batch_keys) | ||
|
|
There was a problem hiding this comment.
Can you explain why this is necessary?
There was a problem hiding this comment.
Pull request overview
Adds an opt-in “platform-finalize” publish mode to generated CI workflows so that per-batch jobs build (and upload artifacts) without publishing, and a final per-platform job/stage publishes only after the full platform build succeeds.
Changes:
- Add
--publish-mode {immediate,platform-finalize}to both GHA and Azure pipeline generators. - In
platform-finalizemode, batch jobs setVINCA_SKIP_UPLOAD=1and upload artifacts; a final publish job/stage downloads artifacts and uploads packages. - Adjust Unix artifact upload path generation to use
~/conda-bld/<target>.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
vinca/generate_gha.py |
Adds publish-mode CLI + deferred publish flow via per-batch artifacts and a final publish job. |
vinca/generate_azure.py |
Adds publish-mode CLI + deferred publish flow via pipeline artifacts and a final publish stage. |
vinca/test_generate_publish_workflows.py |
Adds tests validating the generated YAML structure for platform-finalize mode and artifact paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| append_azure_publish_stage( | ||
| azure_template, | ||
| target, | ||
| stage_names, | ||
| azure_template.get("pool"), |
There was a problem hiding this comment.
In platform-finalize mode, the publish stage depends on stage_names, but stage_names is populated for every computed stage even when that stage ends up omitted from azure_stages (because it has zero jobs). That can produce a dependsOn reference to a stage that is not present in the emitted YAML, causing the pipeline to fail to load. Build the dependency list from the stages actually appended (e.g., derive names from azure_stages) before calling append_azure_publish_stage.
| append_azure_publish_stage( | ||
| azure_template, | ||
| target, | ||
| stage_names, | ||
| azure_template.get("pool"), |
There was a problem hiding this comment.
Same issue as Linux: stage_names can include stages that were never appended to azure_stages, so the publish stage may dependsOn non-existent stages. Consider constructing the dependsOn list from the emitted azure_stages only.
vinca/generate_azure.py
Outdated
| append_azure_publish_stage( | ||
| azure_template, | ||
| "win-64", | ||
| stage_names, |
There was a problem hiding this comment.
Same issue as Linux/OSX: stage_names includes stage names even for stages that were dropped (no jobs), so the publish stage may end up depending on stages that don't exist in the final YAML. Derive the publish stage dependencies from azure_stages instead.
| stage_names, | |
| [stage["stage"] for stage in azure_stages], |
vinca/generate_azure.py
Outdated
| f"""curl -fsSL https://pixi.sh/install.sh | bash | ||
| export PATH="$HOME/.pixi/bin:$PATH" | ||
| shopt -s globstar nullglob | ||
| files=("$(Pipeline.Workspace)"/artifacts/**/*.conda "$(Pipeline.Workspace)"/artifacts/**/*.tar.bz2) | ||
| if (( ${{#files[@]}} == 0 )); then | ||
| echo "No built packages found for {target}" | ||
| exit 1 | ||
| fi | ||
| pixi run upload "${{files[@]}}" --force""" |
There was a problem hiding this comment.
The Unix publish stage installs pixi via curl ... | bash, which is a supply-chain risk and makes builds less reproducible. Prefer a pinned, checksum-verified installer (or another deterministic install mechanism) to avoid executing remote content directly.
| f"""curl -fsSL https://pixi.sh/install.sh | bash | |
| export PATH="$HOME/.pixi/bin:$PATH" | |
| shopt -s globstar nullglob | |
| files=("$(Pipeline.Workspace)"/artifacts/**/*.conda "$(Pipeline.Workspace)"/artifacts/**/*.tar.bz2) | |
| if (( ${{#files[@]}} == 0 )); then | |
| echo "No built packages found for {target}" | |
| exit 1 | |
| fi | |
| pixi run upload "${{files[@]}}" --force""" | |
| f"""shopt -s globstar nullglob | |
| files=("$(Pipeline.Workspace)"/artifacts/**/*.conda "$(Pipeline.Workspace)"/artifacts/**/*.tar.bz2) | |
| if (( ${{#files[@]}} == 0 )); then | |
| echo "No built packages found for {target}" | |
| exit 1 | |
| fi | |
| for file in "${{files[@]}}"; do | |
| anaconda -t "$ANACONDA_API_TOKEN" upload "$file" --force | |
| done""" |
| { | ||
| "name": "Setup pixi", | ||
| "uses": "prefix-dev/setup-pixi@v0.9.4", | ||
| "with": { |
There was a problem hiding this comment.
The publish job sets up pixi without pinning a pixi-version, while the Windows build jobs pin pixi-version: v0.63.2. To keep publish behavior reproducible and aligned with the build jobs, consider pinning the same pixi version here as well.
| "with": { | |
| "with": { | |
| "pixi-version": "v0.63.2", |
|
@han-xudong I asked a GitHub Copilot PR just to help me, but feel free to ignore comments that do not have sense, for example as they just refer to code copied from the rest of the codebase. |
@traversaro Thank you for your quick reply! I am checking the code and will submit a new commit as soon as possible. Copilot's suggestions are helpful~ |
|
@traversaro Thanks for the review. The reason for this mode is to avoid partial per-platform publication during long RoboStack builds. Builds can already run in batches, but if uploads happen batch-by-batch, a platform can temporarily expose only part of its dependency graph while the rest is still building. For ROS packages with dense interdependencies, that can create an inconsistent intermediate state.
I also pushed a follow-up update for the Copilot comments:
I added regression coverage for both and re-ran:
|
Related to #117.
This proposes an optional deferred publish mode for generated GitHub Actions workflows.
In
platform-finalizemode:VINCA_SKIP_UPLOAD=1This keeps the current behavior as the default and makes the deferred publish flow opt-in.
I also included a small follow-up fix for the Unix artifact upload path so generated workflows use
~/conda-bld/<target>rather than relying on${{ env.HOME }}.Validation:
pixi run pytest vinca/test_generate_publish_workflows.py -q5 passed