Updates doc building job on main to match develop#5406
Conversation
Greptile SummaryThis PR refactors the doc-building workflow to mirror the Confidence Score: 4/5Safe to merge with awareness of pre-existing issues already flagged in earlier review rounds. Job logic is correct and mutually exclusive conditions work as intended. One P1 around the semantics of != true for skipped-step outputs is noted; two previously-flagged issues remain unaddressed. .github/workflows/docs.yaml - artifact version mismatch and script-injection concerns from prior review rounds are still present. Important Files Changed
Reviews (2): Last reviewed commit: "Updates doc building job on main to matc..." | Re-trigger Greptile |
| steps: | ||
| - name: Download docs artifact | ||
| uses: actions/download-artifact@v4 | ||
| uses: actions/download-artifact@v8 |
There was a problem hiding this comment.
upload-artifact / download-artifact version mismatch
build-multi-docs uploads with actions/upload-artifact@v7, but deploy-docs downloads with actions/download-artifact@v8. These two actions share an artifact-format wire protocol, and a major-version gap between upload and download has historically caused "artifact not found" or deserialization failures in the deploy step. Both should use the same major version.
| trigger-deploy: ${{ steps.trigger-deploy.outputs.defined }} | ||
| steps: | ||
| - id: info | ||
| run: echo "repo=github.repository=${{ github.repository }}, ref=github.ref=${{ github.ref }}" |
There was a problem hiding this comment.
github.ref / github.repository
Directly interpolating ${{ github.ref }} and ${{ github.repository }} into a run shell command is a GitHub Actions script-injection anti-pattern. A branch name or repository name containing shell metacharacters (e.g. $(cmd), backticks) would be executed. Prefer passing the values through environment variables:
- id: info
env:
GH_REPO: ${{ github.repository }}
GH_REF: ${{ github.ref }}
run: echo "repo=github.repository=$GH_REPO, ref=github.ref=$GH_REF"| SMV_BRANCH_WHITELIST: '^(main|develop)$' | ||
| SMV_TAG_WHITELIST: '^v[2-9]\d*\.\d+\.\d+(-[A-Za-z0-9.]+)?$' |
There was a problem hiding this comment.
SMV_TAG_WHITELIST env-var / conf.py default are intentionally divergent — worth a comment
The CI env sets SMV_TAG_WHITELIST: '^v[2-9]\d*…' (v2+ only), while conf.py defaults to ^v[1-9]\d*… (v1+ included). This means a local make multi-docs will build v1.x tag docs, but the deployed site won't include them. The intent is described only in the YAML comment; a matching note in conf.py next to the default regex would make the discrepancy obvious to future maintainers.
| needs: [check-secrets] | ||
| needs: [doc-build-type] | ||
| # run on non-deploy branches to build current version docs only | ||
| if: needs.doc-build-type.outputs.trigger-deploy != 'true' |
There was a problem hiding this comment.
!= 'true' condition also matches skipped step output
When the trigger-deploy step is skipped (i.e. its if condition evaluates to false), GitHub Actions sets the output to an empty string "", not 'false'. The condition needs.doc-build-type.outputs.trigger-deploy != 'true' therefore evaluates to true for both the PR case (empty string) and any future case where the output is accidentally 'false' or any other non-'true' value. In practice this means build-latest-docs will run for all non-deploy pushes to release/** as well as any PR, which appears to be the intended behaviour — but using a more explicit guard makes the intent clearer and avoids accidental dual-job runs if the output format ever changes.
| if: needs.doc-build-type.outputs.trigger-deploy != 'true' | |
| if: needs.doc-build-type.outputs.trigger-deploy == '' || needs.doc-build-type.outputs.trigger-deploy == null |
- Update Omniverse docs URLs from old /app_isaacsim/ and /prod_extensions/ paths to current docs.isaacsim.omniverse.nvidia.com and /extensions/ paths - Update USDRT docs from /kit/docs/usdrt/ to /kit/docs/usdrt.scenegraph/ - Update Kit Manual testing docs from pinned 104.0 to /latest/ - Update Pixar USD link from graphics.pixar.com to openusd.org - Remove dead physics scene link (page no longer exists), use plain text
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR synchronizes the documentation build workflow on the main branch with the updated logic from the develop branch. It modernizes the CI workflow by splitting single-version and multi-version doc builds into separate jobs, updates GitHub Actions versions, fixes external documentation links, and adjusts version filtering to exclude v1.x tags. The changes are straightforward and correctly implemented.
Architecture Impact
Self-contained. Changes are limited to:
- CI workflow configuration (
.github/workflows/docs.yaml) - Sphinx configuration (
docs/conf.py) - Documentation source files with external link updates
No impact on core framework code, no API changes, no runtime behavior changes.
Implementation Verdict
Ship it — Clean modernization of doc infrastructure with minor observations below.
Test Coverage
This is a documentation/CI infrastructure change. The "Build Latest Docs" CI job passed successfully, confirming the Sphinx build works. Test failures in test-general and test-isaaclab-tasks are unrelated to this PR (they're general test suite issues, not doc-related).
CI Status
- ✅
Build Latest Docs: success— Primary validation for this PR passes - ✅
pre-commit: success— Formatting/linting passes - ❌
test-general: failure— Unrelated to doc changes - ❌
test-isaaclab-tasks: failure— Unrelated to doc changes - ⏳
Build Multi-Version Docs: skipped— Expected (only runs on deploy branches) - ⏳
Deploy Docs: skipped— Expected (only runs on deploy branches)
Findings
🔵 Improvement: .github/workflows/docs.yaml:13 — Temporary branch inclusion
- 'feature/isaacsim-6-0'This hardcoded feature branch in the push trigger will become stale after the feature merges. Consider removing it or adding a comment noting when it should be cleaned up. Not blocking, but creates tech debt.
🔵 Improvement: .github/workflows/docs.yaml:81-82 — ENV variable override narrows tag scope
SMV_BRANCH_WHITELIST: '^(main|develop)$'
SMV_TAG_WHITELIST: '^v[2-9]\d*\.\d+\.\d+(-[A-Za-z0-9.]+)?$'The workflow-level env vars exclude release/ branches and v1.x tags from multi-version docs. This is intentional per the PR description ("eliminates some older versions"), but note this means release branches won't appear in the version dropdown even though they trigger the multi-version build path (line 37). The resulting built docs will include main, develop, and v2.x+ tags only. Verify this is the intended behavior.
🔵 Improvement: docs/conf.py:290-291 — Default branch whitelist includes release branches that workflow excludes
smv_branch_whitelist = os.getenv("SMV_BRANCH_WHITELIST", r"^(main|develop|release/.*)$")The default value in conf.py includes release/.* but the workflow ENV override (line 81) excludes it with ^(main|develop)$. This asymmetry could cause confusion for local doc builds vs CI builds. Consider aligning them or documenting the discrepancy.
🔵 Improvement: docs/conf.py:179 — Duplicate mock entry
"gymnasium",gymnasium now appears twice in autodoc_mock_imports — once at line 119 and again at line 179 (the newly added line). This is harmless but unnecessary. The duplicate should be removed.
🟡 Warning: .github/workflows/docs.yaml:52,77 — Inconsistent action versions between jobs
Both build-latest-docs and build-multi-docs use actions/checkout@v6, actions/setup-python@v5, and actions/upload-artifact@v7, which is good. However, deploy-docs uses actions/download-artifact@v8 (line 123) while uploads use @v7. These should be compatible, but verify the v7→v8 artifact format compatibility is tested.
🔵 Improvement: .github/workflows/docs.yaml:88-89 — Detached HEAD and branch deletion
git checkout --detach HEAD
git for-each-ref --format="%(refname:short)" refs/heads/ | xargs -r git branch -DThis is a clever workaround to prevent sphinx-multiversion from building local branches, forcing it to use only remote refs. The -r flag on xargs is correct (handles empty input). This is fine but could benefit from a comment explaining why it's necessary.
# Description Merge changes from main branch: - #4875 - Adds Isaac-Stack-Cube-Franka-IK-Rel-v0 task variants - #4909 - Updates minor RSL-RL configclass docstring - #4934 - Updates Newton docs on main for 3.0 beta changes - #5182 - Fix flatdict version pin to allow 4.1.0+ - #5195 - Add NCCL troubleshooting notes - #5406 - Updates doc building job on main to match develop - #5311 - Update skrl integration for version 2.0.0 - #5482 - Adds nightly-changelog.yml on main - #5527 - Use isaaclab-bot GitHub App token for nightly changelog push - #5537 - Address deprecation warnings in nightly changelog workflow - #5746 - Fix .dockerignore for _isaac_sim symlink - #5745 - Parameterize nightly compile over configurable branches - #5546 - Fix swapped preserve_order docstrings - #5817 - Update skrl agent configurations in the Isaac Lab template
# Description Merge changes from main branch: - isaac-sim#4875 - Adds Isaac-Stack-Cube-Franka-IK-Rel-v0 task variants - isaac-sim#4909 - Updates minor RSL-RL configclass docstring - isaac-sim#4934 - Updates Newton docs on main for 3.0 beta changes - isaac-sim#5182 - Fix flatdict version pin to allow 4.1.0+ - isaac-sim#5195 - Add NCCL troubleshooting notes - isaac-sim#5406 - Updates doc building job on main to match develop - isaac-sim#5311 - Update skrl integration for version 2.0.0 - isaac-sim#5482 - Adds nightly-changelog.yml on main - isaac-sim#5527 - Use isaaclab-bot GitHub App token for nightly changelog push - isaac-sim#5537 - Address deprecation warnings in nightly changelog workflow - isaac-sim#5746 - Fix .dockerignore for _isaac_sim symlink - isaac-sim#5745 - Parameterize nightly compile over configurable branches - isaac-sim#5546 - Fix swapped preserve_order docstrings - isaac-sim#5817 - Update skrl agent configurations in the Isaac Lab template
Description
Updates the doc building job to match the new logic from develop branch. Eliminates some older versions of docs to be built and hopefully resolves other doc building issues we are seeing in PRs.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there