Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow user specification of bin_dir, bin_name, and man_dir #561

Closed
wants to merge 3 commits into from

Conversation

aarondill
Copy link
Contributor

This allows the user to choose the installation directory, the name of the executable, and the man directory using the environment variables BINDIR, BINNAME, and MANDIR, respectively.

@ajeetdsouza ajeetdsouza force-pushed the main branch 2 times, most recently from c8fb2f2 to 5de13be Compare May 5, 2023 22:25
@ajeetdsouza
Copy link
Owner

Could you explain your use case?

  • bin_name should always be zoxide, or none of the shell plugins would work.
  • bin_dir and man_dir have been set according to XDG standards. With the current changes, one could only set them to another directory in $HOME - most other directories would require sudo, so the use case here is also very limited.
  • Is there any other curl | bash installer that provides these options?

@ajeetdsouza ajeetdsouza added the waiting-for-response Waiting for a response from the issue author. label May 6, 2023
@aarondill
Copy link
Contributor Author

Could you explain your use case?

My use case is a global install (Ie, /usr/local/{bin,man}), which, for my personal single-user computer, I would prefer, as I backup anything in ~/.local/bin to a GH repo, and I would rather run a script to update zoxide on a new machine than keep an old version in a repo.

bin_name should always be zoxide, or none of the shell plugins would work.

That's understandable, I didn't think about that particular use case. It seems reasonable to enforce the binary name anyways since it is the identity of the command.

bin_dir and man_dir have been set according to XDG standards.

While that's great for most users (myself generally included), there are many situations where it's better to let the user that knows what they are doing do what they want. Perhaps these directories aren't in their PATH (and MANPATH if set), or they don't have write access to ~/.local/{bin,man} (odd, but possible), or they want a global install ( me :) ), or maybe they just have an intense hatred for the XDG standards. Regardless of the reasoning, we should allow them to install to another location if they'd like to.

With the current changes, one could only set them to another directory in $HOME - most other directories would require sudo, so the use case here is also very limited.

While the script in the current installation recommendations couldn't write to directories without permission, the user can just as easily run curl ... | sudo bash to allow it to (not generally recommended, but neither is running a script from the internet).

Is there any other curl | bash installer that provides these options?

Here's a short list of well known installers that provide options to set installation directories. Sadly, I couldn't find any examples with man pages, as most curl ... | {ba,}sh installers don't install man pages with their commands. Not all of these directly interact with their environment (delegating to something else), but from a user point of view, it's the same as if they did.

  • The starship installer (https://starship.rs/install.sh) does it through a option when running the script (ie curl ... | sh -s -- -b BINDIR). It also tries to elevate permissions with sudo, but that's optional and beside the point.
  • The pnpm installer respects the environment variable PNPM_HOME. (Installs to a tmpdir and delegates to $tmpdir/pnpm setup, which respect the variable, but is the same from user view)
  • The bun installer respects the BUN_INSTALL variable for installation directory
  • The nvm installer respects the NVM_DIR variable for installation directory
  • The Rust installer (called rustup) allows you to change installation directory with RUSTUP_HOME and CARGO_HOME (see: index.html#choosing-where-to-install )
  • The pyenv installer respects the PYENV_ROOT variable for installation directory
  • The Oh My Zsh installer respects the ZSH variable for destination directory.

While there is little to no convention (sadly), quite a few other well known installers allow these settings, as it's a quite useful option. Additionally, these could be prefixed with Z or ZOXIDE_, to distinguish them, if desired, but either way, documentation should be added (happy to add it when it's decided on).

@aarondill
Copy link
Contributor Author

aarondill commented May 7, 2023

I will note, and perhaps this should be a seperate issue/PR, that the installation directions call for running curl ... | bash, but the install script has a #!/bin/sh shebang, which is ignored when run with an explicit bash implication, so should be corrected to #!/bin/bash as it's purely ornamental anyways.

Perhaps an error should even be displayed and execution halted if not running in bash, as it's explicitly stated to run it in bash. (or else retrofit it to run in sh and change the directions)

PS. # shellcheck shell=dash should also be changed to bash, as it's running in bash, not dash.

@ajeetdsouza ajeetdsouza removed the waiting-for-response Waiting for a response from the issue author. label May 7, 2023
This allows the user to choose the installation directory, the name of the executable, and the man directory using the environment variables BINDIR, BINNAME, and MANDIR, respectively.
This forces _bin_name to always be zoxide
@aarondill
Copy link
Contributor Author

support for BINNAME has now been removed, and this has been rebased onto main.

@ajeetdsouza
Copy link
Owner

Sorry about the delay in replying, I've been incredibly busy.

Perhaps an error should even be displayed and execution halted if not running in bash, as it's explicitly stated to run it in bash. (or else retrofit it to run in sh and change the directions)

The script is meant to be POSIX compliant, with the exception of the local keyword. Most of the script has been taken from the official Rust installer which uses sh, so I suppose the instructions could be updated accordingly.

Here's a short list of well known installers that provide options to set installation directories.

That was very thorough, thank you!

Personally, I think the Starship installer should be the way to go here. Command-line options are relatively more discoverable (using --help) than environment variables. This might be a larger PR, but would you be willing to take something like this up?

  • Change the installation instructions to use curl .. | sh instead of bash
  • -h or --help for usage
  • --bin-dir and --man-dir to configure bin and man directories
  • Privilege escalation via sudo (and optionally doas) if required for installation

@aarondill
Copy link
Contributor Author

@ajeetdsouza These have all been implemented in #590. I am closing this issue as that one take it's place.

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.

None yet

2 participants