build: drop CMake presets, move the wheel tree out of build/ - #916
build: drop CMake presets, move the wheel tree out of build/#916jiwenc-nv wants to merge 2 commits into
Conversation
The presets existed to give each Python version its own directory, because the interpreter and ABI are baked into a build tree. That bought isolation at the cost of making `cmake -B build` — the form every other page and every plugin still used — the unsupported path, and it put the managed trees underneath build/ so `rm -rf build` took out all of them at once. Go back to `cmake -B build` as the one documented flow, and enforce the isolation directly instead: ISAAC_TELEOP_PYTHON_VERSION is now stamped into the cache, and changing it on an existing tree is a hard error pointing at a fresh directory. Previously it was silently ignored — the cache and the "Configuring for Python X" message would say 3.12 while the extensions compiled against the 3.11 venv. The scikit-build-core tree moves to build-wheel/<cache-tag>, out of build/, so pip and a classic configure cannot collide. Python bindings stay on by default. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR removes CMake presets for Python 3.11, 3.12, and 3.13. CI workflows now configure and build explicit Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/getting_started/build_from_source/index.rst`:
- Around line 272-277: Update the clean-build instructions in
docs/source/getting_started/build_from_source/index.rst:272-277,
docs/source/getting_started/lerobot/data_collection_sim.rst:180-181, and
docs/source/references/build.rst:150-154 so all three uses of `cmake -B build
--fresh` are compatible with the supported CMake 3.20 minimum, or consistently
raise the documented minimum to 3.24 if that support is intentionally dropped.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cbc313b2-5755-4056-9a4c-69c89957122d
📒 Files selected for processing (10)
.github/workflows/build-ubuntu.yml.github/workflows/build-windows.yml.gitignoreCMakePresets.jsonCMakePresets.json.licensecmake/SetupPython.cmakedocs/source/getting_started/build_from_source/index.rstdocs/source/getting_started/lerobot/data_collection_sim.rstdocs/source/references/build.rstpyproject.toml
💤 Files with no reviewable changes (2)
- CMakePresets.json
- CMakePresets.json.license
| Clean rebuild (``--fresh`` wipes the CMake cache and reconfigures): | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| cmake --preset py3.12 --fresh | ||
| cmake --build --preset py3.12 | ||
| cmake -B build --fresh | ||
| cmake --build build |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cmake --version
if ! cmake --help | grep -q -- '--fresh'; then
echo "This CMake does not support --fresh."
exit 1
fiRepository: NVIDIA/IsaacTeleop
Length of output: 199
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- CMake version references ---'
rg -n -i 'cmake.{0,40}(3\.20|minimum|version)|3\.20.{0,40}cmake|cmake.{0,20}3\.24' \
docs CMakeLists.txt .github 2>/dev/null || true
printf '%s\n' '--- Referenced documentation sections ---'
for f in \
docs/source/getting_started/build_from_source/index.rst \
docs/source/getting_started/lerobot/data_collection_sim.rst \
docs/source/references/build.rst
do
echo "### $f"
sed -n '1,80p' "$f" | cat -n
case "$f" in
*build_from_source*) sed -n '250,285p' "$f" | cat -n ;;
*data_collection_sim*) sed -n '165,190p' "$f" | cat -n ;;
*references/build.rst) sed -n '135,165p' "$f" | cat -n ;;
esac
doneRepository: NVIDIA/IsaacTeleop
Length of output: 17175
🌐 Web query:
CMake 3.20 cmake --fresh introduced version official documentation
💡 Result:
The --fresh command-line option was introduced in CMake 3.24, not 3.20 [1][2][3]. This option allows users to perform a fresh configuration of a build tree by removing any existing CMakeCache.txt file and associated CMakeFiles/ directory, effectively starting the configuration from scratch as if the build tree were new [2][3]. It is documented in the official CMake manual under the cmake(1) command-line options [2][4].
Citations:
- 1: https://fossies.org/linux/cmake/Help/release/3.24.rst
- 2: https://cmake.org/cmake/help/latest/manual/cmake.1.html
- 3: https://cmake.org/cmake/help/latest/release/3.24.html
- 4: https://cmake.org/cmake/help/latest/manual/cmake.1.html?highlight=install
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- CMake version references ---'
rg -n -i 'cmake.{0,40}(3\.20|minimum|version)|3\.20.{0,40}cmake|cmake.{0,20}3\.24' \
docs CMakeLists.txt .github 2>/dev/null || true
printf '%s\n' '--- Referenced documentation sections ---'
for f in \
docs/source/getting_started/build_from_source/index.rst \
docs/source/getting_started/lerobot/data_collection_sim.rst \
docs/source/references/build.rst
do
echo "### $f"
case "$f" in
*build_from_source*) sed -n '250,285p' "$f" | cat -n ;;
*data_collection_sim*) sed -n '165,190p' "$f" | cat -n ;;
*references/build.rst) sed -n '135,165p' "$f" | cat -n ;;
esac
doneRepository: NVIDIA/IsaacTeleop
Length of output: 5589
Support the documented CMake versions.
--fresh requires CMake 3.24, but the project supports CMake 3.20 and higher. If CMake 3.20 remains supported, replace all three cmake -B build --fresh commands with a compatible clean-build procedure. Otherwise, raise the documented minimum to CMake 3.24.
📍 Affects 3 files
docs/source/getting_started/build_from_source/index.rst#L272-L277(this comment)docs/source/getting_started/lerobot/data_collection_sim.rst#L180-L181docs/source/references/build.rst#L150-L154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/source/getting_started/build_from_source/index.rst` around lines 272 -
277, Update the clean-build instructions in
docs/source/getting_started/build_from_source/index.rst:272-277,
docs/source/getting_started/lerobot/data_collection_sim.rst:180-181, and
docs/source/references/build.rst:150-154 so all three uses of `cmake -B build
--fresh` are compatible with the supported CMake 3.20 minimum, or consistently
raise the documented minimum to 3.24 if that support is intentionally dropped.
Sources: Path instructions, MCP tools
The clean-rebuild instructions use `cmake --fresh`, which landed in CMake 3.24, while the floor was 3.20. Raise the floor rather than working around it. Ubuntu 22.04's apt cmake is 3.22 and no longer qualifies; the prerequisite list now points at Kitware's APT repo or pip. On the pip path scikit-build-core fetches a CMake from PyPI, so only the <4 cap still matters there. Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
1 participant
The presets existed to give each Python version its own build directory, since the interpreter and ABI are baked into a build tree. That isolation came at a price:
cmake -B build— the form every other page and every plugin still used — became the unsupported path, and the managed trees sat underneathbuild/, sorm -rf buildtook out all of them at once.This goes back to
cmake -B buildas the single documented flow and enforces the isolation directly instead.ISAAC_TELEOP_PYTHON_VERSIONis stamped into the cache, and changing it on an existing tree is now a hard error naming a fresh directory to use.That guard is the one addition beyond the mechanical sweep, and it closes a real silent bug: passing
-DISAAC_TELEOP_PYTHON_VERSION=3.12to a tree configured for 3.11 previously printed "Configuring for Python 3.12" and cached 3.12 while still compiling against the 3.11 venv. Happy to drop it if you'd rather just document "use a fresh directory".The scikit-build-core tree moves to
build-wheel/<cache-tag>, outsidebuild/, so pip and a classic configure can't collide. Python bindings stay ON by default.CI keeps its 3-Python matrix via
-DISAAC_TELEOP_PYTHON_VERSION=; only GitHub-hosted runners build, so a plainbuild/is safe.Testing
On Ubuntu 24.04 aarch64:
cmake -B build→ build →cmake --install: green, 0 errors, produceswheels/isaacteleop-1.5+local-cp311-cp311-linux_aarch64.whl.ctest: 160/161. The one failure,cloudxr_test_launcher, is environmental (CloudXR SDK download 404, noNGC_API_KEYon this machine) and predates the change.TRUEare adopted silently rather than erroring — verified aTRUE-stamped cache reconfigures clean and re-stamps to3.11.sphinx -Wbuilds clean.Summary by CodeRabbit
Build Improvements
Documentation
Chores