refactor(makefile): extract host detection into a script - #2107
refactor(makefile): extract host detection into a script#2107shunkakinoki wants to merge 1 commit into
Conversation
Machine detection was a 27-line $(shell ...) blob inline in the Makefile, testable only indirectly by driving 'make build' with a mocked nix and grepping the resulting command log. Move it to scripts/detect-host.sh, which reads OS, ARCH, DMI_SYS_VENDOR, DMI_PRODUCT_NAME and RUNPOD_POD_ID from the environment so spec/detect_host_spec.sh can exercise detection directly. Add 'make detect-host' to show what the current machine maps to. Also collapse the HOST -> DETECTED_HOST -> fallback chain, which was repeated 11 times across nix-build and nix-switch, onto a single RESOLVED_HOST variable. That duplication is what caused #2105: #2100 updated the non-CI copies and missed the CI ones. The two changes touch the same block, so they land together. The guard-style checks ('$(DETECTED_HOST)' = x || '$(HOST)' = x) and the build-vm/build-iso targets keep using DETECTED_HOST directly -- switching those to RESOLVED_HOST would change their semantics.
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesHost resolution and Nix integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Makefile
participant DetectHostScript as scripts/detect-host.sh
participant NixWorkflow as nix-build/nix-switch
Makefile->>DetectHostScript: detect host from OS and machine identity
DetectHostScript-->>Makefile: return detected host or empty result
Makefile->>Makefile: prefer HOST over DETECTED_HOST as RESOLVED_HOST
Makefile->>NixWorkflow: select configuration using RESOLVED_HOST
NixWorkflow-->>Makefile: build or switch selected configuration
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Mesa DescriptionTL;DRExtracted inline Makefile machine detection into a dedicated, testable script and unified host resolution into a single What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the machine detection logic for automatic host mapping out of the Makefile and into a dedicated bash script scripts/detect-host.sh, while adding a comprehensive test suite in spec/detect_host_spec.sh. It also introduces a RESOLVED_HOST variable to simplify host selection across build and switch targets. The review feedback highlights potential shell syntax issues in the Makefile when passing variables inline that might contain single quotes, suggesting using export instead. Additionally, it recommends replacing echo with printf in the bash script when outputting system-provided variables to ensure robust handling of hyphens and backslashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| DETECTED_HOST := $(shell OS='$(OS)' ARCH='$(ARCH)' \ | ||
| DMI_SYS_VENDOR='$(DMI_SYS_VENDOR)' DMI_PRODUCT_NAME='$(DMI_PRODUCT_NAME)' \ | ||
| bash ./scripts/detect-host.sh) |
There was a problem hiding this comment.
Passing variables inline to $(shell ...) using single quotes can break the shell syntax if any of the variables (especially DMI_PRODUCT_NAME or DMI_SYS_VENDOR which are read from system files) contain single quotes (e.g., Shun's Laptop).
Instead of passing them inline, you can use the export directive in the Makefile. This makes the variables automatically available in the environment of the $(shell ...) subshell, eliminating any quoting or escaping issues entirely.
export OS ARCH DMI_SYS_VENDOR DMI_PRODUCT_NAME
DETECTED_HOST := $(shell bash ./scripts/detect-host.sh)
|
|
||
| local computer_name | ||
| computer_name="$(scutil --get ComputerName 2>/dev/null || true)" | ||
| if echo "$computer_name" | grep -q "Shun's MacBook M4"; then |
There was a problem hiding this comment.
Using echo to output arbitrary system-provided variables (like computer_name) can be problematic if the value starts with a hyphen (which echo might interpret as an option) or contains backslashes. Using printf '%s\n' is the standard, robust way to output variable values safely.
| if echo "$computer_name" | grep -q "Shun's MacBook M4"; then | |
| if printf '%s\n' "$computer_name" | grep -q "Shun's MacBook M4"; then |
| if [ "$DMI_SYS_VENDOR" = "Framework" ] && | ||
| echo "$DMI_PRODUCT_NAME" | grep -q "Laptop 13.*AMD Ryzen AI 300"; then |
There was a problem hiding this comment.
Using echo to output arbitrary system-provided variables (like DMI_PRODUCT_NAME) can be problematic if the value starts with a hyphen or contains backslashes. Using printf '%s\n' is the standard, robust way to output variable values safely.
| if [ "$DMI_SYS_VENDOR" = "Framework" ] && | |
| echo "$DMI_PRODUCT_NAME" | grep -q "Laptop 13.*AMD Ryzen AI 300"; then | |
| if [ "$DMI_SYS_VENDOR" = "Framework" ] && | |
| printf '%s\n' "$DMI_PRODUCT_NAME" | grep -q "Laptop 13.*AMD Ryzen AI 300"; then |
![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 `@scripts/detect-host.sh`:
- Around line 20-31: Update the machine-probe logic used by detect_darwin_host
and the related hostname detection to accept environment overrides for whoami,
scutil ComputerName, and hostname, falling back to the existing commands only
when each override is unset. Update spec/detect_host_spec.sh to explicitly
neutralize or set these overrides for deterministic Linux and Darwin detection
cases.
🪄 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: fb6334ca-1de7-498c-986a-6d3848b7887f
📒 Files selected for processing (3)
Makefilescripts/detect-host.shspec/detect_host_spec.sh
| detect_darwin_host() { | ||
| if [ "$(whoami 2>/dev/null || true)" != "shunkakinoki" ] || [ "$ARCH" != "arm64" ]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| local computer_name | ||
| computer_name="$(scutil --get ComputerName 2>/dev/null || true)" | ||
| if echo "$computer_name" | grep -q "Shun's MacBook M4"; then | ||
| echo "galactica" | ||
| fi | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the machine probes injectable for deterministic detection tests.
The script still reads whoami, scutil, and hostname directly. Consequently, the Linux specs expecting DMI or empty output can fail on a machine named kyber or matic, while the positive Darwin mapping cannot be isolated in tests.
Accept environment overrides for these values, falling back to the commands only when the overrides are unset; then explicitly neutralize/set them in spec/detect_host_spec.sh.
Also applies to: 39-46
🤖 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 `@scripts/detect-host.sh` around lines 20 - 31, Update the machine-probe logic
used by detect_darwin_host and the related hostname detection to accept
environment overrides for whoami, scutil ComputerName, and hostname, falling
back to the existing commands only when each override is unset. Update
spec/detect_host_spec.sh to explicitly neutralize or set these overrides for
deterministic Linux and Darwin detection cases.
1 participant
Follow-up to #2105.
Detection moves to a script
Machine detection was a 27-line
$(shell ...)blob inline in the Makefile. It was testable only indirectly, by drivingmake buildwith a mockednixand grepping the resulting command log.It now lives in
scripts/detect-host.sh, matching the existingscripts/*.sh+spec/*_spec.shconvention. It readsOS,ARCH,DMI_SYS_VENDOR,DMI_PRODUCT_NAMEandRUNPOD_POD_IDfrom the environment, sospec/detect_host_spec.shcan exercise detection directly instead of through a build. That spec covers the Framework 13 DMI match, the RunPod override, a non-matching Framework model, matching product data from another vendor, and the various no-host cases.make detect-hostprints what the current machine maps to:Note on shape: detection can't be a make target that the Makefile itself invokes —
DETECTED_HOST := $(shell $(MAKE) ...)would re-enter the Makefile during parsing and recurse. So the script is what does the work, anddetect-hostis a thin target over it.Resolution collapses onto one variable
The
HOST->DETECTED_HOST-> fallback chain was repeated 11 times acrossnix-buildandnix-switch, once per (CI x non-CI) x (Darwin, nixos, home). That duplication is exactly what caused #2105: #2100 updated the non-CI copies and missed the CI ones.All 11 now read a single
RESOLVED_HOST := $(or $(HOST),$(DETECTED_HOST)), which halves the branch count and makes it structurally impossible to update one arm without the other. This is in the same commit because it rewrites the same block.One behavior note: the Darwin branches previously ran the
NIXOS_NAMED_HOSTScheck only for an explicitHOST, and always builtdarwinConfigurationsfor a detected host. Now both go through the case. This is equivalent, because the Darwin detection path only ever yieldsgalactica, which is not inNIXOS_NAMED_HOSTSand so falls to the same default branch.Deliberately not changed
The guard-style checks (
[ "$(DETECTED_HOST)" = x ] || [ "$(HOST)" = x ]) and thebuild-vm/run-vm/build-isotargets still useDETECTED_HOSTdirectly. Switching those toRESOLVED_HOSTwould change their semantics — e.g.make ... HOST=viperon the galactica machine currently satisfies agalacticaguard, and would stop doing so. That is arguably more correct, but it is a real behavior change and doesn't belong in a refactor.Verification
shellspecfull suite: 1645 examples, 0 failuresCI=true shellspecfull suite: 1645 examples, 0 failuresmake shell-check(repo shellcheck): cleanmake detect-hoston this machine still resolvesgalactica, matching pre-refactor behaviorSummary by cubic
Extracted host detection from the Makefile into
scripts/detect-host.shand unified host selection viaRESOLVED_HOST. This removes duplicated logic innix-build/nix-switch, makes detection testable, and keeps behavior the same.Refactors
$(shell ...)blob withscripts/detect-host.shreadingOS,ARCH,DMI_SYS_VENDOR,DMI_PRODUCT_NAME,RUNPOD_POD_ID.HOST/DETECTED_HOSTintoRESOLVED_HOST := $(or $(HOST),$(DETECTED_HOST))used across CI and non-CI paths.RESOLVED_HOST; behavior unchanged since detection only yieldsgalactica(not inNIXOS_NAMED_HOSTS).build-vm/run-vm/build-isoonDETECTED_HOSTto avoid behavior changes.New Features
make detect-hostto print detected and resolved host.spec/detect_host_spec.shcovering Framework 13, RunPod override,kyber/matic, and no-host cases.Written for commit 65043cc. Summary will update on new commits.