Add install script #15

R0dn3yS · 2022-05-30T09:04:58Z

No description provided.

install.sh

SpyHoodle

I've left some changes I'd like to be made - but I need mini to see if a file like this is viable. It's going to need some testing, especially on certain distros that have a /usr/bin directory, but don't use it - such as NixOS.

install.sh

SpyHoodle · 2022-05-30T11:44:41Z

install.sh

+echo "Directory to install to (default: $defaultdir)"
+read installdir
+
+if [[ $installdir = "" ]]; then


This syntax can be simplified to

[ -z "$installdir" ] && installdir=$defaultdir

This can be done for other if statements true, where a condition is ran in [ ], then a logical operator is used, such as && where if the first one is true, the second one is ran. Or ||, where if the first one fails or is false, then the next one is ran. This is cleaner and neater syntax which will also make the script smaller. If you don't understand this I can make these changes so you can see what I mean.

SpyHoodle · 2022-05-30T11:45:23Z

install.sh

+  echo -e "\nPridefetch installed successfully"
+  exit
+else
+  echo -e "\nSomething went wrong"


Something went wrong, and pridefetch couldn't install properly.

Would sound better here

SpyHoodle · 2022-05-30T11:47:14Z

install.sh

+#!/bin/bash
+
+if [[ $(whoami) != "root" ]]; then
+  echo "This file needs to be executed as root or with sudo."


This "script" sounds better instead of "file."

This script may not require root if the directory the user chooses is owned by them (e.g. ~/bin/); could this check be moved down along with a permissions check to see if it is needed?

Minion3665 · 2022-05-30T12:27:44Z

I've left some changes I'd like to be made - but I need mini to see if a file like this is viable. It's going to need some testing, especially on certain distros that have a /usr/bin directory, but don't use it - such as NixOS.

Due to NixOS we we know this script will not work on it with default options; that's ok, we have separate Nix packages and people who use Nix are aware that scripts are likely to break. If it works everywhere else that would be enough to merge it along with a note saying such. It could be helpful to include a bit of detection for where we should install (i.e. with nonstandard paths) however that isn't required.

SpyHoodle · 2022-05-30T12:35:34Z

That's what I was thinking - we need at least something, like a message, so someone new to Nix doesn't go and run it and cause damage or leave files in there.

Minion3665

Generally pretty good; and I'd be happy to merge after the few nits highlighted are sorted out

install.sh

Minion3665 · 2022-05-30T12:31:23Z

install.sh

+#!/bin/bash
+
+if [[ $(whoami) != "root" ]]; then
+  echo "This file needs to be executed as root or with sudo."


This script may not require root if the directory the user chooses is owned by them (e.g. ~/bin/); could this check be moved down along with a permissions check to see if it is needed?

Minion3665 · 2022-05-30T12:34:00Z

install.sh

+if [[ -f "$installdir/pridefetch" ]]; then
+  echo -e "\nPridefetch installed successfully"
+  echo -e "\n\nMake sure $installdir is in PATH\nTo add directory to path, add this to your shells rc file:"
+  echo -e "\nexport PATH=\"$installdir:\$PATH\""


This isn't always needed, and almost certainly won't be needed with the default install directory of /usr/bin; could we check if the install dir is already in path before telling the user to add it to their shell's .rc file?

Minion3665 · 2022-05-30T12:34:26Z

install.sh

+
+if [[ -f "$installdir/pridefetch" ]]; then
+  echo -e "\nPridefetch installed successfully"
+  echo -e "\n\nMake sure $installdir is in PATH\nTo add directory to path, add this to your shells rc file:"


There should probably be an apostrophe in shell's

Minion3665 · 2022-05-30T12:35:42Z

install.sh

+zip -r ../pridefetch.zip *
+cd ..
+echo "#!/usr/bin/env python" | cat - pridefetch.zip > pridefetch
+cp pridefetch "$installdir/"


Should we check if pridefetch is already at this location & if so ask for confirmation? Blindly copying files as root seems unwise to me.

Minion3665 · 2022-05-30T12:36:25Z

install.sh

+zip -r ../pridefetch.zip *
+cd ..
+echo "#!/usr/bin/env python" | cat - pridefetch.zip > pridefetch
+cp pridefetch "$installdir/"


This portion of the script leaves pridefetch and pridefetch.zip inside the root of the git repo; should we remove them or use a directory under /tmp?

I think using /tmp is better, instead of making changes in the git repo - sounds easier to manage in future, if changes have to be made, and can be done with shell scripts nicely. I've also seen ~.cache be used as an alternative.

Minion3665

Here are some code suggestions which should fix all pending review comments; if you decide to use them please look them over and test on your machine

install.sh

Minion3665 · 2022-05-30T20:28:47Z

install.sh

+if [[ $installdir = "" ]]; then
+  installdir=$defaultdir
+fi


Suggested change

if [[ $installdir = "" ]]; then

installdir=$defaultdir

fi

[ -z "$installdir" ] && installdir=$defaultdir

Minion3665 · 2022-05-30T20:29:36Z

install.sh

+  echo -e "\nPridefetch installed successfully"
+  exit
+else
+  echo -e "\nSomething went wrong"


Suggested change

echo -e "\nSomething went wrong"

