feat: add local binaries update system - #629
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR refactors the dotfiles update workflow by introducing a new cross-platform automated updater service and restructuring Makefile targets. It adds platform-specific (Darwin/Linux) periodic updater services, introduces a script for managing local binary builds, reorganizes Makefile targets (update → update-local-binaries, nix-update → nix-upgrade), and includes comprehensive test coverage for new components. Changes
Sequence DiagramssequenceDiagram
participant Timer as Scheduler<br/>(launchd/systemd)
participant Service as Updater Service<br/>(Agent/Systemd)
participant UpdateScript as update.sh
participant Make as Make System
Timer->>Service: Trigger (3-hour interval)
Service->>Service: Set augmented PATH<br/>Set AUTOMATED_UPDATE=true
Service->>UpdateScript: Execute with bash
UpdateScript->>UpdateScript: cd ~/dotfiles
UpdateScript->>Make: make update
Make->>Make: Execute update targets
Make-->>UpdateScript: Return status
UpdateScript-->>Service: Exit with code
Service->>Service: Log output to temp files
sequenceDiagram
participant Make as Makefile
participant UpdateBin as update-local-binaries.sh
participant GHQ as ghq
participant Repo as Local Repository
participant Build as Build System<br/>(Make/Cargo)
Make->>UpdateBin: make update-local-binaries
UpdateBin->>UpdateBin: Parse .local-binaries.txt
UpdateBin->>UpdateBin: Determine repo path<br/>(ghq layout)
UpdateBin->>GHQ: ghq get (if missing)
GHQ-->>Repo: Clone repository
UpdateBin->>Repo: git pull
UpdateBin->>Repo: Detect build system<br/>(Makefile or Cargo.toml)
Repo->>Build: Execute make/cargo build
Build-->>Repo: Build completes
UpdateBin->>UpdateBin: Track success/failure
UpdateBin->>UpdateBin: Print colored summary
UpdateBin-->>Make: Exit with status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 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 |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's maintenance capabilities by introducing a robust and automated system for managing and updating local binaries. It centralizes the process of fetching the latest code and rebuilding these binaries, streamlining developer workflows and ensuring dependencies are current. The changes also improve the clarity and organization of the project's build and update mechanisms through thoughtful Makefile refactoring and comprehensive testing, contributing to a more maintainable and efficient development environment. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRAdded a local binaries update system with an automated home-manager service. 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 introduces a new system for updating local binaries, which is a great addition. The changes are well-structured, with a new script, a home-manager service, and corresponding Makefile targets. The refactoring of update and upgrade targets in the Makefile improves clarity. The addition of comprehensive tests is also commendable. I've found a few issues, mostly related to correctness in the new script and service configuration, and some opportunities for code simplification. Overall, this is a solid contribution.
| lib.makeBinPath [ | ||
| pkgs.git | ||
| pkgs.bash | ||
| pkgs.coreutils | ||
| pkgs.gnumake | ||
| ] |
There was a problem hiding this comment.
The launchd agent's PATH is missing dependencies required by the update script. The script may need to build Rust projects with cargo or clone repositories with ghq. This will fail if they are not in the PATH. Please add pkgs.rustc (for cargo) and pkgs.ghq to the lib.makeBinPath list.
lib.makeBinPath [
pkgs.bash
pkgs.coreutils
pkgs.ghq
pkgs.git
pkgs.gnumake
pkgs.rustc
]
| lib.makeBinPath [ | ||
| pkgs.bash | ||
| pkgs.coreutils | ||
| pkgs.curl | ||
| pkgs.gawk | ||
| pkgs.git | ||
| pkgs.gnumake | ||
| pkgs.gnused | ||
| pkgs.nix | ||
| pkgs.sudo | ||
| pkgs.which | ||
| ] |
There was a problem hiding this comment.
The systemd service's PATH is missing dependencies required by the update script. The script may need to build Rust projects with cargo or clone repositories with ghq. This will fail if they are not in the PATH. Please add pkgs.rustc (for cargo) and pkgs.ghq to the lib.makeBinPath list.
lib.makeBinPath [
pkgs.bash
pkgs.coreutils
pkgs.curl
pkgs.gawk
pkgs.ghq
pkgs.git
pkgs.gnumake
pkgs.gnused
pkgs.nix
pkgs.rustc
pkgs.sudo
pkgs.which
]
| done | ||
|
|
||
| # Fallback: assume 4 levels deep from ghq root (github.com/owner/repo) | ||
| echo "$binary_path" | sed -E 's|(~/ghq/[^/]+/[^/]+/[^/]+)/.*|\1|' | sed "s|~|$HOME|" |
There was a problem hiding this comment.
The fallback logic in get_repo_dir is broken. At the beginning of the function, binary_path has ~ expanded to $HOME, but the sed command attempts to match a literal ~, which will fail. The second sed is also redundant.
To fix this, use double quotes in the sed pattern to allow for $HOME expansion and remove the second sed call.
| echo "$binary_path" | sed -E 's|(~/ghq/[^/]+/[^/]+/[^/]+)/.*|\1|' | sed "s|~|$HOME|" | |
| echo "$binary_path" | sed -E "s|($HOME/ghq/[^/]+/[^/]+/[^/]+)/.*|\1|" |
|
|
||
| .PHONY: install | ||
| install: setup nix-flake-update nix-build nix-switch shell-install ## Set up full environment (setup, flake-update, build, switch, shell-install). | ||
| install: setup nix-build nix-switch shell-install ## Set up full environment (setup, flake-update, build, switch, shell-install). |
There was a problem hiding this comment.
The comment for the install target is out of sync with its dependencies. It still mentions flake-update, but nix-flake-update has been removed from the target. Please update the comment to accurately reflect the target's behavior.
install: setup nix-build nix-switch shell-install ## Set up full environment (setup, build, switch, shell-install).
| if [ -f "$repo_dir/Makefile" ]; then | ||
| if make -C "$repo_dir" build 2>&1; then | ||
| return 0 | ||
| else | ||
| return 1 | ||
| fi | ||
| elif [ -f "$repo_dir/Cargo.toml" ]; then | ||
| if (cd "$repo_dir" && cargo build --release 2>&1); then | ||
| return 0 | ||
| else | ||
| return 1 | ||
| fi | ||
| else | ||
| log_warn " No Makefile or Cargo.toml found, skipping build" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
The if ...; then return 0; else return 1; fi pattern used for build commands is verbose. You can simplify this by letting the build command's exit code propagate directly. The calling function's if statement will correctly interpret a zero exit code as success and non-zero as failure.
| if [ -f "$repo_dir/Makefile" ]; then | |
| if make -C "$repo_dir" build 2>&1; then | |
| return 0 | |
| else | |
| return 1 | |
| fi | |
| elif [ -f "$repo_dir/Cargo.toml" ]; then | |
| if (cd "$repo_dir" && cargo build --release 2>&1); then | |
| return 0 | |
| else | |
| return 1 | |
| fi | |
| else | |
| log_warn " No Makefile or Cargo.toml found, skipping build" | |
| return 0 | |
| fi | |
| if [ -f "$repo_dir/Makefile" ]; then | |
| make -C "$repo_dir" build 2>&1 | |
| elif [ -f "$repo_dir/Cargo.toml" ]; then | |
| (cd "$repo_dir" && cargo build --release 2>&1) | |
| else | |
| log_warn " No Makefile or Cargo.toml found, skipping build" | |
| return 0 | |
| fi |
There was a problem hiding this comment.
Pull request overview
This pull request adds a local binaries update system that automates the process of updating and rebuilding local development binaries. The system consists of a script that reads binary paths from .local-binaries.txt, updates their repositories, and rebuilds them using their respective build systems (Make or Cargo).
Changes:
- Added
scripts/update-local-binaries.shfor automated binary updates with git pull and rebuild functionality - Added
make-updaterhome-manager service to run updates every 3 hours on both macOS and Linux - Reorganized Makefile targets: renamed
upgrade-overlays→overlays-upgrade,nix-update→nix-upgrade, and redefinedupdateto focus on local binaries - Added comprehensive shell tests with 487 lines covering script functionality
- Updated ShellCheck exclusions to skip
.claudedirectories
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/update-local-binaries.sh | New script implementing local binary update logic with path detection, git operations, and build system detection |
| home-manager/services/make-updater/update.sh | Service wrapper script that runs make update from the dotfiles directory |
| home-manager/services/make-updater/default.nix | Service configuration for both macOS (launchd) and Linux (systemd) with 3-hour interval |
| home-manager/services/default.nix | Integration of make-updater service into home-manager configuration |
| spec/update_local_binaries_spec.sh | Comprehensive test suite for the update script (487 lines) |
| spec/make_updater_spec.sh | Basic tests for the make-updater service script |
| spec/coverage_spec.sh | Added test coverage entries for new scripts |
| Makefile | Reorganized Update/Upgrade sections, renamed targets for consistency, added update-local-binaries target, updated ShellCheck exclusions |
| .local-binaries.txt | Fixed gastown entry to include binary path /gt |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| It 'finds repo root by .git directory' | ||
| When run bash "$TEMP_SCRIPT" "$TEMP_DIR/ghq/github.com/owner/repo/target/release/mybin" | ||
| The output should eq "$TEMP_DIR/ghq/github.com/owner/repo" | ||
| End |
There was a problem hiding this comment.
The test only covers the case where the .git directory is found. The fallback path logic at line 81 in the main script should also be tested, especially since it has a bug where it tries to match the tilde pattern after the path has already been expanded.
| pkgs.coreutils | ||
| pkgs.gnumake | ||
| ] | ||
| }:/opt/homebrew/bin:/usr/local/bin"; |
There was a problem hiding this comment.
The Linux systemd service sets AUTOMATED_UPDATE=true but the macOS launchd service doesn't set this environment variable. This creates inconsistent behavior between platforms - on Linux, certain operations will be skipped (like flake updates per the Makefile logic) but on macOS they won't be. Consider setting this variable for both platforms or documenting why the difference is intentional.
| }:/opt/homebrew/bin:/usr/local/bin"; | |
| }:/opt/homebrew/bin:/usr/local/bin"; | |
| AUTOMATED_UPDATE = "true"; |
| done | ||
|
|
||
| # Fallback: assume 4 levels deep from ghq root (github.com/owner/repo) | ||
| echo "$binary_path" | sed -E 's|(~/ghq/[^/]+/[^/]+/[^/]+)/.*|\1|' | sed "s|~|$HOME|" |
There was a problem hiding this comment.
The fallback sed pattern uses ~/ghq/ but at this point in the function (line 66), the tilde has already been expanded to $HOME. The pattern should use $HOME/ghq/ or the expanded path format instead. This will cause the fallback to fail when no .git directory is found in the path hierarchy.
| echo "$binary_path" | sed -E 's|(~/ghq/[^/]+/[^/]+/[^/]+)/.*|\1|' | sed "s|~|$HOME|" | |
| local ghq_root | |
| ghq_root="$(get_ghq_root)" | |
| echo "$binary_path" | sed -E "s|(${ghq_root}/[^/]+/[^/]+/[^/]+)/.*|\1|" |
|
|
||
| .PHONY: install | ||
| install: setup nix-flake-update nix-build nix-switch shell-install ## Set up full environment (setup, flake-update, build, switch, shell-install). | ||
| install: setup nix-build nix-switch shell-install ## Set up full environment (setup, flake-update, build, switch, shell-install). |
There was a problem hiding this comment.
The help comment for the install target mentions "flake-update" but the target definition no longer includes nix-flake-update (which was removed in this change). The comment should be updated to accurately reflect the current dependencies.
| install: setup nix-build nix-switch shell-install ## Set up full environment (setup, flake-update, build, switch, shell-install). | |
| install: setup nix-build nix-switch shell-install ## Set up full environment (setup, build, switch, shell-install). |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="home-manager/services/make-updater/default.nix">
<violation number="1" location="home-manager/services/make-updater/default.nix:18">
P2: macOS launchd PATH omits sed/gnused, but make update requires sed, causing the mac updater to fail</violation>
</file>
<file name="scripts/update-local-binaries.sh">
<violation number="1" location="scripts/update-local-binaries.sh:81">
P2: Fallback repo detection never matches expanded paths, returning the binary path and causing ghq clone/pull against an invalid repo path when .git is missing</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| lib.makeBinPath [ | ||
| pkgs.git | ||
| pkgs.bash | ||
| pkgs.coreutils |
There was a problem hiding this comment.
P2: macOS launchd PATH omits sed/gnused, but make update requires sed, causing the mac updater to fail
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/make-updater/default.nix, line 18:
<comment>macOS launchd PATH omits sed/gnused, but make update requires sed, causing the mac updater to fail</comment>
<file context>
@@ -0,0 +1,68 @@
+ lib.makeBinPath [
+ pkgs.git
+ pkgs.bash
+ pkgs.coreutils
+ pkgs.gnumake
+ ]
</file context>
| done | ||
|
|
||
| # Fallback: assume 4 levels deep from ghq root (github.com/owner/repo) | ||
| echo "$binary_path" | sed -E 's|(~/ghq/[^/]+/[^/]+/[^/]+)/.*|\1|' | sed "s|~|$HOME|" |
There was a problem hiding this comment.
P2: Fallback repo detection never matches expanded paths, returning the binary path and causing ghq clone/pull against an invalid repo path when .git is missing
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/update-local-binaries.sh, line 81:
<comment>Fallback repo detection never matches expanded paths, returning the binary path and causing ghq clone/pull against an invalid repo path when .git is missing</comment>
<file context>
@@ -0,0 +1,251 @@
+ done
+
+ # Fallback: assume 4 levels deep from ghq root (github.com/owner/repo)
+ echo "$binary_path" | sed -E 's|(~/ghq/[^/]+/[^/]+/[^/]+)/.*|\1|' | sed "s|~|$HOME|"
+}
+
</file context>
Changes
scripts/update-local-binaries.shto update local binaries from.local-binaries.txtmake-updaterfor automated updatesupgrade-overlaystooverlays-upgrade, renamenix-updatetonix-upgrade, addupdate-local-binariestarget.claudedirectories from ShellCheckTechnical Details
Testing
Generated with opencode by claude-sonnet-4.5
Summary by cubic
Adds an automated system to update local binaries listed in .local-binaries.txt via a new script and a Home Manager timered service. Also simplifies Makefile targets and adds tests to cover the flow.
New Features
Refactors
Written for commit fc29411. Summary will update on new commits.