Add support for installing binaries #1413

crodjer · 2023-11-27T18:44:59Z

As noted in a existing TODO, this installs the atuin binary directly at /usr/local/bin for operating systems that don't have an official method. For example: Raspberry Pi / MacOS (without homebrew).

Steps:

Get latest release info to find out the version.
Get the release archive and extract it to a temporary directory.
Move the atuin binary to PATH.

This is based on the installation script
for sysit - a small utility I made.

Also, add support for custom binary installation path. It can be an additional argument to the install command. For instance:

INSTALL_DIR=~/.local/bin bash <(curl https://raw.githubusercontent.com/ellie/atuin/main/install.sh)

to install the binary in ~/.local/bin

vercel · 2023-11-27T18:45:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
atuin-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 24, 2023 7:29am

ellie

Thanks! Just a couple of comments

ellie · 2023-11-29T09:03:01Z

install.sh

+    local arch=$(uname -sm)
+    local bin_dir=/usr/local/bin
+
+    # todo: Can make it an optional argument to install script as well,


could we do this now? would be really great to have!

Sure, that's something I can add. I had it in the patch but wasn't sure if it is okay to change the interface for the script.

ellie · 2023-11-29T09:04:19Z

install.sh

+    local binary_url="https://github.com/atuinsh/atuin/releases/download/v17.0.1/$archive_name.tar.gz"
+    download_dir=$(mktemp -d)
+    curl -s -L $binary_url | tar -xz -C $download_dir -f -
+    sudo mkdir -p $bin_dir


I think it would be good to

Let the user know what's going on (eg tell them we couldn't find anything + will install a binary from releases)

Tell them where it's being installed, and ask for y/n to go ahead

I don't think we should go putting files in the users filesystem without getting their confirmation that is OK, especially when they'll be getting a sudo prompt without explanation

True. I think I can add logging here, with a prompt.

crodjer · 2023-11-29T16:08:44Z

@ellie, made the suggested changes.

crodjer · 2023-12-06T11:01:51Z

@ellie did you get a chance to look at it?

ellie

Just a couple more things

ellie · 2023-12-09T17:31:57Z

install.sh

@@ -34,6 +34,8 @@ LATEST_RELEASE=$(curl -L -s -H 'Accept: application/json' https://github.com/atu
 # Allow sed; sometimes it's more readable than ${variable//search/replace}
 # shellcheck disable=SC2001
 LATEST_VERSION=$(echo "$LATEST_RELEASE" | sed -e 's/.*"tag_name":"\([^"]*\)".*/\1/')
+# For binary install, use the first argument as the installation directory.
+INSTALL_DIRECTORY=${1:-/usr/local/bin}


I'd really rather we just made this an env var input. That way we don't change the interface at all, but still allow customization.

This makes sense.

ellie · 2023-12-09T17:32:41Z

install.sh

+
+    local latest_version=$(curl -s https://api.github.com/repos/atuinsh/atuin/releases/latest | grep tag_name  | grep -Eo "v[0-9]+\.[0-9]+\.[0-9]+")
+    local archive_name=atuin-$latest_version-$target
+    local binary_url="https://github.com/atuinsh/atuin/releases/download/v17.0.1/$archive_name.tar.gz"


This hardcodes v17.0.1

Made a few updates for the review, shellcheck and a small fix for darwin's stat utility.

@ellie , did you get a chance to see the changes?

As noted in a existing TODO, this installs the `atuin` binary directly at `/usr/local/bin` for operating systems that don't have an official method. For example: Raspberry Pi / MacOS (without homebrew). Steps: 1. Get latest release info to find out the version. 2. Get the release archive and extract it to a temporary directory. 3. Move the `atuin` binary to PATH. > This is based on the [installation script](https://github.com/crodjer/sysit/blob/main/scripts/install.sh) > for `sysit` - a small utility I made.

The optional first argument to the installer can specify the install directory other than `/usr/local/bin`.

- Don't change installation interface, use `INSTALL_DIR` instead. - Fix all the shellcheck errors. - Remove the hard coded 17.0.1 - Fix `stat` usage for `darwin`

ellie · 2023-12-23T16:22:29Z

@crodjer looks like the prompt I mentioned in a previous review isn't here?

I think it would be good to

Let the user know what's going on (eg tell them we couldn't find anything + will install a binary from releases)
Tell them where it's being installed, and ask for y/n to go ahead

I really don't want to go installing files on the users system without confirmation. It would be good to tell them what files are going to be downloaded where, and also explain how they can change that.

Also - the real fix for Ubuntu is to also build a deb for aarch64 and install with the proper tooling, not to install a binary manually. This should be a last resort.

For scripting usage (say ansible playbooks), setting ATUIN_NO_PROMPT will ensure no prompt is shown.

crodjer · 2023-12-24T07:30:45Z

looks like the prompt I mentioned in a previous review isn't here?

@ellie Yes, I figured we could rely on the sudo call. But you are right, users may miss out on seeing the prompt if sudo authorization is cached. So, added an explicit prompt as well. It'll behave like this:

Will copy atuin binary to /usr/local/bin...
Okay? [Y/n]: Y
Downloading atuin v17.1.0 from the latest github release...
Copying atuin binary to /usr/local/bin ...

I didn't want an explicit prompt because it'll then affect my scripts. For that, I have added another environment variable: ATUIN_NO_PROMPT. My goal toward having a binary install support was primarily to ensure my automation works on all my machines.

ellie · 2024-02-26T12:33:30Z

Longer term, I think the approach for installing binaries should be one of the following

Use the users' package manager wherever possible
Fallback to installing a binary to ~/.atuin or similar, request the user add to PATH
Provide instructions for manual install to /usr/local/bin, ~/.local/bin, etc. These paths differ across distributions, platforms, and individual setups.

I am generally quite against automatically installing to known binary paths. This is the job of a package manager, and there are almost certainly many edge cases we have not considered. This can also conflict with existing package managers and other installs, which is not something we want.

Especially in the case of /usr/local/bin, I'd rather avoid calling sudo (unless it's needed to call a package manager).

