Skip to content

Build from source and remove compiled nemoclaw/dist/ from repository#461

Closed
arpankanwer wants to merge 5 commits intoNVIDIA:mainfrom
arpankanwer:main
Closed

Build from source and remove compiled nemoclaw/dist/ from repository#461
arpankanwer wants to merge 5 commits intoNVIDIA:mainfrom
arpankanwer:main

Conversation

@arpankanwer
Copy link
Copy Markdown

@arpankanwer arpankanwer commented Mar 20, 2026

Summary

This PR removes the nemoclaw/dist/ directory from being tracking in the git repository to prevent noisy PR diffs, frequent merge conflicts, and the risk of stale builds. We implemented a prepare script, transitioned the Dockerfile to a multi-stage approach, and updated the setup.sh script to build the project dynamically from source instead.

Related Issue

Fixes #457

Changes

  • Add a prepare script to the root package.json to automatically compile dist when installing via GitHub URL.
  • Remove !nemoclaw/dist/ from .gitignore and untrack existing files (git rm -r --cached nemoclaw/dist/).
  • Update Dockerfile to utilize a multi-stage build, generating the plugin from source within a builder container rather than exclusively copying the host's dist/.
  • Refactor scripts/setup.sh to auto-build dist/ if missing locally instead of hard-failing, while removing the logic that aggressively deleted src files.

Type of Change

  • 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

  • make check passes.
  • npm test passes.
  • make docs builds without warnings. (for doc-only changes)

Checklist

General

Code Changes

  • make format applied (TypeScript and Python).
  • Tests added or updated for new or changed behavior.
  • 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

  • Follows the style guide. Try running the update-docs agent skill to draft changes while complying with the style guide. For example, prompt your agent with "/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.

Summary by CodeRabbit

  • Chores
    • Refactored Docker build process to use multi-stage builds for plugin compilation.
    • Updated build initialization scripts to automatically compile plugins when necessary.
    • Improved build configuration for streamlined setup workflow.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 20, 2026

📝 Walkthrough

Walkthrough

This change removes committed build artifacts by eliminating the .gitignore exception for nemoclaw/dist/, introduces a prepare npm lifecycle script to auto-build dist on install, updates the Dockerfile to build dist from source in a multi-stage build, and modifies the setup script to automatically generate dist if missing instead of failing.

Changes

Cohort / File(s) Summary
Build Artifact Tracking
.gitignore
Removed exception that previously unignored nemoclaw/dist/, allowing it to be ignored with all other dist/ directory contents.
Build Process Configuration
package.json
Added prepare lifecycle script that runs cd nemoclaw && npm install --ignore-scripts && npm run build to generate dist artifacts during package installation.
Docker Build
Dockerfile
Introduced multi-stage build with node:22-slim builder stage that installs NemoClaw dependencies and runs the build, then copies compiled dist/ output from builder stage instead of pre-built artifacts from build context.
Setup Script Logic
scripts/setup.sh
Removed unconditional deletion of nemoclaw/src and replaced hard-fail behavior when nemoclaw/dist is missing with auto-build logic that runs npm install and npm run build to generate artifacts, then cleans up node_modules to minimize context size.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A rabbit hops through git commits clear,
No dist artifacts cluttering here!
Build scripts prepare on each fresh clone,
The Dockerfile now builds its own—
Cleaner repos, less merge fray,
Better coding the rabbit way! 🌟

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main objectives: removing compiled nemoclaw/dist/ from git tracking and building from source instead.
Linked Issues check ✅ Passed All five coding requirements from issue #457 are fully implemented: prepare script added, .gitignore exception removed, Dockerfile converted to multi-stage build, and setup.sh updated with auto-build logic.
Out of Scope Changes check ✅ Passed All changes are directly aligned with issue #457 requirements; no unrelated modifications were introduced.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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

Tip

You can enable review details to help with troubleshooting, context usage and more.

Enable the reviews.review_details setting to include review details such as the model used, the time taken for each step and more in the review comments.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
scripts/setup.sh (1)

178-184: Consider adding --ignore-scripts for consistency.

The npm install here differs from package.json's prepare script which uses --ignore-scripts. This could cause nested lifecycle scripts to run unexpectedly.

♻️ Suggested fix for consistency
 if [ ! -d "$BUILD_CTX/nemoclaw/dist" ] || [ -z "$(ls -A "$BUILD_CTX/nemoclaw/dist" 2>/dev/null)" ]; then
   info "nemoclaw/dist/ is missing. Auto-building from source..."
