Skip to content

fix(install): default to ~/.local/bin for unprivileged installs#77

Open
stevensmith37 wants to merge 1 commit intomainfrom
install_non_priv
Open

fix(install): default to ~/.local/bin for unprivileged installs#77
stevensmith37 wants to merge 1 commit intomainfrom
install_non_priv

Conversation

@stevensmith37
Copy link
Collaborator

Summary

Change the default INSTALL_DIR from /usr/local/bin to $HOME/.local/bin so the script works without sudo. Also move mkdir -p before the privilege check so a non-existent directory doesn't fail the writability test.

Changes

  • Changes the INSTALL_DIR to a location that doesn't require admin permissions to write to.
  • Changes the location of the directory creation to before the binary installation.

Test plan

  • make build passes
  • make test passes
  • CI green

Rollback plan

Revert this PR.

Change the default INSTALL_DIR from /usr/local/bin to $HOME/.local/bin
so the script works without sudo. Also move mkdir -p before the
privilege check so a non-existent directory doesn't fail the writability
test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Owner

@mrmaxsteel mrmaxsteel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Bug: mkdir -p before check_privileges defeats the privilege check

The PR moves mkdir -p "$INSTALL_DIR" before check_privileges(). The problem is that check_privileges() tests [ ! -w "$INSTALL_DIR" ] — if mkdir -p just created the directory, it will always be writable by the current user, so the privilege check will never fail, even when the user sets INSTALL_DIR to somewhere they shouldn't be writing (e.g., INSTALL_DIR=/usr/local/bin).

The original code had mkdir -p after the privilege check intentionally — it only creates the directory once we've confirmed we have write access to the parent.

The real fix for "non-existent directory fails the writability test" is to check writability of the parent directory when the target doesn't exist yet, not to create the directory preemptively. Something like:

check_privileges() {
    TARGET="$INSTALL_DIR"
    # If the directory doesn't exist, check the closest existing parent
    while [ ! -d "$TARGET" ]; do
        TARGET=$(dirname "$TARGET")
    done
    if [ ! -w "$TARGET" ]; then
        ...
    fi
}

Minor: ~/.local/bin may not be on $PATH

Many systems (macOS in particular) don't have ~/.local/bin on $PATH by default. The script checks command -v mindspec after install, which would fail and show a confusing success-then-not-found experience. Worth adding a PATH hint after install if $INSTALL_DIR isn't on $PATH.

Summary

The default change to ~/.local/bin is a good idea, but the mkdir -p reorder introduces a bug that silently bypasses the privilege check. Recommend fixing check_privileges to walk up to the nearest existing parent directory instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants