Modifies ROOT user related setup to Dockerfile.curobo#5376
Modifies ROOT user related setup to Dockerfile.curobo#5376njawale42 wants to merge 5 commits intoisaac-sim:developfrom
Conversation
The NVIDIA Isaac Sim base image declares no USER directive and defaults to root, so the explicit USER root in this Dockerfile is a no-op declaration. Removing it aligns Dockerfile.curobo with the upstream NVIDIA Isaac Sim Dockerfile and with the other Isaac Lab Dockerfiles (Dockerfile.ros2, Dockerfile.installci) which omit the directive. Build and runtime behavior are unchanged: the container still runs as root via the base image default.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR makes two minor cleanup changes to docker/Dockerfile.curobo: (1) removes a redundant USER root directive since the base image already defaults to root, and (2) changes two hardcoded /root/.bashrc references to use ${HOME}/.bashrc for consistency with the 8 preceding lines in the same RUN block. The changes are correct and improve maintainability.
Architecture Impact
Self-contained. This is a Docker build configuration change that affects only the cuRobo container image build process. No runtime code, no Isaac Lab framework code, and no other Dockerfiles are impacted. The container behavior remains identical since ${HOME} resolves to /root in the current configuration.
Implementation Verdict
Ship it
Test Coverage
No tests required. This is a Docker configuration cleanup with no behavioral change. The PR author's claim that "the container still builds and runs identically" is correct — both changes are purely cosmetic consistency improvements. Manual verification that the container builds successfully would be the appropriate validation, which is covered by standard CI Docker builds if available.
CI Status
No CI checks available yet. Once CI runs, the Docker build job (if present) would validate this change.
Findings
🔵 Improvement: docker/Dockerfile.curobo:34 — Removal of USER root is correct
The NVIDIA Isaac Sim base image does not set a non-root user, so this directive was indeed a no-op. Removing it aligns with the upstream Dockerfile style and reduces noise.
🔵 Improvement: docker/Dockerfile.curobo:170-171 — Consistent use of ${HOME} is correct
The change from /root/.bashrc to ${HOME}/.bashrc makes the entire RUN block internally consistent. While HOME=/root in the current build context, using the variable is more maintainable if the base image ever changes or if the Dockerfile is adapted for non-root users in the future.
Verification note: I confirmed that ${HOME} is the correct variable to use here by examining the full file context. The DOCKER_USER_HOME environment variable (line 33) is a different variable intended for bind-mount paths in Singularity (lines 116-123), while ${HOME} is the standard shell variable used throughout the alias block. The PR correctly uses ${HOME} to match the existing pattern.
No critical or warning-level issues identified. This is a clean, low-risk cleanup PR.
Greptile SummaryThis PR refactors
Confidence Score: 3/5Not safe to merge as-is — two P1 issues: a build-breaking hardcoded Python path and a HOME/DOCKER_USER_HOME mismatch that orphans pre-created cache directories at runtime. Two P1 findings: the hardcoded docker/Dockerfile.curobo — both P1 issues are in the new user-setup block (lines 157-165) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[FROM base image\ninitial user: isaac-sim uid=1234] --> B[USER root]
B --> C[apt installs / CUDA keyring\ncuda-toolkit-12-8 etc.]
C --> D[COPY source files\nno chown - root-owned]
D --> E[pip install toml / apt deps\ncreate singularity dirs under DOCKER_USER_HOME=/root\nnvidia placeholders / reinstall pip]
E --> F[useradd isaaclab uid=1000\nhome=/home/isaaclab]
F --> G[chown -R isaaclab:isaaclab\n ISAACLAB_PATH\n kit/python/lib/python3.12/site-packages ⚠️\n DOCKER_USER_HOME=/root\n /home/isaaclab]
G --> H[USER isaaclab]
H --> I[pip cache mount uid=1000\nisaaclab.sh --install\npip installs cuRobo / teleop]
I --> J[bashrc aliases\nHOME=/home/isaaclab ⚠️ vs DOCKER_USER_HOME=/root]
J --> K[COPY --chown=isaaclab:isaaclab\nall remaining files]
K --> L[WORKDIR ISAACLAB_PATH\nruntime as isaaclab uid=1000]
Reviews (3): Last reviewed commit: "Runs Dockerfile.curobo as new isaaclab u..." | Re-trigger Greptile |
…ile.curobo
The base image (nvcr.io/nvidian/isaac-sim:latest-develop) ends with
USER isaac-sim (uid 1234). Previously the Dockerfile switched back to
root for the entire build and left runtime as root via
OMNI_KIT_ALLOW_ROOT. Removing the initial USER root broke CI because
apt needs root; simply restoring it keeps us as root the whole way
which is what we want to avoid.
Correct pattern applied here:
* Explicit USER root for the system-level setup block (apt, cuda
repo, /bin/nvidia-* placeholders, /root cache dirs, get-pip
bootstrap, COPYed source files).
* After the last root-required step, chown only the paths we are
about to write to from the non-root user
(${ISAACLAB_PATH}, site-packages, ${DOCKER_USER_HOME}) to
isaac-sim:isaac-sim. /isaac-sim itself is already isaac-sim-owned
from the base image, so re-chowning it would needlessly bloat
the layer.
* USER isaac-sim before the editable pip installs
(isaaclab.sh --install, cuRobo, isaaclab_teleop) and the bashrc
writes, so those paths end up owned by the runtime user.
* BuildKit cache mount at ${DOCKER_USER_HOME}/.cache/pip pinned to
uid=1234,gid=1234, otherwise it would default to root-owned and
isaac-sim could not write into it.
* Final COPY annotated with --chown=isaac-sim:isaac-sim for
consistency.
This keeps CI build args (DOCKER_USER_HOME_ARG=/root) and
docker-compose paths unchanged, so docker-compose usage continues to
work with OMNI_KIT_ALLOW_ROOT. Dockerfile.base and Dockerfile.ros2
follow the same legacy pattern and are out of scope for this PR.
njawale42
changed the title
… compat
The previous attempt switched to the base image's isaac-sim user (uid 1234),
which broke the test step in CI: the test runner bind-mounts the host
workspace (/opt/github-runner/_work/.../IsaacLab, owned by uid 1000) over
/workspace/isaaclab, overlaying our build-time chown. As uid 1234, the
container could not write to the bind mount and `mkdir tests` failed before
pytest even started.
Fix: create a fresh isaaclab user with uid 1000 to match the runner host
user, instead of remapping isaac-sim. Creating a new user (rather than
remapping isaac-sim's UID) avoids triggering an OverlayFS copy-up of the
multi-GB Isaac Sim install in /isaac-sim, which would otherwise become
orphan-owned. /isaac-sim stays mode 755 and isaac-sim-owned, so isaaclab
can still read and execute everything there. Only the paths the new user
needs to write to (${ISAACLAB_PATH}, kit site-packages, ${DOCKER_USER_HOME},
/home/isaaclab) are chowned.
Also bumps the BuildKit pip cache mount uid/gid from 1234 to 1000 (numeric
form is required because BuildKit does not consult /etc/passwd at mount
time) and switches the final COPY --chown to isaaclab.
Caveat: hardcodes uid 1000, matching the typical EC2 self-hosted runner
user. If a runner is reconfigured to a different uid this will break the
same way; in that case the right next step is to parameterize via build
arg, but until then 1000 is the simplest correct value.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| RUN chown -R isaaclab:isaaclab \ | ||
| ${ISAACLAB_PATH} \ | ||
| ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages \ |
There was a problem hiding this comment.
python3.12 is hard-coded in the chown path. If the upstream Isaac Sim base image ships Python 3.13 (or any other version), this path won't exist and the RUN chown -R layer will fail at build time with "No such file or directory," breaking the entire image build.
Use a glob or a shell expansion to resolve the versioned directory dynamically:
| ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages \ | |
| $(ls -d ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.*/site-packages 2>/dev/null | head -1) \ |
| RUN useradd -ms /bin/bash -l --uid 1000 isaaclab -d /home/isaaclab | ||
|
|
||
| RUN chown -R isaaclab:isaaclab \ | ||
| ${ISAACLAB_PATH} \ | ||
| ${ISAACSIM_ROOT_PATH}/kit/python/lib/python3.12/site-packages \ | ||
| ${DOCKER_USER_HOME} \ | ||
| /home/isaaclab | ||
|
|
||
| USER isaaclab |
There was a problem hiding this comment.
$HOME and $DOCKER_USER_HOME point to different directories
useradd -d /home/isaaclab sets the isaaclab user's $HOME to /home/isaaclab. However, DOCKER_USER_HOME (passed in as /root from CI build args) is what drives the singularity cache-directory setup: mkdir -p ${DOCKER_USER_HOME}/.cache/ov, ${DOCKER_USER_HOME}/.nv, ${DOCKER_USER_HOME}/.nvidia-omniverse, etc., all land under /root/.
At runtime any tool that resolves its cache via $HOME (standard XDG behaviour, ~) will look in /home/isaaclab/ — where those dirs don't exist — rather than in /root/. The pre-created directories under $DOCKER_USER_HOME=/root will be orphaned for those tools.
Consider one of:
- Setting
DOCKER_USER_HOME_ARG=/home/isaaclabin CI so the singularity dirs are created under the user's real home, or - Keeping
DOCKER_USER_HOME=/rootbut additionally creating the same set of subdirectories (or symlinks) under/home/isaaclab/so both$HOME-based and$DOCKER_USER_HOME-based lookups succeed.
kellyguo11
changed the title
2 participants
Description
Refactors
docker/Dockerfile.curoboso the editable Isaac Lab installs and the final container runtime drop privileges to the non-rootisaac-simuser (uid 1234) provided by the base image (nvcr.io/nvidian/isaac-sim:latest-develop, see upstream Isaac Sim Dockerfile). The CI image (build-curobo→test-curobo/test-skillgen) currently runs as root the whole way, even though the base image already moved to a non-root default.What changed
USER rootfor the system-level setup block. apt, cuda-keyring repo,install_deps.py apt,${DOCKER_USER_HOME}cache dirs,/bin/nvidia-*placeholders, the pre-bundled torch removal, and theget-pip.pybootstrap all require root and stay as root.chown+USER isaac-simbefore the pip installs. Only the paths we still need to write to are chowned (${ISAACLAB_PATH},${ISAACSIM_ROOT_PATH}/kit/python/.../site-packages,${DOCKER_USER_HOME}) —/isaac-simis already isaac-sim-owned by the base image, so re-chowning it would needlessly bloat the layer.uid=1234,gid=1234for${DOCKER_USER_HOME}/.cache/pip, otherwise the default root-owned cache mount would be unwritable fromisaac-sim.${HOME}/.bashrcused consistently in the aliasRUNblock. Two lines hardcoded/root/.bashrcwhile the surrounding 8 lines used${HOME}/.bashrc; after the user switch this inconsistency would silently land the trailing config in the wrong file.COPYannotated with--chown=isaac-sim:isaac-simfor ownership consistency.CI build args (
DOCKER_USER_HOME_ARG=/rootin.github/actions/ecr-build-push-pull/action.yml,.github/actions/docker-build/action.yml, and.github/workflows/postmerge-ci.yml) are unchanged —/rootis chowned toisaac-siminside the image, so they continue to work.Scope
docker/Dockerfile.baseanddocker/Dockerfile.ros2follow the same legacy root-everywhere pattern but are also consumed bydocker/docker-compose.yaml(which setsOMNI_KIT_ALLOW_ROOT=1and binds${DOCKER_USER_HOME}paths). Migrating those needs coordinated changes to the compose file and CI build args, so they're left for a follow-up PR.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there