My goal toward having a binary install support was primarily to ensure my automation works on all my machines.

For this specific case, you should be able to use ansible/saltstack/etc to automate a manual binary install.

crodjer · 2024-03-01T18:15:40Z

I understand. Lately I haven't been using atuin, so I didn't ping further for review.

I discovered https://github.com/cargo-bins/cargo-binstall, so cargo binstall atuin should be able to serve a similar use-case.
You might consider adding that as an option to the doc.

vercel bot deployed to Preview November 27, 2023 18:45 View deployment

ellie requested changes Nov 29, 2023

View reviewed changes

vercel bot deployed to Preview November 29, 2023 15:50 View deployment

ellie requested changes Dec 9, 2023

View reviewed changes

vercel bot deployed to Preview December 10, 2023 14:55 View deployment

crodjer force-pushed the binary-installer branch from 305b7fe to 54e34d2 Compare December 10, 2023 15:00

vercel bot deployed to Preview December 10, 2023 15:00 View deployment

crodjer added 2 commits December 10, 2023 20:30

Add logging and custom install directory in binary installer

5f8378d

The optional first argument to the installer can specify the install directory other than `/usr/local/bin`.

crodjer force-pushed the binary-installer branch from 54e34d2 to ae871ee Compare December 10, 2023 15:00

vercel bot deployed to Preview December 10, 2023 15:01 View deployment

crodjer force-pushed the binary-installer branch from ae871ee to b17811e Compare December 10, 2023 15:12

vercel bot deployed to Preview December 10, 2023 15:12 View deployment

Review and shellcheck fixes

9442813

- Don't change installation interface, use `INSTALL_DIR` instead. - Fix all the shellcheck errors. - Remove the hard coded 17.0.1 - Fix `stat` usage for `darwin`

crodjer force-pushed the binary-installer branch from b17811e to 9442813 Compare December 10, 2023 15:13

vercel bot deployed to Preview December 10, 2023 15:14 View deployment

crodjer requested a review from ellie December 23, 2023 09:05

vercel bot deployed to Preview December 24, 2023 07:27 View deployment

Add prompt to binary installer function

7982375

For scripting usage (say ansible playbooks), setting ATUIN_NO_PROMPT will ensure no prompt is shown.

crodjer force-pushed the binary-installer branch from 8e333ef to 7982375 Compare December 24, 2023 07:29

vercel bot deployed to Preview December 24, 2023 07:29 View deployment

ellie closed this Feb 26, 2024

crodjer deleted the binary-installer branch March 1, 2024 18:15

crodjer restored the binary-installer branch March 1, 2024 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for installing binaries #1413

Add support for installing binaries #1413

crodjer commented Nov 27, 2023 •

edited

Loading

vercel bot commented Nov 27, 2023 •

edited

Loading

ellie left a comment

ellie Nov 29, 2023

crodjer Nov 29, 2023

ellie Nov 29, 2023

crodjer Nov 29, 2023

crodjer commented Nov 29, 2023

crodjer commented Dec 6, 2023

ellie left a comment

ellie Dec 9, 2023

crodjer Dec 10, 2023

ellie Dec 9, 2023

crodjer Dec 10, 2023

crodjer Dec 23, 2023

ellie commented Dec 23, 2023

crodjer commented Dec 24, 2023 •

edited

Loading

ellie commented Feb 26, 2024 •

edited

Loading

crodjer commented Mar 1, 2024

Add support for installing binaries #1413

Add support for installing binaries #1413

Conversation

crodjer commented Nov 27, 2023 • edited Loading

vercel bot commented Nov 27, 2023 • edited Loading

ellie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodjer commented Nov 29, 2023

crodjer commented Dec 6, 2023

ellie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellie commented Dec 23, 2023

crodjer commented Dec 24, 2023 • edited Loading

ellie commented Feb 26, 2024 • edited Loading

crodjer commented Mar 1, 2024

crodjer commented Nov 27, 2023 •

edited

Loading

vercel bot commented Nov 27, 2023 •

edited

Loading

crodjer commented Dec 24, 2023 •

edited

Loading

ellie commented Feb 26, 2024 •

edited

Loading