Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Dev / Local
participant CI as CI Runner
participant GH as GitHub Actions (build-push-action)
participant Script as run_tests_with_cloudxr.sh
participant Compose as Docker Compose
participant Docker as Docker Daemon
Note over Dev,CI: Two possible flows
Dev->>Script: invoke script (uses docker compose build)
Script->>Compose: docker compose build (cloudxr-runtime, isaacteleop-tests) with CXR_BUILD_CONTEXT
Compose->>Docker: build images from repository context (uses .dockerignore)
Script->>Compose: docker compose up -d cloudxr-runtime
Compose->>Docker: create & start runtime container
CI->>GH: workflow triggers teleop-ros2 build
GH->>Docker: build image via build-push-action (buildx, cache, load:true)
GH->>Docker: load built image into runner
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…llow VERSION file
aristarkhovNV
force-pushed
the
aaristarkhov/containers
branch
from
0017afc to
d3ed299
Compare
![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 the current code and only fix it if needed.
Inline comments:
In @.dockerignore:
- Around line 1-21: Remove the redundant allow-pattern and its comment: delete
the line containing "!deps/cloudxr/" (and the preceding comment "Allow CloudXR
Dockerfiles and runtime assets." if present) because the later pattern "!deps/"
already re-includes the entire deps/ tree; update .dockerignore so only the
broader "!deps/" entry remains.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 814d1fe0-6df0-4ab9-9af6-1959df615145
📒 Files selected for processing (6)
.dockerignorecmake/IsaacTeleopVersion.cmakedeps/cloudxr/Dockerfile.testdeps/cloudxr/docker-compose.test.yamlexamples/teleop_ros2/Dockerfilescripts/run_tests_with_cloudxr.sh
💤 Files with no reviewable changes (1)
- examples/teleop_ros2/Dockerfile
| # Keep Docker build contexts lean for CloudXR compose builds. | ||
| # Ignore everything by default, then allow only what current Dockerfiles need. | ||
|
|
||
| * | ||
|
|
||
| # Allow Docker metadata files. | ||
| !.dockerignore | ||
|
|
||
| # Allow CloudXR Dockerfiles and runtime assets. | ||
| !deps/cloudxr/ | ||
|
|
||
| # Allow build artifacts and tests consumed by Dockerfile.test and Dockerfile.runtime. | ||
| !install/ | ||
| !examples/ | ||
|
|
||
| # Allow building from source for ros2 workflows | ||
| !cmake/ | ||
| !deps/ | ||
| !src/ | ||
| !CMakeLists.txt | ||
| !VERSION |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant pattern at line 10.
Line 10 (!deps/cloudxr/) is redundant because line 18 (!deps/) already re-includes the entire deps/ directory. Consider removing line 10 and its associated comment.
🔧 Suggested fix
# Allow Docker metadata files.
!.dockerignore
-# Allow CloudXR Dockerfiles and runtime assets.
-!deps/cloudxr/
-
# Allow build artifacts and tests consumed by Dockerfile.test and Dockerfile.runtime.
!install/
!examples/
-# Allow building from source for ros2 workflows
+# Allow CloudXR Dockerfiles, runtime assets, and building from source for ros2 workflows
!cmake/
!deps/
!src/
!CMakeLists.txt
!VERSION🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.dockerignore around lines 1 - 21, Remove the redundant allow-pattern and
its comment: delete the line containing "!deps/cloudxr/" (and the preceding
comment "Allow CloudXR Dockerfiles and runtime assets." if present) because the
later pattern "!deps/" already re-includes the entire deps/ tree; update
.dockerignore so only the broader "!deps/" entry remains.
There was a problem hiding this comment.
Pull request overview
This PR improves incremental container builds for CloudXR test runs by moving container image builds under Docker Compose (so cache and service build definitions are shared), and by reducing build-context churn with a restrictive root .dockerignore. It also adjusts CMake version derivation to avoid requiring Git metadata for non-CI builds (which is important when Docker build contexts exclude .git).
Changes:
- Switch
run_tests_with_cloudxr.shfromdocker build+docker compose up --buildto a dedicateddocker compose buildstep andupwithout rebuilds. - Add a Compose
build:definition for theisaacteleop-testsservice and reorderCOPYsteps in the test Dockerfile to improve layer caching. - Add a root
.dockerignoreto keep the CloudXR compose build context lean, and relax CMake version logic to only require Git when running under CI.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/run_tests_with_cloudxr.sh | Builds runtime + test images via docker compose build and avoids rebuilding on up, improving incremental iteration. |
| examples/teleop_ros2/Dockerfile | Removes unused build packages (clang-format, git) from the ROS2 image build stage. |
| deps/cloudxr/docker-compose.test.yaml | Adds build: config for isaacteleop-tests so Compose can build it directly from repo root context with args. |
| deps/cloudxr/Dockerfile.test | Reorders COPY to improve cache reuse and removes redundant WORKDIR. |
| cmake/IsaacTeleopVersion.cmake | Avoids requiring Git for non-CI version calculation, improving builds in contexts without .git. |
| .dockerignore | Introduces an allowlist-based build context for root-context Docker builds (used by CloudXR compose builds). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/teleop_ros2/Dockerfile (2)
74-87:⚠️ Potential issue | 🟠 Major
rm -rf builddefeats the cache mount's purpose for incremental builds.The
--mount=type=cacheon line 74 persists the build directory acrossdocker buildinvocations to enable incremental compilation. However,rm -rf buildon line 87 destroys these cached artifacts at the end of each build, negating the incremental build benefit.Additionally, cache mount contents are never included in Docker layers, so this removal doesn't reduce image size—it only wipes the cache.
Proposed fix: Remove the unnecessary rm -rf
&& cmake --build build --config Release -j$(nproc) \ - && cmake --install build --config Release \ - && rm -rf build + && cmake --install build --config Release🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/teleop_ros2/Dockerfile` around lines 74 - 87, The Dockerfile currently mounts a build cache using --mount=type=cache,target=/opt/isaacteleop/build,... to enable incremental builds but then deletes that cache with rm -rf build after cmake install; remove the rm -rf build step so the cache mount is preserved across builds (i.e., delete the final "rm -rf build" invocation following the cmake --install command) and rely on the cache mount for cleanup/size management instead of removing the build directory.
65-69:⚠️ Potential issue | 🔴 CriticalAdd
gitto the builder stage's apt-get install—FetchContent dependencies require it.The CMake configuration (line 75-81) sets
BUILD_PLUGINS=ONand usesFetchContent_DeclarewithGIT_REPOSITORYURLs to fetch sanitizers-cmake, openxr-sdk, yaml-cpp, flatbuffers, and mcap from GitHub. All of these are unconditional dependencies. Withoutgit, the CMake configure step will fail.Proposed fix
RUN apt-get update && apt-get install -y --no-install-recommends \ build-essential \ cmake \ pkg-config \ + git \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/teleop_ros2/Dockerfile` around lines 65 - 69, The builder stage apt-get install in the Dockerfile is missing git, but the project's CMake uses FetchContent_Declare (unconditional FetchContent dependencies like sanitizers-cmake, openxr-sdk, yaml-cpp, flatbuffers, mcap) when BUILD_PLUGINS=ON during configure; add git to the apt-get install list in the builder stage so FetchContent can clone those GIT_REPOSITORY URLs and the CMake configure step succeeds (update the RUN apt-get install -y --no-install-recommends ... line to include git).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@examples/teleop_ros2/Dockerfile`:
- Around line 74-87: The Dockerfile currently mounts a build cache using
--mount=type=cache,target=/opt/isaacteleop/build,... to enable incremental
builds but then deletes that cache with rm -rf build after cmake install; remove
the rm -rf build step so the cache mount is preserved across builds (i.e.,
delete the final "rm -rf build" invocation following the cmake --install
command) and rely on the cache mount for cleanup/size management instead of
removing the build directory.
- Around line 65-69: The builder stage apt-get install in the Dockerfile is
missing git, but the project's CMake uses FetchContent_Declare (unconditional
FetchContent dependencies like sanitizers-cmake, openxr-sdk, yaml-cpp,
flatbuffers, mcap) when BUILD_PLUGINS=ON during configure; add git to the
apt-get install list in the builder stage so FetchContent can clone those
GIT_REPOSITORY URLs and the CMake configure step succeeds (update the RUN
apt-get install -y --no-install-recommends ... line to include git).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06111663-9829-45d8-af7e-e57147ecdf1b
📒 Files selected for processing (1)
examples/teleop_ros2/Dockerfile
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-ubuntu.yml:
- Around line 171-182: The GitHub Action uses docker/build-push-action@v7 with
context: . and file: examples/teleop_ros2/Dockerfile, but your .dockerignore
currently unignores all of examples/, sending unnecessary files and killing
cache effectiveness; open .dockerignore and replace the broad unignore (e.g.,
"!examples/") with a narrow allowlist that only unignores the exact build
context and required files (for example "examples/teleop_ros2/**", plus any
specific top-level files the Dockerfile needs such as LICENSE or build scripts),
or alternatively change the action's context to examples/teleop_ros2 to avoid
sending the repo root—apply the approach that best fits the Dockerfile
dependencies.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 0ac00f03-82b6-4297-92b4-d41a4f83fccf
📒 Files selected for processing (2)
.github/workflows/build-ubuntu.ymlexamples/teleop_ros2/Dockerfile
💤 Files with no reviewable changes (1)
- examples/teleop_ros2/Dockerfile
.github/workflows/build-ubuntu.yml
Outdated
| uses: docker/build-push-action@v7 | ||
| with: | ||
| context: . | ||
| file: examples/teleop_ros2/Dockerfile | ||
| build-args: | | ||
| ROS_DISTRO=${{ matrix.ros_distro }} | ||
| PYTHON_VERSION=${{ matrix.python_version }} | ||
| tags: teleop_ros2_ref:${{ matrix.ros_distro }} | ||
| load: true | ||
| push: false | ||
| cache-from: type=gha,scope=teleop-ros2-${{ matrix.ros_distro }} | ||
| cache-to: type=gha,mode=min,scope=teleop-ros2-${{ matrix.ros_distro }} |
There was a problem hiding this comment.
context: . currently defeats incremental gains due to overly broad .dockerignore allowlist.
Line 173 switches to repo-root context, but .dockerignore currently unignores all of examples/. That sends far more files than needed on every build and weakens cache efficiency.
Suggested fix (narrow allowlist in .dockerignore)
-!examples/
+!examples/
+# Re-ignore all examples content, then allow only required paths.
+examples/**
+!examples/teleop_ros2/
+!examples/teleop_ros2/**
+!examples/oxr/
+!examples/oxr/python/
+!examples/oxr/python/**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-ubuntu.yml around lines 171 - 182, The GitHub Action
uses docker/build-push-action@v7 with context: . and file:
examples/teleop_ros2/Dockerfile, but your .dockerignore currently unignores all
of examples/, sending unnecessary files and killing cache effectiveness; open
.dockerignore and replace the broad unignore (e.g., "!examples/") with a narrow
allowlist that only unignores the exact build context and required files (for
example "examples/teleop_ros2/**", plus any specific top-level files the
Dockerfile needs such as LICENSE or build scripts), or alternatively change the
action's context to examples/teleop_ros2 to avoid sending the repo root—apply
the approach that best fits the Dockerfile dependencies.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/teleop_ros2/Dockerfile`:
- Around line 110-114: The Dockerfile repeats the inline assignment
UV_CACHE_DIR=/root/.cache/uv before each uv invocation (seen around the RUN that
calls "uv venv" and "uv pip install"); remove the redundant per-command prefixes
by either adding a single ENV UV_CACHE_DIR=/root/.cache/uv earlier in the
Dockerfile or by omitting UV_CACHE_DIR entirely and relying on uv's default
cache when running as root, then update the RUN line(s) to call "uv venv
--python=${PYTHON_VERSION}" and "uv pip install ..." without the inline
UV_CACHE_DIR=... prefixes.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: ee2cbcd8-c6a5-4700-abea-6d958d33ef4f
📒 Files selected for processing (1)
examples/teleop_ros2/Dockerfile
examples/teleop_ros2/Dockerfile
Outdated
| RUN --mount=type=cache,target=/root/.cache/uv,id=isaacteleop-teleop-ros2-uv-${PYTHON_VERSION} \ | ||
| UV_CACHE_DIR=/root/.cache/uv uv venv --python=${PYTHON_VERSION} && \ | ||
| UV_CACHE_DIR=/root/.cache/uv uv pip install setuptools wheel && \ | ||
| UV_CACHE_DIR=/root/.cache/uv uv pip install --no-build-isolation-package nlopt \ | ||
| -r /tmp/requirements.txt -r /tmp/requirements-retargeters.txt msgpack msgpack-numpy && \ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reducing verbosity by setting UV_CACHE_DIR once.
The inline UV_CACHE_DIR=/root/.cache/uv prefix is repeated for each uv command. Since /root/.cache/uv is already uv's default cache location when running as root, the explicit setting is defensive but redundant. If you prefer to keep it explicit for clarity, consider setting it once:
RUN --mount=type=cache,target=/root/.cache/uv,id=isaacteleop-teleop-ros2-uv-${PYTHON_VERSION} \
- UV_CACHE_DIR=/root/.cache/uv uv venv --python=${PYTHON_VERSION} && \
- UV_CACHE_DIR=/root/.cache/uv uv pip install setuptools wheel && \
- UV_CACHE_DIR=/root/.cache/uv uv pip install --no-build-isolation-package nlopt \
+ export UV_CACHE_DIR=/root/.cache/uv && \
+ uv venv --python=${PYTHON_VERSION} && \
+ uv pip install setuptools wheel && \
+ uv pip install --no-build-isolation-package nlopt \
-r /tmp/requirements.txt -r /tmp/requirements-retargeters.txt msgpack msgpack-numpy && \
rm /tmp/requirements.txt /tmp/requirements-retargeters.txtAlternatively, since this is the default location, you could omit UV_CACHE_DIR entirely and rely on uv's default behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/teleop_ros2/Dockerfile` around lines 110 - 114, The Dockerfile
repeats the inline assignment UV_CACHE_DIR=/root/.cache/uv before each uv
invocation (seen around the RUN that calls "uv venv" and "uv pip install");
remove the redundant per-command prefixes by either adding a single ENV
UV_CACHE_DIR=/root/.cache/uv earlier in the Dockerfile or by omitting
UV_CACHE_DIR entirely and relying on uv's default cache when running as root,
then update the RUN line(s) to call "uv venv --python=${PYTHON_VERSION}" and "uv
pip install ..." without the inline UV_CACHE_DIR=... prefixes.
b0cc0d5 to
630d0d3
Compare
Summary by CodeRabbit