Skip to content

build: drop CMake presets, move the wheel tree out of build/ - #913

Closed
jiwenc-nv wants to merge 1 commit into
NVIDIA:mainfrom
jiwenc-nv:jiwenc/build-dir-layout
Closed

build: drop CMake presets, move the wheel tree out of build/#913
jiwenc-nv wants to merge 1 commit into
NVIDIA:mainfrom
jiwenc-nv:jiwenc/build-dir-layout

Conversation

@jiwenc-nv

@jiwenc-nv jiwenc-nv commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

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 underneath build/, so rm -rf build took out all of them at once.

This goes back to cmake -B build as the single documented flow and enforces the isolation directly instead. ISAAC_TELEOP_PYTHON_VERSION is 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.12 to 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>, outside build/, 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 plain build/ is safe.

Testing

On Ubuntu 24.04 aarch64:

  • cmake -B build → build → cmake --install: green, 0 errors, produces wheels/isaacteleop-1.5+local-cp311-cp311-linux_aarch64.whl.
  • Upgrade path: trees stamped with the old boolean TRUE are adopted silently rather than erroring — verified a TRUE-stamped cache reconfigures clean and re-stamps to 3.11.
  • sphinx -W builds clean.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

📝 Docs preview is not auto-deployed for fork PRs.

A maintainer with write access to NVIDIA/IsaacTeleop can deploy a preview by
commenting /preview-docs on this PR. Once deployed, the preview
will live at:

https://nvidia.github.io/IsaacTeleop/preview/pr-913/

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 033e36d1-da96-4be7-b4ca-18e1064df735

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change separates managed CMake and wheel build outputs into build-cmake/<cache-tag>/ and build-wheel/<cache-tag>/. CMake presets, scikit-build configuration, CI workflows, .gitignore, and build documentation now use these paths. The documentation also updates installation, testing, output-location, and troubleshooting commands. The build/ directory remains reserved for ad-hoc configurations.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes moving the wheel tree but incorrectly states that CMake presets are being dropped.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@jiwenc-nv jiwenc-nv changed the title build: move managed build trees out of build/ build: drop CMake presets, move the wheel tree out of build/ Aug 6, 2026
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc/build-dir-layout branch from 55c94e9 to 64cc93b Compare August 6, 2026 04:53
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>
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc/build-dir-layout branch from 64cc93b to 6666bdc Compare August 6, 2026 05:02
@jiwenc-nv

Copy link
Copy Markdown
Collaborator Author

/preview-docs

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

✅ Preview deployed: https://NVIDIA.github.io/IsaacTeleop/preview/pr-913/

@jiwenc-nv

Copy link
Copy Markdown
Collaborator Author

Superseded by #916, which carries the same change from a branch on NVIDIA/IsaacTeleop rather than a personal fork.

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.

1 participant