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

New installer options (v2) #672

Merged
merged 14 commits into from May 5, 2024
Merged

Conversation

aarondill
Copy link
Contributor

This is a rewrite of #590 to reduce the diff and make it a more reasonable pr.

Add support for options in install script

This adds support for serveral new command line options (taken from the usage):
- -b, --bin-dir Override the bin installation directory
- -m, --man-dir Override the man installation directory
- -a, --arch Override the architecture identified by the installer
- -h, --help Display this help message
This also (coincidentally) allows for $BIN_DIR, $MAN_DIR, and $ARCH to be set to create default settings for these.
These variables are always overwritten by the command line arguments.

This adds support for serveral new command line options (taken from the usage):
    - `-b, --bin-dir   Override the bin installation directory`
    - `-m, --man-dir   Override the man installation directory`
    - `-a, --arch      Override the architecture identified by the installer`
    - `-h, --help      Display this help message`
This also (coincidentally) allows for $BIN_DIR, $MAN_DIR, and $ARCH to be set to create default settings for these.
These variables are *always* overwritten by the command line arguments.
This helps ensure that mismatched permission on `_bin_dir` and `_man_dir` don't cause issues.
Also prevents creating man pages owned by root being written to the user's $HOME
This prevents attempting to create files in `/` if a path is not given, instead failing early.
They now suggest passing to `sh`, rather than `bash`, to match the shebang and reduce posibility that non-posix behaivor of `bash` affects the script.

this was requested by @ajeetdsouza
Since the installation instructions now recommend using `sh`, check that this implementation *does* support local before continuing.
This ensures that odd values for _bin_name, _bin_dir, and others aren't treated as options to the command and cause a failure.
Remove the display of this default value, since we can't detect it without erroring before usage.
@aarondill aarondill mentioned this pull request Jan 23, 2024
@aarondill
Copy link
Contributor Author

@ajeetdsouza I hope I've accomplished what you expected when we first discussed this (almost a year ago) here.
If anything is missing, or you have any changes you'd like made, please let me know 😄

the ci requires that you run `shfmt -w --indent=4 --language-dialect=posix --simplify *.sh'`, and as such, i now have.

note: I also formatted the case statement in parse_args to fit on one screen while still being readable.
@rseichter
Copy link

I used the official install script for the first time today, because of the outdated Debian 11 package. Right away I was surprised by the hard-coded $HOME/.local/{bin,man} directories. While this was easy enough to fix manually, I would appreciate being able to pass the desired directories (or in my case a directory prefix /usr/local) as either a installer script argument or environment variable. I checked the list of Zoxide pull requests and found this one, so I'd like to vote in favour.

@ajeetdsouza ajeetdsouza merged commit 801d5e2 into ajeetdsouza:main May 5, 2024
4 checks passed
@rseichter
Copy link

Thank you for the new configuration options. They make automation easier for me, and I have already updated my jobs accordingly.

@ajeetdsouza
Copy link
Owner

Thank you @aarondill for the PR.

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

3 participants