-  (cd "$BUILD_CTX/nemoclaw" && npm install && npm run build)
+  (cd "$BUILD_CTX/nemoclaw" && npm install --ignore-scripts && npm run build)
   # Clean up node_modules to keep build context small
   rm -rf "$BUILD_CTX/nemoclaw/node_modules"
 fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/setup.sh` around lines 178 - 184, The npm install in the auto-build
block (the conditional that checks "$BUILD_CTX/nemoclaw/dist") can run lifecycle
scripts unexpectedly; change the install invocation used there to include
--ignore-scripts (i.e., use npm install --ignore-scripts) so it matches the
prepare script behavior while still running npm run build afterward (leave the
(cd "$BUILD_CTX/nemoclaw" && npm install && npm run build) flow but replace npm
install with npm install --ignore-scripts) and keep the subsequent rm -rf
cleanup as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@scripts/setup.sh`:
- Around line 178-184: The npm install in the auto-build block (the conditional
that checks "$BUILD_CTX/nemoclaw/dist") can run lifecycle scripts unexpectedly;
change the install invocation used there to include --ignore-scripts (i.e., use
npm install --ignore-scripts) so it matches the prepare script behavior while
still running npm run build afterward (leave the (cd "$BUILD_CTX/nemoclaw" &&
npm install && npm run build) flow but replace npm install with npm install
--ignore-scripts) and keep the subsequent rm -rf cleanup as-is.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 903eb678-87cb-46e1-8ef0-d60b4b0d7431

📥 Commits

Reviewing files that changed from the base of the PR and between dbfd78c and 4996cb1.

⛔ Files ignored due to path filters (76)
  • nemoclaw/dist/blueprint/exec.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/exec.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/blueprint/exec.js is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/exec.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/blueprint/fetch.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/fetch.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/blueprint/fetch.js is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/fetch.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/blueprint/resolve.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/resolve.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/blueprint/resolve.js is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/resolve.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/blueprint/state.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/state.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/blueprint/state.js is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/state.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/blueprint/verify.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/verify.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/blueprint/verify.js is excluded by !**/dist/**
  • nemoclaw/dist/blueprint/verify.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/cli.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/cli.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/cli.js is excluded by !**/dist/**
  • nemoclaw/dist/cli.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/connect.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/commands/connect.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/connect.js is excluded by !**/dist/**
  • nemoclaw/dist/commands/connect.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/eject.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/commands/eject.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/eject.js is excluded by !**/dist/**
  • nemoclaw/dist/commands/eject.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/launch.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/commands/launch.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/launch.js is excluded by !**/dist/**
  • nemoclaw/dist/commands/launch.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/logs.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/commands/logs.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/logs.js is excluded by !**/dist/**
  • nemoclaw/dist/commands/logs.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/migrate.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/commands/migrate.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/migrate.js is excluded by !**/dist/**
  • nemoclaw/dist/commands/migrate.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/migration-state.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/commands/migration-state.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/migration-state.js is excluded by !**/dist/**
  • nemoclaw/dist/commands/migration-state.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/onboard.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/commands/onboard.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/onboard.js is excluded by !**/dist/**
  • nemoclaw/dist/commands/onboard.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/slash.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/commands/slash.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/slash.js is excluded by !**/dist/**
  • nemoclaw/dist/commands/slash.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/status.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/commands/status.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/commands/status.js is excluded by !**/dist/**
  • nemoclaw/dist/commands/status.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/index.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/index.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/index.js is excluded by !**/dist/**
  • nemoclaw/dist/index.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/onboard/config.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/onboard/config.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/onboard/config.js is excluded by !**/dist/**
  • nemoclaw/dist/onboard/config.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/onboard/prompt.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/onboard/prompt.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/onboard/prompt.js is excluded by !**/dist/**
  • nemoclaw/dist/onboard/prompt.js.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/onboard/validate.d.ts is excluded by !**/dist/**
  • nemoclaw/dist/onboard/validate.d.ts.map is excluded by !**/dist/**, !**/*.map
  • nemoclaw/dist/onboard/validate.js is excluded by !**/dist/**
  • nemoclaw/dist/onboard/validate.js.map is excluded by !**/dist/**, !**/*.map
📒 Files selected for processing (4)
  • .gitignore
  • Dockerfile
  • package.json
  • scripts/setup.sh
💤 Files with no reviewable changes (1)
  • .gitignore

@wscurran wscurran added the enhancement New feature or request label Mar 20, 2026
@wscurran
Copy link
Copy Markdown
Contributor

Thanks for suggesting the removal of the compiled nemoclaw/dist/ directory and transitioning to a build-from-source approach, this could simplify our development process.

@arpankanwer arpankanwer closed this Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove committed dist/ build artifacts from git tracking

2 participants