echo -e "\nSomething went wrong, and the installation of pridefetch was not completed."

Minion3665 · 2022-05-30T20:53:58Z

install.sh

+if [[ $(whoami) != "root" ]]; then
+  echo "This file needs to be executed as root or with sudo."
+  exit
+fi
+
+defaultdir="/usr/bin"
+
+echo "Directory to install to (default: $defaultdir)"
+read installdir
+
+if [[ $installdir = "" ]]; then
+  installdir=$defaultdir
+fi
+
+if [[ ! -d $installdir ]]; then
+  echo "This directory does not exist."
+  exit
+fi


Suggested change

if [[ $(whoami) != "root" ]]; then

echo "This file needs to be executed as root or with sudo."

exit

fi

defaultdir="/usr/bin"

echo "Directory to install to (default: $defaultdir)"

read installdir

if [[ $installdir = "" ]]; then

installdir=$defaultdir

fi

if [[ ! -d $installdir ]]; then

echo "This directory does not exist."

exit

fi

defaultdir="/usr/bin"

echo "Directory to install to (default: $defaultdir)"

read installdir

if [[ $installdir = "" ]]; then

installdir=$defaultdir

fi

if [[ ! -d $installdir ]]; then

echo "This directory does not exist."

exit 1

fi

if [[ ! -r $installdir || ! -w $installdir ]]; then

echo "You can't read and write files in this directory; please try running with root access or change the installation directory."

exit 1

fi

if [[ -e "$installdir/pridefetch" ]] ; then

if [[ ! -f "$installdir/pridefetch" ]]; then

echo "$installdir/pridefetch exists and is not a file, so cannot be overwritten. Please remove it and try again"

exit 1

fi

if [[ ! -r "$installdir/pridefetch" || ! -w "$installdir/pridefetch" ]]; then

echo "You can't read and write over the existing pridefetch file; please try running with root access or change the installation directory."

exit 1

fi

echo "The file $installdir/pridefetch already exists; do you want to overwrite it?"

read -p "Continue (y/n)?: " continue

if [ "$continue" != "y" ]; then

exit 1

fi

fi

install.sh

Minion3665 · 2022-05-30T20:59:22Z

install.sh

+zip -r ../pridefetch.zip *
+cd ..
+echo "#!/usr/bin/env python" | cat - pridefetch.zip > pridefetch
+cp pridefetch "$installdir/"


Suggested change

zip -r ../pridefetch.zip *

cd ..

echo "#!/usr/bin/env python" | cat - pridefetch.zip > pridefetch

cp pridefetch "$installdir/"

builddir=$(mktemp -d)

zip -r "$builddir/pridefetch.zip" *

cd ..

echo "#!/usr/bin/env python" | cat - "$builddir/pridefetch.zip" > "$builddir/pridefetch"

cp "$builddir/pridefetch" "$installdir/"

SpyHoodle added the packaging This is to do with packaging label May 30, 2022

SpyHoodle requested a review from Minion3665 May 30, 2022 11:33

SpyHoodle reviewed May 30, 2022

View reviewed changes

install.sh Outdated Show resolved Hide resolved

SpyHoodle requested changes May 30, 2022

View reviewed changes

Minion3665 requested changes May 30, 2022

View reviewed changes

Minion3665 reviewed May 30, 2022

View reviewed changes

SpyHoodle linked an issue Jun 5, 2022 that may be closed by this pull request

Give instructions on packaging to a single file for adding to path #13

Open

R0dn3yS closed this Oct 27, 2022

R0dn3yS force-pushed the master branch from 1a5af3a to 49047e5 Compare October 27, 2022 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add install script #15

Add install script #15

R0dn3yS commented May 30, 2022

SpyHoodle left a comment

SpyHoodle May 30, 2022

SpyHoodle May 30, 2022

SpyHoodle May 30, 2022

Minion3665 May 30, 2022

Minion3665 commented May 30, 2022

SpyHoodle commented May 30, 2022

Minion3665 left a comment

Minion3665 May 30, 2022

Minion3665 May 30, 2022

Minion3665 May 30, 2022

Minion3665 May 30, 2022

Minion3665 May 30, 2022

SpyHoodle May 30, 2022

Minion3665 left a comment

Minion3665 May 30, 2022

Minion3665 May 30, 2022

Minion3665 May 30, 2022

Minion3665 May 30, 2022

	echo -e "\nSomething went wrong"
	echo -e "\nSomething went wrong, and the installation of pridefetch was not completed."

-zip -r ../pridefetch.zip *
-cd ..
-echo "#!/usr/bin/env python" | cat - pridefetch.zip > pridefetch
-cp pridefetch "$installdir/"
+builddir=$(mktemp -d)
+zip -r "$builddir/pridefetch.zip" *
+cd ..
+echo "#!/usr/bin/env python" | cat - "$builddir/pridefetch.zip" > "$builddir/pridefetch"
+cp "$builddir/pridefetch" "$installdir/"

Add install script #15

Add install script #15

Conversation

R0dn3yS commented May 30, 2022

SpyHoodle 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

Minion3665 commented May 30, 2022

SpyHoodle commented May 30, 2022

Minion3665 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

Choose a reason for hiding this comment

Minion3665 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