New installer options #590

aarondill · 2023-07-05T18:09:54Z

This adds support for serveral new command line options (taken from the usage):

    -b, --bin-dir
                Override the bin installation directory [default: /home/aaron/.local/bin]

        -m, --man-dir
                Override the man installation directory [default: /home/aaron/.local/share/man]

        -a, --arch
                Override the architecture identified by the installer [default: x86_64-unknown-linux-musl]

        -s, --sudo
                Override the command used to elevate to root privaliges [default: sudo/doas]

        -h, --help
                Display this help message

This also (coincidentally) allows for $BIN_DIR, $MAN_DIR, $ARCH, and $SUDO to be set to create default settings for these.
These variables are always overwritten by the command line arguments.

This diff is unusually large, as my formatter (shfmt) ran, reformatting the majority of the script.

This also adds a few utilities to the script (log/abort) and modifies err to simply output to stderr.
This allows for more fine grain control over script flow and output, without relying on echo >&2.

Each commit should include descriptions regarding their necessity. If you don't understand something or just want some extra clarity, I'm happy to explain any of my choices.

NOTE:

These options are not yet fully tested. I am posting this here while I test on my own machine for the purposes of a second set of eyes, but this should not be merged until tested by at least two people 😄

This adds support for serveral new command line options (taken from the usage): "-b, --bin-dir" "Override the bin installation directory [default: ${_bin_dir}]" \ "-m, --man-dir" "Override the man installation directory [default: ${_man_dir}]" \ "-a, --arch" "Override the architecture identified by the installer [default: ${_arch}]" \ "-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 diff is unusually large, as my formatter ([shfmt](https://github.com/mvdan/sh)) ran, reformatting the majority of the script. This also adds a few utilities to the script (`log`/`abort`) and modifies `err` to simply output to stderr. This allows for more fine grain control over script flow and output, without relying on `echo >&2`.

This avoids logging the message when `--help` is specified, and helps ensure the correct output if `--arch` is specified

The default is *not* programatically generated in the help menu, as we don't determine this until later. This is intentional, so we can validate the user's choice of command.

This resets the indention to 4 spaces, reducing the size of the final diff to just code changes and *minor* formatting differences, reducing the effect of indention.

…h multiple collaborators

SC3043 is the warning about lack of `local` definition in POSIX. This is a Dev Exp improvement, as it removes unneeded and unwanted warnings.

aarondill · 2023-07-05T18:14:31Z

one known issue, which might be complicated to resolve, is that the use of sudo is based on the writability of _bin_dir, so if _man_dir is under "$HOME", this could result in files owned by root in the user's home directory.

This fixes a logic error, where `$_man_dir` is checked to ensure it exists, but not the `man1` subdirectory. It feels most reasonable to ensure the selected mandir exists, but not that the file structure of the directory is user-managed.

aarondill · 2023-07-05T18:18:37Z

one known issue, which might be complicated to resolve, is that the use of sudo is based on the writability of _bin_dir, so if _man_dir is under "$HOME", this could result in files owned by root in the user's home directory.

Additionally, if $_bin_dir is writable, but $_man_dir is not, then the script fails during man page installation. I think the only ways to resolve this is to reset the $_sudo variable before installing man pages, or to create two $_sudo variables (ie $_sudo/$_sudo_man)

This stops the script from breaking if the user specified sudo command outputs to stdout

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 ensures that mismatched permissions/ownership on `${_man_dir}` and `${_man_dir}/man1` don't cause the script to fail. *If* `${man_dir}/man1` already exists and is writable, OR `${_man_dir}/man1` *doesn't* yet exist, but `${_man_dir}` is writable, then the script continues without sudo. If `/man1` doesn't exist, and it's parent isn't writable, we need sudo. If `/man1` *does* exist, and it's not writable, we need sudo.

SUDO may not be set, if no parameter or environment variable is passed. This ensures that elevate_priv gets an empty string (as expected) in those cases

This was causing the script to always use sudo privilages if `/man1` already exists

*usually*, $PATH will *not* contain trailing slashes, while _bin_dir may or may not. This changes the case statement to test for `_bin_dir`, `_bin_dir/`, and `_bin_dir` without a trailing slash. This reduces false positives and unnecessary logs

The CI shellcheck linter requires brackets on variables, so this commit adds them

This prevents attempting to create files in `/` if a path is not given, instead failing early.

Thanks shfmt for pointing out these agressive simplifications that are possible, but not necisarily needed. Now can I pass CI? Please? 😄

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 pattern *is* POSIX defined (see [here](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02)): ``` Guideline 10: The first -- argument that is not an option-argument should be accepted as a delimiter indicating the end of options. Any following arguments should be treated as operands, even if they begin with the '-' character. ``` This ensures that odd values for _bin_name, _bin_dir, and others aren't treated as options to the command and cause a failure.

aarondill · 2023-07-05T19:40:30Z

I have now tested all the options (and they work on my system), and I have fixed the above mentioned error about _man_dir and _bin_dir permission mismatches.

@ajeetdsouza, Please take a look and let me know what you think. I would also appreciate if you could test these on your own system to avoid "it works on my machine" 😄

hanoii · 2023-09-13T15:20:52Z

