Build from source and remove compiled nemoclaw/dist/ from repository#461
Build from source and remove compiled nemoclaw/dist/ from repository#461arpankanwer wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughThis change removes committed build artifacts by eliminating the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/setup.sh (1)
178-184: Consider adding--ignore-scriptsfor consistency.The
npm installhere differs frompackage.json'spreparescript 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
⛔ Files ignored due to path filters (76)
nemoclaw/dist/blueprint/exec.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/exec.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/exec.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/exec.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/fetch.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/fetch.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/fetch.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/fetch.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/resolve.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/resolve.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/resolve.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/resolve.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/state.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/state.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/state.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/state.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/verify.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/verify.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/verify.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/verify.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/cli.d.tsis excluded by!**/dist/**nemoclaw/dist/cli.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/cli.jsis excluded by!**/dist/**nemoclaw/dist/cli.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/connect.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/connect.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/connect.jsis excluded by!**/dist/**nemoclaw/dist/commands/connect.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/eject.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/eject.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/eject.jsis excluded by!**/dist/**nemoclaw/dist/commands/eject.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/launch.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/launch.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/launch.jsis excluded by!**/dist/**nemoclaw/dist/commands/launch.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/logs.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/logs.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/logs.jsis excluded by!**/dist/**nemoclaw/dist/commands/logs.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/migrate.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/migrate.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/migrate.jsis excluded by!**/dist/**nemoclaw/dist/commands/migrate.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/migration-state.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/migration-state.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/migration-state.jsis excluded by!**/dist/**nemoclaw/dist/commands/migration-state.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/onboard.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/onboard.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/onboard.jsis excluded by!**/dist/**nemoclaw/dist/commands/onboard.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/slash.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/slash.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/slash.jsis excluded by!**/dist/**nemoclaw/dist/commands/slash.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/status.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/status.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/status.jsis excluded by!**/dist/**nemoclaw/dist/commands/status.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/index.d.tsis excluded by!**/dist/**nemoclaw/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/index.jsis excluded by!**/dist/**nemoclaw/dist/index.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/config.d.tsis excluded by!**/dist/**nemoclaw/dist/onboard/config.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/config.jsis excluded by!**/dist/**nemoclaw/dist/onboard/config.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/prompt.d.tsis excluded by!**/dist/**nemoclaw/dist/onboard/prompt.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/prompt.jsis excluded by!**/dist/**nemoclaw/dist/onboard/prompt.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/validate.d.tsis excluded by!**/dist/**nemoclaw/dist/onboard/validate.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/validate.jsis excluded by!**/dist/**nemoclaw/dist/onboard/validate.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (4)
.gitignoreDockerfilepackage.jsonscripts/setup.sh
💤 Files with no reviewable changes (1)
- .gitignore
|
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. |
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 apreparescript, transitioned theDockerfileto a multi-stage approach, and updated thesetup.shscript to build the project dynamically from source instead.Related Issue
Fixes #457
Changes
preparescript to the rootpackage.jsonto automatically compiledistwhen installing via GitHub URL.!nemoclaw/dist/from.gitignoreand untrack existing files (git rm -r --cached nemoclaw/dist/).Dockerfileto utilize a multi-stage build, generating the plugin from source within a builder container rather than exclusively copying the host'sdist/.scripts/setup.shto auto-builddist/if missing locally instead of hard-failing, while removing the logic that aggressively deletedsrcfiles.Type of Change
Testing
make checkpasses.npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
make formatapplied (TypeScript and Python).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Summary by CodeRabbit