feat(local-binaries): add module to sync local binaries to PATH #470
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| # AI | ||
| .claude | ||
|
|
||
| # Local binaries list (machine-specific) | ||
| .local-binaries.txt | ||
| .devenv.nix | ||
| objectstore | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| [ | ||
| ./local-binaries | ||
| ./npm-globals | ||
| ./tailscale | ||
| ./yek | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { config, pkgs, ... }: | ||
| { | ||
| # Create ~/.local/bin directory | ||
| home.file.".local/bin/.keep".text = ""; | ||
|
|
||
| # Symlink local binaries during activation | ||
| home.activation.symlinkLocalBinaries = config.lib.dag.entryAfter [ "writeBoundary" ] '' | ||
| $DRY_RUN_CMD ${pkgs.bash}/bin/bash ${./sync-local-binaries.sh} | ||
| ''; | ||
| } | ||
|
Comment on lines
+1
to
+10
There was a problem hiding this comment. 🛠️ Refactor suggestion | 🟠 Major Module lacks proper option declarations and documentation. Per home-manager module structure guidelines, custom modules should include:
Currently, the module always activates without user control and has no configuration options. As per coding guidelines: "Custom modules should include proper option types and document all options" and "Each module in 🔎 Proposed module structure{ config, lib, pkgs, ... }:
with lib;
let
cfg = config.programs.local-binaries;
in
{
options.programs.local-binaries = {
enable = mkEnableOption "local binaries synchronization";
binariesFile = mkOption {
type = types.str;
default = "${config.home.homeDirectory}/dotfiles/.local-binaries.txt";
description = "Path to the file containing binary paths to symlink";
};
binDir = mkOption {
type = types.str;
default = "${config.home.homeDirectory}/.local/bin";
description = "Directory where symlinks will be created";
};
};
config = mkIf cfg.enable {
# Create bin directory
home.file."${cfg.binDir}/.keep".text = "";
# Symlink local binaries during activation
home.activation.symlinkLocalBinaries =
config.lib.dag.entryAfter [ "writeBoundary" ] ''
$DRY_RUN_CMD ${pkgs.bash}/bin/bash ${./sync-local-binaries.sh} \
"${cfg.binariesFile}" "${cfg.binDir}"
'';
};
}Note: This also requires updating 🤖 Prompt for AI Agents |
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,47 @@ | ||||||||||||||||||||||||||||||||||
| #!/usr/bin/env bash | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| set -euo pipefail | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Sync local binaries from ~/.local-binaries.txt to ~/.local/bin | ||||||||||||||||||||||||||||||||||
| # Each line in the file should be an absolute path to a binary | ||||||||||||||||||||||||||||||||||
| # Lines starting with # are comments, empty lines are ignored | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| BINARIES_FILE="${HOME}/dotfiles/.local-binaries.txt" | ||||||||||||||||||||||||||||||||||
| BIN_DIR="${HOME}/.local/bin" | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+9
to
+10
There was a problem hiding this comment. Hardcoded "dotfiles" directory name reduces portability. The path Consider one of these approaches:
🔎 Example: Use script location-BINARIES_FILE="${HOME}/dotfiles/.local-binaries.txt"
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+REPO_ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd)"
+BINARIES_FILE="$REPO_ROOT/.local-binaries.txt"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Exit if no binaries file exists | ||||||||||||||||||||||||||||||||||
| if [ ! -f "$BINARIES_FILE" ]; then | ||||||||||||||||||||||||||||||||||
| echo "No ${BINARIES_FILE} found, skipping local binaries sync" | ||||||||||||||||||||||||||||||||||
| exit 0 | ||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Ensure bin directory exists | ||||||||||||||||||||||||||||||||||
| mkdir -p "$BIN_DIR" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Process each line in the binaries file | ||||||||||||||||||||||||||||||||||
| while IFS= read -r line || [ -n "$line" ]; do | ||||||||||||||||||||||||||||||||||
| # Trim leading and trailing whitespace | ||||||||||||||||||||||||||||||||||
| line="${line#"${line%%[![:space:]]*}"}" | ||||||||||||||||||||||||||||||||||
| line="${line%"${line##*[![:space:]]}"}" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Skip empty lines | ||||||||||||||||||||||||||||||||||
| [ -z "$line" ] && continue | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Skip comments | ||||||||||||||||||||||||||||||||||
| case "$line" in | ||||||||||||||||||||||||||||||||||
| \#*) continue ;; | ||||||||||||||||||||||||||||||||||
| esac | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Check if binary exists and is executable | ||||||||||||||||||||||||||||||||||
| if [ ! -f "$line" ] || [ ! -x "$line" ]; then | ||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. P2: Missing validation for absolute paths. The script documents that paths should be absolute but doesn't enforce it. A relative path that happens to resolve could create a broken symlink. Consider adding validation after the comment check: case "$line" in
/*) ;;
*) echo "Skipping (not an absolute path): $line"; continue ;;
esacPrompt for AI agents
|
||||||||||||||||||||||||||||||||||
| echo "Skipping (not found or not executable): $line" | ||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Get binary name and create symlink | ||||||||||||||||||||||||||||||||||
| bin_name="$(basename "$line")" | ||||||||||||||||||||||||||||||||||
| target="$BIN_DIR/$bin_name" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| ln -sf "$line" "$target" | ||||||||||||||||||||||||||||||||||
| echo "Linked: $bin_name -> $line" | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+41
to
+46
There was a problem hiding this comment. Basename conflicts are not detected or reported. If multiple binaries in Consider warning users when basename conflicts occur: 🔎 Proposed fix # Get binary name and create symlink
bin_name="$(basename "$line")"
target="$BIN_DIR/$bin_name"
+
+ if [ -L "$target" ] && [ "$(readlink "$target")" != "$line" ]; then
+ echo "Warning: $bin_name already links to $(readlink "$target"), overwriting with $line"
+ fi
ln -sf "$line" "$target"
echo "Linked: $bin_name -> $line"📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||
| done <"$BINARIES_FILE" | ||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| #!/usr/bin/env bash | ||
| # shellcheck disable=SC2329 | ||
|
|
||
| Describe 'local-binaries/sync-local-binaries.sh' | ||
| SCRIPT="$PWD/home-manager/modules/local-binaries/sync-local-binaries.sh" | ||
|
|
||
| Describe 'script properties' | ||
| It 'uses bash shebang' | ||
| When run bash -c "head -1 '$SCRIPT'" | ||
| The output should include '#!/usr/bin/env bash' | ||
| End | ||
|
|
||
| It 'uses strict mode' | ||
| When run bash -c "head -5 '$SCRIPT'" | ||
| The output should include 'set -euo pipefail' | ||
| End | ||
| End | ||
|
|
||
| Describe 'configuration' | ||
| It 'reads from dotfiles/.local-binaries.txt' | ||
| When run bash -c "grep 'BINARIES_FILE=' '$SCRIPT'" | ||
| The output should include 'dotfiles/.local-binaries.txt' | ||
| End | ||
|
|
||
| It 'writes to ~/.local/bin' | ||
| When run bash -c "grep 'BIN_DIR=' '$SCRIPT'" | ||
| The output should include '.local/bin' | ||
| End | ||
| End | ||
|
|
||
| Describe 'file handling' | ||
| It 'exits gracefully when binaries file is missing' | ||
| When run bash -c "grep -A 2 'if \[ ! -f' '$SCRIPT'" | ||
| The output should include 'exit 0' | ||
| End | ||
|
|
||
| It 'creates bin directory if needed' | ||
| When run bash -c "grep 'mkdir -p' '$SCRIPT'" | ||
| The output should include 'mkdir -p' | ||
| End | ||
| End | ||
|
|
||
| Describe 'line processing' | ||
| It 'skips empty lines' | ||
| When run bash -c "grep -E '\\[ -z' '$SCRIPT'" | ||
| The output should include 'continue' | ||
| End | ||
|
|
||
| It 'skips comment lines' | ||
| When run bash -c "grep -A 2 'Skip comments' '$SCRIPT'" | ||
| The output should include 'continue' | ||
| End | ||
|
|
||
| It 'checks if binary exists and is executable' | ||
| When run bash -c "grep '\[ ! -f' '$SCRIPT'" | ||
| The output should include '! -x' | ||
| End | ||
| End | ||
|
|
||
| Describe 'symlink creation' | ||
| It 'uses basename for symlink name' | ||
| When run bash -c "grep 'basename' '$SCRIPT'" | ||
| The output should include 'basename' | ||
| End | ||
|
|
||
| It 'creates symlinks with ln -sf' | ||
| When run bash -c "grep 'ln -sf' '$SCRIPT'" | ||
| The output should include 'ln -sf' | ||
| End | ||
| End | ||
|
|
||
| End | ||
|
Comment on lines
+4
to
+72
Contributor
There was a problem hiding this comment. The tests in this file check the implementation details of the script by For more robust and maintainable tests, I recommend switching to behavioral tests. You could use
This approach tests what the script does, not how it's written, which is much more valuable. Describe 'sync-local-binaries.sh behavior'
setup() {
# Set up a temporary environment for testing
HOME="$SHELLSPEC_TMPDIR"
export HOME
BINARIES_FILE="$HOME/dotfiles/.local-binaries.txt"
BIN_DIR="$HOME/.local/bin"
mkdir -p "$(dirname "$BINARIES_FILE")"
# Create a dummy executable
DUMMY_BIN_PATH="$SHELLSPEC_TMPDIR/my-test-binary"
echo '#!/bin/sh' > "$DUMMY_BIN_PATH"
chmod +x "$DUMMY_BIN_PATH"
}
Before 'setup'
It 'creates a symlink for a valid binary path'
# Arrange
echo "$DUMMY_BIN_PATH" > "$BINARIES_FILE"
# Act
When run "$SCRIPT"
# Assert
The status should be success
The path "$BIN_DIR/my-test-binary" should be a symlink to "$DUMMY_BIN_PATH"
The output should include "Linked: my-test-binary -> $DUMMY_BIN_PATH"
End
End |
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No error handling for sync script failure.
If
sync-local-binaries.shencounters an error (despiteset -e), the activation continues silently. Users won't be notified of synchronization failures.Consider whether script failures should:
-e)-e, add explicit error logging)The current approach with
set -euo pipefailwill fail activation on any error, which may be too strict if a single invalid binary prevents the entire home-manager switch.