I'd recommend re-applying your changes without shfmt. I stumbled upon this looking for finding a way to select the install directory.

Generally speaking, It's very hard to review a PR if you bring in more than what you are suggesting adding to the project. One should keep the general coding style and formatting of the project you are contributing to. If you think what shfmt brings is great a nice improvement, you can add that as an issue and discuss it with the maintainer but not add it as part of your attempted contributed work. This PR is very unlikely to be even considered with such a huge diff.

Same goes for:

This also adds a few utilities to the script (log/abort) and modifies err to simply output to stderr.
This allows for more fine grain control over script flow and output, without relying on echo >&2.

Please don't take this in the wrong way, I know that your intentions are great and that you put time into all of these improvements, only sharing my experience with open source and the general expectation of the open source module maintainers.

hanoii · 2023-09-13T15:39:01Z

For what it worth, I am doing:

curl -sS https://raw.githubusercontent.com/ajeetdsouza/zoxide/master/install.sh | HOME=/opt/zoxide bash && cp -R /opt/zoxide/.local/* /usr/local && rm -fr /opt/zoxide as a workaound to move the installed files to a directory of choice.

aarondill · 2024-01-23T10:09:08Z

I am closing this issue in favor of the rewrite at #672, without the formatting. Sorry for the time wasted on this PR, hopefully the new one is much more reasonable.

Note: I created a new PR instead of rebasing, because I rewrote it on a new branch, and the history is much neater with a new PR than with a bunch of force pushes / edits.

aarondill added 6 commits July 5, 2023 12:48

move the "detected architecture: ..." message to *after* parse_args

213f69f

This avoids logging the message when `--help` is specified, and helps ensure the correct output if `--arch` is specified

add -s/--sudo option to choose the sudo command

2d3c5bb

The default is *not* programatically generated in the help menu, as we don't determine this until later. This is intentional, so we can validate the user's choice of command.

reformat with shift_width=4

86be4f4

This resets the indention to 4 spaces, reducing the size of the final diff to just code changes and *minor* formatting differences, reducing the effect of indention.

add a vim modeline to end of document to ensure proper formatting wit…

bd3dd1d

…h multiple collaborators

add explaination for shellcheck disable

c9a4e52

SC3043 is the warning about lack of `local` definition in POSIX. This is a Dev Exp improvement, as it removes unneeded and unwanted warnings.

ensure _man_dir/man1/ folder exists before copy

aaa1418

This fixes a logic error, where `$_man_dir` is checked to ensure it exists, but not the `man1` subdirectory. It feels most reasonable to ensure the selected mandir exists, but not that the file structure of the directory is user-managed.

aarondill added 11 commits July 5, 2023 13:23

fix error due to using wrong command name for has_cmd function

ebebf77

use RETVAL for elevate_priv

606b2f4

This stops the script from breaking if the user specified sudo command outputs to stdout

reset $sudo when writing man pages.

b92ff04

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

FIX: SUDO: parameter not set

68b2240

SUDO may not be set, if no parameter or environment variable is passed. This ensures that elevate_priv gets an empty string (as expected) in those cases

fix typo /man1} -> /man1

dff0050

This was causing the script to always use sudo privilages if `/man1` already exists

handle slashes at end of _bin_dir

98bf021

*usually*, $PATH will *not* contain trailing slashes, while _bin_dir may or may not. This changes the case statement to test for `_bin_dir`, `_bin_dir/`, and `_bin_dir` without a trailing slash. This reduces false positives and unnecessary logs

add brackets to variables to pass lint

e011aba

The CI shellcheck linter requires brackets on variables, so this commit adds them

create safeguard in test_writable

b0ab15c

This prevents attempting to create files in `/` if a path is not given, instead failing early.

"Simplify" script

20246a6

Thanks shfmt for pointing out these agressive simplifications that are possible, but not necisarily needed. Now can I pass CI? Please? 😄

change installation instructions

45d41f2

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.

aarondill mentioned this pull request Jul 5, 2023

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

Closed

ajeetdsouza force-pushed the main branch 3 times, most recently from 4a7505d to 073500c Compare October 10, 2023 08:58

ajeetdsouza force-pushed the main branch from 1f435ab to afdcf6b Compare November 19, 2023 07:59

aarondill mentioned this pull request Jan 23, 2024

New installer options (v2) #672

Merged

aarondill closed this Jan 23, 2024

aarondill deleted the new-installer branch January 23, 2024 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New installer options #590

New installer options #590

aarondill commented Jul 5, 2023

aarondill commented Jul 5, 2023

aarondill commented Jul 5, 2023

aarondill commented Jul 5, 2023

hanoii commented Sep 13, 2023 •

edited

hanoii commented Sep 13, 2023 •

edited

aarondill commented Jan 23, 2024

New installer options #590

New installer options #590

Conversation

aarondill commented Jul 5, 2023

NOTE:

aarondill commented Jul 5, 2023

aarondill commented Jul 5, 2023

aarondill commented Jul 5, 2023

hanoii commented Sep 13, 2023 • edited

hanoii commented Sep 13, 2023 • edited

aarondill commented Jan 23, 2024

hanoii commented Sep 13, 2023 •

edited

hanoii commented Sep 13, 2023 •

edited