fix: install CLI in one env and elevate only per-operation#193
Merged
Conversation
…6980) `./holohub setup` regressed on PEP 668 hosts (Jetson AGX Thor / IGX Thor): `sudo ./holohub setup` failed with `externally-managed-environment`, and a venv install then died in `sccache.sh` writing `/opt/sccache` unprivileged. Rework the setup privilege model around two rules: - One Python environment. The CLI runs in the interpreter you're already in (active venv, or the wrapper-managed venv), so build/run/setup and `sys.executable` stay consistent, and setup never installs into system Python. - Elevate only the system steps. Replace the command-text sudo heuristic in `utils/io.py` with an explicit `run_command(..., as_root=True)` and a `write_system_file()` helper; delete `_classify_sudo_requirement` / `_process_command_with_sudo`. Route apt, the Kitware keyring/apt source, and the bash-completion copy through explicit elevation; install the ngc CLI into `~/.local/bin` (no root). Setup scripts are plain, familiar bash (no helper library): they run as the invoking user in the project's Python environment with sudo available -- `sudo apt-get ...` for system packages, `python3 -m pip ...` for Python. The CLI runs them with the venv on PATH and prompts for sudo once on the default path. `sccache` installs to a user-owned dir on the host (system-wide only when root), so no elevation is needed to close the bug; `Dockerfile.util` installs sudo so the scripts work in-container. Signed-off-by: Wenqi Li <wenqil@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
wyli
changed the title
- write_system_file: remove the never-used `mode` parameter (YAGNI). - setup_cmake: a failed Kitware key download now exits with a clear fatal message instead of an uncaught CalledProcessError traceback. - setup_ngc_cli: print a PATH hint when ~/.local/bin is not on PATH, so users know why `ngc` isn't found (and why setup would re-install it). Signed-off-by: Wenqi Li <wenqil@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
wyli
marked this pull request as ready for review
July 2, 2026 08:56
check_existing_sccache relied on `command -v sccache` alone; with the user-local default install (~/.local/bin, possibly not on PATH yet) that missed prior installs and re-downloaded on every run. Also check the install symlink location. Mirrors the same fix in the HoloHub copy of this script (holohub#1635 review feedback). Signed-off-by: Wenqi Li <wenqil@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
WalkthroughThe PR makes setup and installation flows explicitly root-aware, updates CLI environment/status reporting, and adds unit coverage for the new command execution, installation, and reporting behavior. ChangesRoot-aware setup and CLI reporting
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 3
🧹 Nitpick comments (1)
src/holoscan_cli/setup_scripts/sccache.sh (1)
71-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit declare/assign to silence SC2155.
Shellcheck flags Line 75 for masking return values by combining
localand command substitution.🔧 Proposed fix
local sccache_bin sccache_bin="$(command -v sccache 2>/dev/null || true)" [[ -n "$sccache_bin" ]] || sccache_bin="$SYMLINK_PATH" [[ -x "$sccache_bin" ]] || return 1 - local ver_output=$("$sccache_bin" --version 2>/dev/null || true) + local ver_output + ver_output=$("$sccache_bin" --version 2>/dev/null || true)🤖 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 `@src/holoscan_cli/setup_scripts/sccache.sh` around lines 71 - 75, The sccache version probe in the sccache.sh setup script is triggering SC2155 because `local` is combined with command substitution. Update the version-check logic in the sccache_bin/version retrieval block by splitting the declaration from the assignment so the command’s exit status is not masked, while keeping the existing behavior of capturing `sccache --version` output.Source: Linters/SAST tools
🤖 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 `@src/holoscan_cli/utils/host_setup.py`:
- Around line 237-239: The Kitware source is added in the host setup flow, but
install_packages_if_missing may reuse _apt_updated and skip apt-get update, so
the new repository is not visible before installing cmake packages. In
host_setup.py, update the setup sequence around the write_system_file calls and
install_packages_if_missing so apt is refreshed immediately after adding
/etc/apt/sources.list.d/kitware.list, then install "cmake" and
"cmake-curses-gui". Keep the fix localized to the host setup logic that prepares
the Kitware repo and package installation.
- Around line 225-231: In host_setup.py, the key retrieval logic in the
subprocess.run call for the Kitware archive key masks wget failures because the
shell pipeline only reports the last command’s status. Update this path by
either splitting the download and dearmor steps into separate subprocess calls
or configuring the shell with pipefail so the failure from wget is propagated;
keep the fix centered around the dearmored assignment and the key download
command.
In `@src/holoscan_cli/utils/io.py`:
- Around line 217-225: The elevate handling in the command execution helper is
validating and requiring sudo before the dry-run path can return, which causes
dry-run to fail unnecessarily. In the io.py helper that checks elevate and later
returns for dry_run, move the sudo lookup/required-root fatal check to only run
when an actual command execution is needed, and keep dry-run output generation
working even if sudo is missing. Use the existing command execution logic around
elevate and dry_run in this function to ensure `holoscan setup --dryrun` never
depends on sudo availability.
---
Nitpick comments:
In `@src/holoscan_cli/setup_scripts/sccache.sh`:
- Around line 71-75: The sccache version probe in the sccache.sh setup script is
triggering SC2155 because `local` is combined with command substitution. Update
the version-check logic in the sccache_bin/version retrieval block by splitting
the declaration from the assignment so the command’s exit status is not masked,
while keeping the existing behavior of capturing `sccache --version` output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f74734f6-65fe-4ca5-aa74-a77c9aaa88de
📒 Files selected for processing (9)
src/holoscan_cli/commands/setup_cmd.pysrc/holoscan_cli/setup_scripts/Dockerfile.utilsrc/holoscan_cli/setup_scripts/benchmarking.shsrc/holoscan_cli/setup_scripts/debug.shsrc/holoscan_cli/setup_scripts/sccache.shsrc/holoscan_cli/setup_scripts/xvfb.shsrc/holoscan_cli/utils/host_setup.pysrc/holoscan_cli/utils/io.pytests/unit/test_host_setup.py
Address three CodeRabbit findings on the setup path: - io.py: `--dryrun` no longer requires sudo to be installed. When sudo is missing on a non-root host, dry-run prints the would-be `sudo ...` command instead of exiting; real execution still fails with the clear actionable message. Adds unit tests for both paths and for the run-directly-as-root case. - host_setup.py: fetch and dearmor the Kitware key as two separate subprocess steps instead of a `wget | gpg` shell pipeline, so a download failure cannot be masked by the pipeline's exit status and install an empty keyring (also removes shell=True). - host_setup.py: force `apt-get update` after adding the Kitware apt source. `_apt_updated` may already be true from an earlier step, which made install_packages_if_missing skip the refresh and miss Kitware's cmake packages. Signed-off-by: Wenqi Li <wenqil@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ination setup_ngc_cli linked ngc into ~/.local/bin unconditionally: in a container build (root) that is /root/.local/bin, which is not on PATH in the built image (the previous /usr/local/bin location was). Split by invoker: container build (root) = system-wide /usr/local/bin; host = per-user ~/.local/bin, no root needed. Also check the destination itself before downloading -- `which ngc` alone re-downloads NGC on every setup run whenever ~/.local/bin is not on PATH. A dangling symlink fails the check and is repaired by the `ln -sf`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes holoscan setup behavior on PEP 668 (“externally managed”) hosts by ensuring the CLI never installs into system Python and by making privilege elevation explicit per operation (instead of inferred from command text).
Changes:
- Reworked
run_command()to support explicitas_root=Trueelevation and addedwrite_system_file()for privileged file writes viasudo tee. - Updated host setup flows (apt operations, Kitware CMake repo/key install, NGC linking) to use explicit elevation and more idempotent destination checks.
- Adjusted setup script execution to run in the same Python environment as the CLI, and updated several setup scripts / Dockerfile to rely on
sudofor system package steps.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_io.py | Adds unit tests for explicit as_root behavior (dry-run, missing sudo, already-root). |
| tests/unit/test_host_setup.py | Updates/extends tests for root-elevated apt installs and NGC destination/link behavior. |
| src/holoscan_cli/utils/io.py | Implements explicit elevation (as_root) and adds write_system_file() helper. |
| src/holoscan_cli/utils/host_setup.py | Switches apt actions to explicit elevation; rewrites Kitware repo/key install without shell pipeline; updates NGC installation/linking. |
| src/holoscan_cli/commands/setup_cmd.py | Runs scripts in the CLI’s Python environment and primes sudo once for default setup. |
| src/holoscan_cli/setup_scripts/xvfb.sh | Converts apt steps to sudo apt-get ... and splits cleanup commands. |
| src/holoscan_cli/setup_scripts/debug.sh | Converts apt steps to sudo apt-get ... and splits cleanup commands. |
| src/holoscan_cli/setup_scripts/benchmarking.sh | Converts apt steps to sudo apt-get ... and keeps Python installs in the project environment. |
| src/holoscan_cli/setup_scripts/sccache.sh | Installs sccache system-wide only when root; otherwise uses user-owned install + symlink and PATH guidance. |
| src/holoscan_cli/setup_scripts/Dockerfile.util | Ensures sudo exists in the build image so setup scripts can run consistently. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- run_command: dry-run now returns CompletedProcess(exec_cmd, 0) so .args has the same shape as a real run instead of the display string. - setup_cmake: ensure wget alongside gpg so the helper is self-contained, and catch FileNotFoundError from the key fetch with an actionable fatal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
The first `run_command(..., as_root=True)` prompts naturally and sudo caches the credential for the rest of the run. Pre-priming added a helper plus two imports, prompted even when everything was already installed (no as_root command ever runs then), and could not prevent a re-prompt after sudo's timeout anyway. The explicit as_root=True / write_system_file() design is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
`env-info` said nothing about the holoscan-cli package itself. Add a "Holoscan CLI Information" block reporting the package version, install location, which Python environment it runs in (wrapper-managed venv / virtual environment / system Python), the matching uninstall command, and any HOLOSCAN_CLI_SOURCE override. Surface the wrapper's own env vars (SOURCE/VENV/PYTHON_BIN/INSTALL_ARGS, PIP_BREAK_SYSTEM_PACKAGES) in the env-var listing, and prefix `env-check`'s CLI line with the package version. Also fix the entry-point test's skip guard: entry_points() never raises PackageNotFoundError, so uninstalled dev environments failed instead of skipping; probe the distribution explicitly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/holoscan_cli/utils/env_info.py (1)
42-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between virtualenv/system-python branches.
The
elif/elsebranches print identical uninstall guidance ({sys.executable} -m pip uninstall holoscan-cli), differing only in the label text. Could be consolidated, but low priority given the branches are clear and well tested.🤖 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 `@src/holoscan_cli/utils/env_info.py` around lines 42 - 61, The virtualenv and system-Python branches in collect_cli_info() duplicate the same uninstall command, with only the environment label changing. Refactor the shared uninstall guidance out of the conditional so the environment-specific label is handled in the branches and the common {sys.executable} -m pip uninstall holoscan-cli message is printed once, while keeping the wrapper-managed venv case separate.
🤖 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.
Nitpick comments:
In `@src/holoscan_cli/utils/env_info.py`:
- Around line 42-61: The virtualenv and system-Python branches in
collect_cli_info() duplicate the same uninstall command, with only the
environment label changing. Refactor the shared uninstall guidance out of the
conditional so the environment-specific label is handled in the branches and the
common {sys.executable} -m pip uninstall holoscan-cli message is printed once,
while keeping the wrapper-managed venv case separate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3cf11de-ef52-40bf-ba72-e793b1150627
📒 Files selected for processing (4)
src/holoscan_cli/system_check.pysrc/holoscan_cli/utils/env_info.pytests/unit/test_env_info.pytests/unit/test_package_data.py
…ests - main.yaml: new holohub-integration job runs HoloHub's wrapper suite against the PR checkout (HOLOSCAN_CLI_SOURCE), closing the reverse direction -- holohub already tests against holoscan-cli@main, but a holoscan-cli PR never validated the wrapper contract before merge. The build job now gates on it. - main.yaml/release.yaml: smoke the sdist install and the wheel's [create] extra in clean venvs, not just the bare wheel. - test_host_setup.py: cover setup_cmake's post-source apt refresh, setup_sccache's bundled-script + SCCACHE_MIN_VERSION contract, and the aarch64 ngc archive selection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
The setup command's own dispatch had no unit coverage (only the host_setup utilities did). Cover _build_script_env's venv-on-PATH / VIRTUAL_ENV / PYTHONHOME handling, the --scripts path's script-env and dry-run contract, and the default path's helper ordering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Mirror the holohub review fix: clean the apt cache alongside the lists cleanup so the sudo bootstrap layer does not retain package archives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Mirror the holohub review fix: non-root defaults expand $HOME; with HOME unset they degraded to /.local/... and confusing permission errors. Fail with an actionable message unless SCCACHE_INSTALL_DIR and SCCACHE_SYMLINK are both provided. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Containerized `run --as-root` now runs two containers: a user-mapped, blocking builder (`holoscan build --local`, default run args filtered of name/cidfile/detach/user), then the application container as root with --no-local-build. Local `run --local --as-root` elevates only the final application via sudo, carrying the CLI-computed environment across. Also: env-info reports HOLOSCAN_CLI_PINNED_VERSION; setup scripts drop a stale inherited VIRTUAL_ENV; ngc destination must be an executable file (directory collisions fail; non-executable leftovers are repaired). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Keep a single representative test per new behavior (elevation, ngc destination split, setup dispatch, run --as-root two-phase, env-info) and drop the elaborate edge-case variants to keep the suite lean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
- Drop `sudo -E`: only the explicit preserve_env allowlist reaches the root process via /usr/bin/env; extend the run --as-root allowlist with the project-declared run/mode env keys so declared variables still arrive. - Clear PYTHONHOME unconditionally for setup scripts; a stale value breaks the python/pip resolved from the prepended PATH even outside a venv. - Resolve wget/gpg to absolute paths in the Kitware key fetch (S607). - holohub-integration job: `permissions: contents: read` and `persist-credentials: false` on both checkouts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
build and run --as-root assembled the same in-container build command independently; extract make_local_build_command and cover its flags with one exact-string assertion in the existing container-branch test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
tbirdso
approved these changes
Jul 8, 2026
…one-env # Conflicts: # src/holoscan_cli/container/core.py # src/holoscan_cli/utils/io.py
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
./holohub setupregressed on PEP 668 hosts (Jetson AGX Thor / IGX Thor):sudo ./holohub setup→error: externally-managed-environment./holohub setup→sccache.shdies writing/opt/sccacheunprivilegedApproach — two rules
(your active venv, or the wrapper-managed venv); setup never installs into
system Python. Because CLI and app share one interpreter,
sys.executableisthe app's interpreter and build/run/setup stay consistent.
run_command(..., as_root=True)and
write_system_file()replace the command-text sudo heuristic(
_classify_sudo_requirement/_process_command_with_sudodeleted). apt, theKitware keyring/apt source, and the bash-completion copy elevate explicitly;
ngcand the hostsccachebinary install to~/.local(no root).Setup scripts are plain, familiar bash (no helper DSL): they run as the invoking
user in the project's Python environment with sudo available —
sudo apt-get …for system packages,
python3 -m pip …for Python.Dockerfile.utilinstallssudo so the scripts work in-container.
Notes
fix: bootstrap HoloHub CLI in managed venv).🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
holoscan setupby running setup scripts with the intended environment and by explicitly using elevated permissions for apt installs and system file updates (including bash completion).sudo apt-getflows and cleanup.sccacheandngcinstallers to correctly handle root vs non-root installs, skip when already present, and provide clearer PATH guidance.Enhancements
check_cli()output to include the installedholoscan-cliversion and additional environment details.Tests
ngcsymlink setup, CLI info reporting, andcheck_cli().