fix(build): repoint Makefile at the live checkout - #473
Conversation
REPO pointed at /Users/tindang/workspaces/tind-repo/moon — the stale second checkout CLAUDE.md warns about, sitting at #126 (hash-ttl era) while main is at #472. Every target built, tested and ran ~350 PRs of the wrong code. The dangerous one is `make ci`, which advertises CI parity and would report green against a tree nobody is shipping. Three fixes: - REPO -> /Volumes/Games/tindang-repo/moon, with a comment naming the stale path so the next person does not "restore" it. - CARGO_TARGET_DIR=target-linux on every cargo target. The VM and the host compile the same shared checkout, so without this the Linux ELF artifacts clobber the macOS Mach-O ones (CLAUDE.md requires it). Declared with `export ... &&` rather than a bare VAR=x prefix, which would only have applied to the first command of each `&&` chain — `make ci` and `make clippy` each run two cargo invocations. - `pkill -x moon` -> `pkill -f "$(REPO)/target-linux/release/moon"`. Matching by bare process name kills any process called `moon`, not just the one this Makefile built. Verified: `make fmt` runs clean end-to-end in the moon-dev VM against the live checkout; `make -n ci` shows the export surviving the full && chain. author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe Makefile now exports ChangesLinux target isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🟡 Moderate · up to The updated development commands may fail to stop the server they previously launched, leaving stale processes running and causing subsequent starts to fail when ports are already occupied. The executable path should be defined once and used consistently for both launching and stopping before merge. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 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 `@Makefile`:
- Around line 59-68: Update the start, start-bg, start-ephemeral, and stop
targets to define and use one absolute runtime MOON_BIN path for both launching
and process matching. Anchor the pkill pattern to the executable command at the
beginning, construct it from MOON_BIN so the wrapper shell is not matched, and
invoke the binary through the same absolute value in every start target.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| $(ORB) '$(CARGO_ENV) && cd $(REPO) && pkill -f "$(REPO)/target-linux/release/moon" 2>/dev/null; sleep 0.5; mkdir -p $(DATA_DIR) && ./target-linux/release/moon --port $(PORT) --shards $(SHARDS) --admin-port $(ADMIN_PORT) $(PERSIST_ARGS) $(EXTRA_ARGS)' | ||
|
|
||
| start-bg: build | ||
| $(ORB) '$(CARGO_ENV) && cd $(REPO) && pkill -x moon 2>/dev/null; sleep 0.5; mkdir -p $(DATA_DIR) && nohup ./target/release/moon --port $(PORT) --shards $(SHARDS) --admin-port $(ADMIN_PORT) $(PERSIST_ARGS) $(EXTRA_ARGS) &>/tmp/moon.log & echo "Moon started (PID $$!), log: /tmp/moon.log"' | ||
| $(ORB) '$(CARGO_ENV) && cd $(REPO) && pkill -f "$(REPO)/target-linux/release/moon" 2>/dev/null; sleep 0.5; mkdir -p $(DATA_DIR) && nohup ./target-linux/release/moon --port $(PORT) --shards $(SHARDS) --admin-port $(ADMIN_PORT) $(PERSIST_ARGS) $(EXTRA_ARGS) &>/tmp/moon.log & echo "Moon started (PID $$!), log: /tmp/moon.log"' | ||
|
|
||
| start-ephemeral: build | ||
| $(ORB) '$(CARGO_ENV) && cd $(REPO) && pkill -x moon 2>/dev/null; sleep 0.5; ./target/release/moon --port $(PORT) --shards $(SHARDS) --admin-port $(ADMIN_PORT) $(EXTRA_ARGS)' | ||
| $(ORB) '$(CARGO_ENV) && cd $(REPO) && pkill -f "$(REPO)/target-linux/release/moon" 2>/dev/null; sleep 0.5; ./target-linux/release/moon --port $(PORT) --shards $(SHARDS) --admin-port $(ADMIN_PORT) $(EXTRA_ARGS)' | ||
|
|
||
| stop: | ||
| $(ORB) 'pkill -x moon 2>/dev/null && echo "Moon stopped" || echo "Moon not running"' | ||
| $(ORB) 'pkill -f "$(REPO)/target-linux/release/moon" 2>/dev/null && echo "Moon stopped" || echo "Moon not running"' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the process match use the same executable path as the launch.
pkill -f matches the full command line. The server is launched as ./target-linux/release/moon, but the match uses /Volumes/Games/tindang-repo/moon/target-linux/release/moon. The existing server is therefore not reliably matched. start* can leave the old process running and then fail to bind its ports. stop can report the wrong state.
Use one absolute runtime MOON_BIN value for both matching and launching. Anchor the match. Construct the pattern at runtime, or compare /proc/<pid>/exe, so the wrapper shell is not matched by its own command text.
Based on the supplied Makefile: the termination pattern and launch command use different path forms.
Safer command shape
bin="/Volumes/Games/tindang-repo/moon/target-linux/release/moon"
pkill -f -- "^${bin}([[:space:]]|$)" 2>/dev/null || true
"${bin}" ...🤖 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 `@Makefile` around lines 59 - 68, Update the start, start-bg, start-ephemeral,
and stop targets to define and use one absolute runtime MOON_BIN path for both
launching and process matching. Anchor the pkill pattern to the executable
command at the beginning, construct it from MOON_BIN so the wrapper shell is not
matched, and invoke the binary through the same absolute value in every start
target.
1 participant
REPOpointed at/Users/tindang/workspaces/tind-repo/moon— the stale second checkout CLAUDE.md warns about, at #126 (hash-ttl era) while main is at #472. Every target built, tested and ran ~350 PRs of the wrong code. The dangerous one ismake ci: it advertises CI parity and would report green against a tree nobody is shipping.Fixes
REPO→/Volumes/Games/tindang-repo/moon, with a comment naming the stale path so it doesn't get "restored".CARGO_TARGET_DIR=target-linuxon every cargo target — the VM and host compile the same shared checkout, so without it Linux ELF artifacts clobber the macOS Mach-O ones (required by CLAUDE.md). Declared asexport … &&rather than a bareVAR=xprefix, which would only apply to the first command of each chain —make ciandmake clippyeach run two cargo invocations.pkill -x moon→pkill -f "$(REPO)/target-linux/release/moon"— matching by bare process name kills any process namedmoon, not just the one this Makefile built.Verified
make fmtruns clean end-to-end in the moon-dev VM against the live checkout.make -n cishows the export surviving the full&&chain.No
src/changes; dev tooling only.Summary by CodeRabbit