fix(install): add Jetson host setup to installer - #1702
Merged
ericksoa merged 9 commits intoApr 11, 2026
Conversation
Contributor
|
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 an unconditional early invocation of Changes
Sequence DiagramsequenceDiagram
participant User as Invoking User
participant Installer as scripts/install.sh
participant JetsonSetup as scripts/setup-jetson.sh
participant Host as Host (kernel / Docker / files)
User->>Installer: run installer
Installer->>JetsonSetup: execute early setup script
JetsonSetup->>Host: read /etc/nv_tegra_release
JetsonSetup->>JetsonSetup: derive L4T -> map to jp6/jp7 or none
alt supported Jetson (jp6/jp7)
JetsonSetup->>Host: modprobe br_netfilter
JetsonSetup->>Host: sysctl net.bridge.bridge-nf-call-iptables=1
JetsonSetup->>Host: persist /etc/modules-load.d & /etc/sysctl.d
alt jp6
JetsonSetup->>Host: update-alternatives to iptables-legacy
JetsonSetup->>Host: patch /etc/docker/daemon.json (jq required)
JetsonSetup->>Host: systemctl restart docker
end
else unsupported or non-Jetson
JetsonSetup-->>JetsonSetup: exit 0 (no-op)
end
JetsonSetup-->>Installer: return status
Installer->>Installer: continue remaining install steps
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/install.sh (2)
413-416: Function name and comment are misleading.
detect_sudoactually checks if running as root (EUID == 0), not specifically if invoked viasudo. A user could be root without sudo (e.g., logged in as root directly). Consider renaming tois_rootorrunning_as_rootfor clarity.♻️ Suggested clarification
-detect_sudo() { - # 0 if sudo +is_root() { + # Returns 0 (success) if running as root ((EUID == 0)) }And update the call site at line 806:
- if ! detect_sudo; then + if ! is_root; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 413 - 416, Rename the misleading function detect_sudo to a clearer name like is_root (or running_as_root) and update its internal comment to state it checks whether the current effective UID is root (EUID == 0); then update all call sites that reference detect_sudo (including the one that was noted in the review) to use the new name so behavior remains the same but intent is clear. Ensure you change the function declaration name and every invocation (and any related documentation/comments) to avoid leaving the old identifier behind.
826-827: Kernel module and sysctl settings are not persisted across reboots.The
modprobe br_netfilterandsysctl -wchanges will be lost on reboot. If these settings are required for normal operation, consider persisting them:
/etc/modules-load.d/nemoclaw.confforbr_netfilter/etc/sysctl.d/99-nemoclaw.conffornet.bridge.bridge-nf-call-iptables=1💡 Persistence suggestion
modprobe br_netfilter + echo "br_netfilter" > /etc/modules-load.d/nemoclaw.conf sysctl -w net.bridge.bridge-nf-call-iptables=1 >/dev/null + echo "net.bridge.bridge-nf-call-iptables=1" > /etc/sysctl.d/99-nemoclaw.conf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 826 - 827, The script currently runs the one-off commands "modprobe br_netfilter" and "sysctl -w net.bridge.bridge-nf-call-iptables=1" which are not persisted; update the install.sh flow to both apply the settings immediately and persist them by writing the module name ("br_netfilter") into a modules-load configuration file and writing "net.bridge.bridge-nf-call-iptables=1" into a sysctl configuration file (ensure you run sysctl --system or equivalent after writing the file), and keep the existing immediate commands so behavior is unchanged on first run; locate the occurrences of the commands "modprobe br_netfilter" and "sysctl -w net.bridge.bridge-nf-call-iptables=1" to add the persistence file writes and reload step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 811-819: In the orin) case the current sed invocation (the line
containing /"iptables": false,/d; /"bridge": "none"/d; s/"default-runtime":
"nvidia",/"default-runtime": "nvidia"/) can corrupt JSON and the substitution is
effectively a no-op; replace this line with a safe JSON edit using jq: load the
daemon.json, delete the "iptables" and "bridge" keys if present, set
"default-runtime" to "nvidia", write the file atomically and validate JSON; also
add a check in the orin) branch to warn and skip the change if jq is not
installed (or install it), and ensure you handle trailing commas/empty objects
by relying on jq rather than sed.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 413-416: Rename the misleading function detect_sudo to a clearer
name like is_root (or running_as_root) and update its internal comment to state
it checks whether the current effective UID is root (EUID == 0); then update all
call sites that reference detect_sudo (including the one that was noted in the
review) to use the new name so behavior remains the same but intent is clear.
Ensure you change the function declaration name and every invocation (and any
related documentation/comments) to avoid leaving the old identifier behind.
- Around line 826-827: The script currently runs the one-off commands "modprobe
br_netfilter" and "sysctl -w net.bridge.bridge-nf-call-iptables=1" which are not
persisted; update the install.sh flow to both apply the settings immediately and
persist them by writing the module name ("br_netfilter") into a modules-load
configuration file and writing "net.bridge.bridge-nf-call-iptables=1" into a
sysctl configuration file (ensure you run sysctl --system or equivalent after
writing the file), and keep the existing immediate commands so behavior is
unchanged on first run; locate the occurrences of the commands "modprobe
br_netfilter" and "sysctl -w net.bridge.bridge-nf-call-iptables=1" to add the
persistence file writes and reload step.
🪄 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: CHILL
Plan: Pro
Run ID: 839c2062-8da0-4aa8-ac02-6883c5ccd888
📒 Files selected for processing (1)
scripts/install.sh
Contributor
|
✨ Thanks for submitting this PR, which proposes a fix for an issue with the installer on Jetson devices and may improve the overall installation experience. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/setup-jetson.sh (1)
45-45:⚠️ Potential issue | 🟠 MajorLine-based
sedondaemon.jsonis brittle and can corrupt Docker config.Line [45] mutates JSON textually (and also assumes
/etc/docker/daemon.jsonexists). A malformed file here will break Docker restart and fail the installer.Suggested fix
- sudo sed -i '/"iptables": false,/d; /"bridge": "none"/d; s/"default-runtime": "nvidia",/"default-runtime": "nvidia"/' /etc/docker/daemon.json + if [[ -f /etc/docker/daemon.json ]]; then + if command -v jq >/dev/null 2>&1; then + local tmp_daemon + tmp_daemon="$(mktemp)" + "${SUDO[@]}" jq 'del(.iptables, .bridge) | .["default-runtime"] = "nvidia"' /etc/docker/daemon.json >"$tmp_daemon" \ + && "${SUDO[@]}" mv "$tmp_daemon" /etc/docker/daemon.json + else + error "jq is required to safely patch /etc/docker/daemon.json" + fi + else + info "/etc/docker/daemon.json not found; skipping Docker daemon patch" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-jetson.sh` at line 45, The inline sed invocation in scripts/setup-jetson.sh that edits the Docker daemon JSON textually is brittle and can corrupt the config; replace the sed-based mutation (the sed -i '/"iptables": false,/d; /"bridge": "none"/d; s/"default-runtime": "nvidia",/"default-runtime": "nvidia"/' invocation) with a robust jq-based workflow: verify the daemon JSON file exists (or create a safe minimal JSON), make a timestamped backup, use jq to delete keys with values {"iptables":false} and {"bridge":"none"} and to set or ensure "default-runtime":"nvidia", write the output atomically to a temp file then move it into place, validate resulting JSON with jq --exit-status, and if validation fails restore the backup and exit with an error; update the script to log these steps and avoid any direct textual regex edits.
🧹 Nitpick comments (1)
test/runner.test.ts (1)
632-636: Strengthen this guard to assert executable permissions too.Existence alone won’t catch mode regressions on
scripts/setup-jetson.sh.Suggested test enhancement
it("scripts/setup-jetson.sh exists", () => { - expect(fs.existsSync(path.join(import.meta.dirname, "..", "scripts", "setup-jetson.sh"))).toBe( - true, - ); + const scriptPath = path.join(import.meta.dirname, "..", "scripts", "setup-jetson.sh"); + expect(fs.existsSync(scriptPath)).toBe(true); + const mode = fs.statSync(scriptPath).mode; + expect((mode & 0o111) !== 0).toBe(true); });As per coding guidelines,
**/*.sh: All shell scripts must have shebangs and be executable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner.test.ts` around lines 632 - 636, The test "scripts/setup-jetson.sh exists" currently only checks existence; update it to also assert the file is executable by the test runner. After locating the file via the existing path.join(import.meta.dirname, "..", "scripts", "setup-jetson.sh"), use a permission check (e.g., fs.accessSync with fs.constants.X_OK or fs.statSync and bitmask 0o111) to assert executable bits are set and fail the test if not.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 1146-1147: The installer now has four steps but the TOTAL_STEPS
variable and step labels are still set for three; update the TOTAL_STEPS
constant to 4 and fix the step invocation/labels so progress displays correctly
(e.g., change the "step 0 \"Jetson Setup\"" call and any subsequent step numbers
to the correct 1-based sequence) — look for TOTAL_STEPS and the step(...)
invocations in scripts/install.sh and renumber/increment them consistently so
the progress shown (e.g., [current/TOTAL_STEPS]) matches the actual step count.
In `@scripts/setup-jetson.sh`:
- Around line 37-59: The script currently always prefixes critical commands with
sudo (update-alternatives, sed editing /etc/docker/daemon.json, modprobe
br_netfilter, sysctl, and systemctl restart docker) even when EUID==0; change
the calls so they run without sudo when already root—e.g., add a small helper or
inline conditional that uses sudo only if EUID != 0 (use the existing EUID check
around the top) and apply it to the commands referenced (update-alternatives,
sed, modprobe, sysctl -w net.bridge.bridge-nf-call-iptables, and systemctl
restart docker) to avoid failing on root systems where sudo is not available.
---
Duplicate comments:
In `@scripts/setup-jetson.sh`:
- Line 45: The inline sed invocation in scripts/setup-jetson.sh that edits the
Docker daemon JSON textually is brittle and can corrupt the config; replace the
sed-based mutation (the sed -i '/"iptables": false,/d; /"bridge": "none"/d;
s/"default-runtime": "nvidia",/"default-runtime": "nvidia"/' invocation) with a
robust jq-based workflow: verify the daemon JSON file exists (or create a safe
minimal JSON), make a timestamped backup, use jq to delete keys with values
{"iptables":false} and {"bridge":"none"} and to set or ensure
"default-runtime":"nvidia", write the output atomically to a temp file then move
it into place, validate resulting JSON with jq --exit-status, and if validation
fails restore the backup and exit with an error; update the script to log these
steps and avoid any direct textual regex edits.
---
Nitpick comments:
In `@test/runner.test.ts`:
- Around line 632-636: The test "scripts/setup-jetson.sh exists" currently only
checks existence; update it to also assert the file is executable by the test
runner. After locating the file via the existing path.join(import.meta.dirname,
"..", "scripts", "setup-jetson.sh"), use a permission check (e.g., fs.accessSync
with fs.constants.X_OK or fs.statSync and bitmask 0o111) to assert executable
bits are set and fail the test if not.
🪄 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: CHILL
Plan: Pro
Run ID: f8ca80e7-db66-4809-a5b7-6c65dc37aa04
📒 Files selected for processing (3)
scripts/install.shscripts/setup-jetson.shtest/runner.test.ts
- Replace brittle sed-based daemon.json editing with jq - Use SUDO array pattern so script works when already root - Widen L4T version match to 38.* glob for JP7 - Persist br_netfilter and sysctl settings across reboots - Add info message for unrecognized Jetson L4T versions - Test executable bit in addition to file existence Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
The contributor tested the sed approach on real Jetson hardware. jq is not guaranteed to be available on JetPack images. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
approved these changes
Apr 11, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Tested fixes pushed — LGTM.
ericksoa
enabled auto-merge (squash)
April 11, 2026 03:26
realkim93
added a commit
to realkim93/NemoClaw
that referenced
this pull request
Apr 12, 2026
Resolve setup-jetson.sh add/add conflict: keep our more complete version (236 lines vs 84 lines from NVIDIA#1702) which includes Node.js version check, Docker runtime config, and kernel module setup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9 tasks
ericksoa
added a commit
to cheese-head/NemoClaw
that referenced
this pull request
Apr 14, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR attempts to add Jetson support to the default NemoClaw installer. Right now the installer does not work on Jetson because some required host kernel and network settings need to be enabled before the normal install flow can succeed. This change makes the installer handle that setup automatically so the installer flow works on Jetson Orin and Jetson Thor as well and no manual intervention is needed by the user. ## Changes - Added Jetson detection - Added Jetson family detection for Orin (nvgpu) and NVIDIA Thor. - Added a Jetson-only setup step that runs before the normal installer steps. ## Type of Change <!-- Check the one that applies. --> - [X] Code change for a new feature, bug fix, or refactor. - [ ] Code change with doc updates. - [ ] Doc only. Prose changes without code sample modifications. - [ ] Doc only. Includes code sample changes. ## Testing <!-- What testing was done? --> I ran the installer script on Jetson Thor and Jetson and I verified it ran end to end with no issues. I did not run on anything else to verify there is no regression else where yet. - [X] `npx prek run --all-files` passes (or equivalently `make check`). - [X] `npm test` passes. - [ ] `make docs` builds without warnings. (for doc-only changes) ## Checklist ### General - [X] I have read and followed the [contributing guide](https://github.com/NVIDIA/NemoClaw/blob/main/CONTRIBUTING.md). - [X] I have read and followed the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md). (for doc-only changes) ### Code Changes <!-- Skip if this is a doc-only PR. --> - [X] Formatters applied — `npx prek run --all-files` auto-fixes formatting (or `make format` for targeted runs). - [X] Tests added or updated for new or changed behavior. - [X] No secrets, API keys, or credentials committed. - [ ] Doc pages updated for any user-facing behavior changes (new commands, changed defaults, new features, bug fixes that contradict existing docs). ### Doc Changes <!-- Skip if this PR has no doc changes. --> - [ ] Follows the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md). Try running the `nemoclaw-contributor-update-docs` agent skill to draft changes while complying with the style guide. For example, prompt your agent with "`/nemoclaw-contributor-update-docs` catch up the docs for the new changes I made in this PR." - [ ] New pages include SPDX license header and frontmatter, if creating a new page. - [ ] Cross-references and links verified. --- <!-- DCO sign-off (required by CI). Replace with your real name and email. --> Signed-off-by: Khalil Ben Khaled <kbenkhaled@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Installer now detects NVIDIA Jetson devices and runs an automatic host-configuration step early during setup. * **Improvements** * Applies targeted system and networking adjustments for supported Jetson platforms (iptables/Docker, bridge netfilter, sysctl) and improves privilege escalation handling for reliable configuration. * **Tests** * Added a regression test ensuring the Jetson setup step is present and executable in the installer. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
Closed
2 tasks
10 tasks
wscurran
added
bug-fix
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
3 participants
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.
Summary
This PR attempts to add Jetson support to the default NemoClaw installer. Right now the installer does not work on Jetson because some required host kernel and network settings need to be enabled before the normal install flow can succeed. This change makes the installer handle that setup automatically so the installer flow works on Jetson Orin and Jetson Thor as well and no manual intervention is needed by the user.
Changes
Type of Change
Testing
I ran the installer script on Jetson Thor and Jetson and I verified it ran end to end with no issues. I did not run on anything else to verify there is no regression else where yet.
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
nemoclaw-contributor-update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/nemoclaw-contributor-update-docscatch up the docs for the new changes I made in this PR."Signed-off-by: Khalil Ben Khaled kbenkhaled@nvidia.com
Summary by CodeRabbit
New Features
Improvements
Tests