fix: improve local binary build robustness - #1324
Conversation
- Fall through to Cargo.toml when Makefile has no build target (fixes asupersync) - Auto-trust mise.toml before installing tools (fixes symphony) - Init git submodules before building if .gitmodules exists - Provide ICU headers via nix for CGo builds (fixes beads)
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe script now adds mise trust verification before installation, conditionally initializes git submodules, checks for Makefile build targets before execution (with fallback warnings), and optionally injects ICU compilation settings for Go builds via nix-build environment variables. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
✨ 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;DRImproved robustness of local binary builds for various projects by addressing issues like missing build targets, untrusted What changed?
Description generated by Mesa. Update settings |
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of d7b52b4...1fa3d8b
Analysis
• Build system priority logic has shifted—when Makefile exists but lacks a build target, the system now falls through to attempt Cargo/Go builds. This changes the original clear priority hierarchy (Makefile → Cargo → Go) and could cause multiple build attempts in hybrid projects, wasting resources and creating ambiguous error states.
• Build-time requirements are accumulating in a bash script (mise trust, git submodules, ICU headers, Nix dependencies) without structured orchestration. The function is growing in complexity and this scattered approach makes it difficult to test, maintain, and understand actual build prerequisites. This belongs in a more disciplined build configuration layer.
• ICU header setup introduces a hard Nix dependency into the build script, creating an assumption that Nix is available and configured. While the codebase appears Nix-based, this tightens coupling and reduces portability without clear documentation of this requirement.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 1 comments | Edit Agent Settings • Read Docs
| elif [ -f "$build_dir/Cargo.toml" ]; then | ||
| fi | ||
|
|
||
| if [ -f "$build_dir/Cargo.toml" ]; then |
There was a problem hiding this comment.
The control flow change from elif to if means that if a Makefile exists but has no build target, the script will now try both Cargo.toml AND go.mod builds (if both exist). This could lead to confusing behavior where multiple build systems are attempted for the same project. Consider adding early returns or making the checks mutually exclusive to maintain the original intent of trying one build system at a time.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#1324
File: scripts/update-local-binaries.sh#L174
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The control flow change from `elif` to `if` means that if a Makefile exists but has no build target, the script will now try both Cargo.toml AND go.mod builds (if both exist). This could lead to confusing behavior where multiple build systems are attempted for the same project. Consider adding early returns or making the checks mutually exclusive to maintain the original intent of trying one build system at a time.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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 enhances the update-local-binaries.sh script by adding support for mise trust, initializing git submodules, and improving build system fallback logic when a Makefile lacks a build target. It also introduces automated detection and configuration of ICU dependencies for Go builds using Nix. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
Improves the robustness of scripts/update-local-binaries.sh when building a variety of local repos by adding safer fallbacks and pre-build setup steps (mise trust/install, submodule init, and Go ICU env support).
Changes:
- Run
mise trustbeforemise installwhenmise.tomlis present. - Initialize git submodules automatically when
.gitmodulesexists. - If a Makefile lacks a
buildtarget, fall through to Cargo/Go builds; additionally provide ICU-related env vars for Go/CGo builds via Nix.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (cd "$build_dir" && mise trust 2>&1) || true | ||
| (cd "$build_dir" && mise install 2>&1) || true |
There was a problem hiding this comment.
mise trust can require interactive confirmation; running it without a non-interactive flag risks hanging (or failing) and then mise install may still run with an untrusted mise.toml. Consider using a non-interactive option (e.g., --yes/equivalent) and/or only invoking trust when the file is currently untrusted.
| (cd "$build_dir" && mise trust 2>&1) || true | |
| (cd "$build_dir" && mise install 2>&1) || true | |
| if (cd "$build_dir" && mise trust --yes 2>&1); then | |
| (cd "$build_dir" && mise install 2>&1) || true | |
| else | |
| log_warn " Skipping mise install because 'mise trust' failed or was not confirmed" | |
| fi |
| # Provide ICU headers for CGo builds that need them (e.g. go-icu-regex) | ||
| local icu_dev | ||
| icu_dev="$(nix-build '<nixpkgs>' -A icu.dev --no-out-link 2>/dev/null || true)" | ||
| local icu_lib | ||
| icu_lib="$(nix-build '<nixpkgs>' -A icu --no-out-link 2>/dev/null || true)" | ||
| local go_env=() |
There was a problem hiding this comment.
The nix-build '<nixpkgs>' ... calls run for every Go module, which can significantly slow down builds and may fail on flake-based setups where NIX_PATH/<nixpkgs> isn't configured. Consider guarding this behind command -v nix/nix-build, and/or only resolving ICU via Nix as a fallback retry when the initial go build fails due to missing ICU/pkg-config headers (or use nix build nixpkgs#icu{,.dev} to avoid relying on <nixpkgs>).
| else | ||
| return 1 | ||
| fi | ||
| else |
There was a problem hiding this comment.
With the Makefile handling split into its own if, a repo that has a Makefile (but no build target) and also lacks Cargo.toml/go.mod will later hit the fallback branch that logs "No Makefile, Cargo.toml, or go.mod found". That warning becomes misleading; consider updating the fallback message or adding a dedicated fallback for the "Makefile present but no supported build targets/files" case.
| else | |
| else | |
| log_warn " Makefile has no build target." | |
| # If there is no Cargo.toml or go.mod either, there's nothing we know how to build here. | |
| if [ ! -f "$build_dir/Cargo.toml" ] && [ ! -f "$build_dir/go.mod" ]; then | |
| log_warn " Makefile has no 'build' target and no Cargo.toml or go.mod found; nothing to build in $build_dir." | |
| return 1 | |
| fi |
Summary
buildtarget but hasCargo.toml— now falls through to cargo whenmake buildtarget doesn't existmise.tomlnot trusted — now auto-runsmise trustbeforemise install.gitmodulesexistsPKG_CONFIG_PATH/CGO_CFLAGS/CGO_LDFLAGSvia nixicu.dev.gitmodulesentry) — upstream issue, can't fixTest plan
make shell-testpasses (no new failures)make formatclean (0 changed)🤖 Generated with Claude Code
Summary by cubic
Make local binary builds more robust. Adds a
makefallback tocargo, auto-trustsmise.toml, initializes submodules, and wires ICU headers for CGo.Makefilehas nobuildtarget, fall back tocargo build --releasewhenCargo.tomlexists.mise trustbeforemise installwhenmise.tomlis present.git submoduleswhen.gitmodulesexists.goprojects needing ICU, setPKG_CONFIG_PATH,CGO_CFLAGS, andCGO_LDFLAGSvianixicu/icu.dev.Written for commit 1fa3d8b. Summary will update on new commits.