install: Improve linux ipfs install script #2504

mrshu · 2016-03-25T16:19:33Z

Improve linux ipfs install script by trying to install the ipfs binary
to every directory in $PATH, starting with /usr/local/bin and
/usr/bin which were used in the previous version of this script..

Signed-off-by: mr.Shu mr@shu.io

mrshu · 2016-03-25T16:21:11Z

cmd/ipfs/dist/install.sh

+
+for binpath in $paths;
+do
+  if [ -d "$binpath" ]; then


@jbenet At this point we might want to ask the user whether they would really like to install ifps to given directory $binpath. I am sure there are people who would like to skip /usr/bin/local and rather install ipfs to ~/bin for instance.

whyrusleeping · 2016-03-25T16:46:22Z

It might be nice to have the user experience be something like:

ipfs will be installed to '$BEST_PATH_WE_FOUND'
Is that okay? [Y/n]

If they type n, we can ask them for a path that they would like it installed to, OR alternatively print out a list of possible locations (from $PATH) and have them select one by number.

We could also check permissions, and if they need sudo to install it in the selected location, we can use sudo (so it prompts them for a password).

mrshu · 2016-03-25T17:30:51Z

Thanks for the comments @whyrusleeping!

The yes/no dialog is something that I had in mind in my comment. I'll try to figure out how update my PR to facilitate your vision (as it looks pretty solid and way better than what I previously had in mind).

With regards to sudo and permissions, I have two comments:

Checking whether a directory is writable is pretty difficult as just doing [ -w $DIR] might not always be enough. If we are OK with leaving a couple of edge cases uncovered, however, it might be feasible to do it this way.
I am not sure if directly using sudo is the best way of dealing with this as we cannot be always sure that sudo will be installed on user's system. I believe informing the user would be a better option.

whyrusleeping · 2016-03-25T17:59:46Z

re 1, I think have a few edge cases is fine for now, as long as we can still fail gracefully.

As for 2, youre likely right. maybe something like "you must run this command with sudo to install there" would be better

mrshu · 2016-03-25T23:14:31Z

Thanks @whyrusleeping, I implemented all of your suggestions, except for selecting a directory by number/user input as that would be very difficult to implement correctly.

In the current setup the user sees the status of each directory in their $PATH and the script is not ran under root a note is printed. By doing this a huge portion of your desired UX is implemented while the script is still pretty short and clean. I believe it is a tremendous improvement over its current state.

Please do let me know if you have additional feedback.

whyrusleeping · 2016-03-28T00:43:12Z

@mrshu awesome, thanks! I likely won't get to reviewing PR's until tuesday or wednesday though, just a heads up!

chriscool · 2016-03-28T18:58:02Z

cmd/ipfs/dist/install.sh

-if [ -d "$binpath" ]; then
-  mv "$bin" "$binpath/$bin"
-  echo "installed $binpath/$bin"
+if [ -z $bestpath ]; then


$bestpath should be quoted here.

chriscool · 2016-03-28T19:04:56Z

Except the above comment, this LGTM.

* Improve linux ipfs install script by trying to install the ipfs binary to every directory in $PATH, starting with `/usr/local/bin` and `/usr/bin` which were used in the previous version of this script.. License: MIT Signed-off-by: mr.Shu <mr@shu.io>

mrshu · 2016-03-28T20:10:12Z

Thanks for the comment @chriscool

@whyrusleeping no worries, feel free to review it whenever you find some time to spare!

mrshu · 2016-04-10T21:59:10Z

@whyrusleeping There is no rush here, just checking if you have any further comments/feedback.

whyrusleeping · 2016-04-12T18:27:04Z

@mrshu just tried this out, it looks good :) thank you!
I'd like to have @jbenet take a look at this before we merge it, he cares a lot about the install process

jbenet · 2016-07-07T09:26:40Z

cmd/ipfs/dist/install.sh

+
+if [ ! -f "$bin" ]; then
+  echo "The '$bin' binary was not found in the current directory."
+  echo "There is nothing to install."


this should exaplain more, and suggest a fixing action:

echo "Perhaps you have already installed this binary?" echo "If not, you can try extracting the archive again, which should have the '$bin' binary." echo "If you no longer have the archive, you may have to download it again from its origin."

a more advanced version could also test the binary is installed, it just needs the right version to know. could get the version from the readme.

if the versions match, the language should still only say "it seems '$bin $version' is already installed". Operative word "seems". we cannot be sure, as some builds will be different but have exact same version.

We could check the hash and/or use diff.

jbenet · 2016-07-07T09:36:10Z

Sorry for my delay in reviewing this. i've contributed comments to make this a smoother install experience. it is already much smoother (thanks!) but important to make sure errors are not cryptic and are actionable.

kulla · 2016-09-07T14:55:25Z

cmd/ipfs/dist/install.sh

+    esac
+done
+
+if mv "$bin" "$binpath"; then


I recommend changing mv into cp. Cf. PR #3194 comment 3. I also would use the -t parameter. So I would write

cp -t "$binpath" "$bin"

By this the target directory is stated explicitly. There might be the (very very rare :-) ) case that for $binpath=/usr/local/bin the directory /usr/local is empty and thus the ipfs binary is stored in the file /usr/local/bin.

please use mv. it makes sure the binary did indeed go somewhere. People may be confused by the continued presence of the binary.

+1 to removing the bug that might wind up with ipfs moved to a file named /ipfs/local/bin (maybe just be explicit with the filename)

mv "$bin" "$binpath/$bin"

mv "$bin" "$binpath/$bin" also has his (very rare) pitfalls. If /usr/local/bin/ipfs is a directory, then for binpath=/usr/local/bin and bin=ipfs the file ipfs will be stored as /usr/local/bin/ipfs/ipfs. Therefore I guess mv -t "$binpath" "$bin" is the better command... (cf. #3194 (comment))

Ah yep
On Thu, Sep 8, 2016 at 10:02 AM Stephan Kulla notifications@github.com
wrote:

In cmd/ipfs/dist/install.sh
#2504 (comment):

+while true; do

read -p "Is that okay? [Y/n] " yn

Default to yes on empty response

if [ -z "$yn" ]; then

yn="Y"

fi

case $yn in

[Yy]\* ) break;;

[Nn]\* ) exit 0;;

\* ) echo "Please answer yes or no.";;

esac
+done

+if mv "$bin" "$binpath"; then

mv "$bin" "$binpath/$bin" also has his (very rare) pitfalls. If
/usr/local/bin/ipfs is a directory, then for binpath=/usr/local/bin and
bin=ipfs the file ipfs will be stored as /usr/local/bin/ipfs/ipfs.
Therefore I guess mv -t "$binpath" "$bin" is the better command...

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
https://github.com/ipfs/go-ipfs/pull/2504/files/c26ebbcf757b4a5e30f467b6db6849a2c4a81263#r77961445,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIcoYrcGry4L2APQNt7TfytSPz7nTimks5qn8ERgaJpZM4H42aR
.

kulla · 2016-09-07T16:03:56Z

I have a more general question: Is it really necessary to check all directories in $PATH?! Concerning KISS we shall only do this, when this is really necessary. I mean for most users the directory /usr/local/bin exists and will be used as the target directory for this script so that there is no difference with the current version of install.sh. Are there any unix systems without /usr/local/bin?! What are your opinions?

Kubuxu · 2016-09-07T16:38:57Z

Arch for example doesn't have /usr/local/bin by default but most people will add it as most installers use /usr/local/bin either way.

jbenet · 2016-09-07T17:58:19Z

cmd/ipfs/dist/install.sh

+else
+  echo "We were unable to install $bin into $binpath"
+  echo "Please make sure that you can write into $binpath"
+  echo "(possibly by running this script as a privileged user)"


"privileged user" is not obvious to many people. it should instead say:

Perhaps try running: sudo $0

jbenet · 2016-09-07T17:58:42Z

The install script should be very simple. Meaning no more than a few (<20?) clear lines of code. If you need to use such an install.sh it means installing a binary is mysterious to you. A non-totally-obvious script will make matters much worse. I think the script in this PR is too complicated.

Implement proposal of the comment ipfs#2504 (comment) License: MIT Signed-off-by: Stephan Kulla <git.mail@kulla.me>

I removed the comment about using $PATH since it leads to long installation scripts (which violates the KISS principle). Cf. the discussion on ipfs#2504 License: MIT Signed-off-by: Stephan Kulla <git.mail@kulla.me>

Implement proposal of the comment ipfs#2504 (comment) License: MIT Signed-off-by: Stephan Kulla <git.mail@kulla.me>

I removed the comment about using $PATH since it leads to long installation scripts (which violates the KISS principle). Cf. the discussion on ipfs#2504 License: MIT Signed-off-by: Stephan Kulla <git.mail@kulla.me>

whyrusleeping · 2016-11-02T20:40:15Z

closing in favor of #3194

mrshu reviewed Mar 25, 2016
View reviewed changes

mrshu force-pushed the mrshu/linux-install-script branch from 8b6c78f to e4c644d Compare March 25, 2016 16:25

mrshu force-pushed the mrshu/linux-install-script branch from e4c644d to 6216ade Compare March 25, 2016 23:01

chriscool reviewed Mar 28, 2016
View reviewed changes

mrshu force-pushed the mrshu/linux-install-script branch from 6216ade to c26ebbc Compare March 28, 2016 20:09

whyrusleeping assigned jbenet Apr 23, 2016

whyrusleeping added the need/review Needs a review label May 14, 2016

Kubuxu mentioned this pull request Jul 2, 2016

Improve the install.sh script #2936

Closed

jbenet reviewed Jul 7, 2016
View reviewed changes

Kubuxu mentioned this pull request Sep 7, 2016

Refactoring install.sh #3194

Merged

kulla reviewed Sep 7, 2016
View reviewed changes

jbenet reviewed Sep 7, 2016
View reviewed changes

kulla added a commit to kulla/go-ipfs that referenced this pull request Sep 8, 2016

install.sh: Make success message clearer.

0b6ef03

Implement proposal of the comment ipfs#2504 (comment) License: MIT Signed-off-by: Stephan Kulla <git.mail@kulla.me>

kulla added a commit to kulla/go-ipfs that referenced this pull request Sep 8, 2016

install.sh: Make success message clearer.

f00b8d5

Implement proposal of the comment ipfs#2504 (comment) License: MIT Signed-off-by: Stephan Kulla <git.mail@kulla.me>

Kubuxu added the status/deferred Conscious decision to pause or backlog label Sep 29, 2016

whyrusleeping closed this Nov 2, 2016

whyrusleeping removed the status/deferred Conscious decision to pause or backlog label Nov 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

install: Improve linux ipfs install script #2504

install: Improve linux ipfs install script #2504

mrshu commented Mar 25, 2016

mrshu Mar 25, 2016

whyrusleeping commented Mar 25, 2016

mrshu commented Mar 25, 2016

whyrusleeping commented Mar 25, 2016

mrshu commented Mar 25, 2016

whyrusleeping commented Mar 28, 2016

chriscool Mar 28, 2016

chriscool commented Mar 28, 2016

mrshu commented Mar 28, 2016

mrshu commented Apr 10, 2016

whyrusleeping commented Apr 12, 2016

jbenet Jul 7, 2016

jbenet Jul 7, 2016

Kubuxu Sep 7, 2016

jbenet commented Jul 7, 2016

kulla Sep 7, 2016

jbenet Sep 7, 2016

jbenet Sep 7, 2016

kulla Sep 8, 2016 •

edited

Loading

jbenet Sep 8, 2016

Default to yes on empty response

kulla commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

jbenet Sep 7, 2016

jbenet commented Sep 7, 2016 •

edited

Loading

whyrusleeping commented Nov 2, 2016

install: Improve linux ipfs install script #2504

install: Improve linux ipfs install script #2504

Conversation

mrshu commented Mar 25, 2016

Choose a reason for hiding this comment

whyrusleeping commented Mar 25, 2016

mrshu commented Mar 25, 2016

whyrusleeping commented Mar 25, 2016

mrshu commented Mar 25, 2016

whyrusleeping commented Mar 28, 2016

Choose a reason for hiding this comment

chriscool commented Mar 28, 2016

mrshu commented Mar 28, 2016

mrshu commented Apr 10, 2016

whyrusleeping commented Apr 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kulla Sep 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Default to yes on empty response

kulla commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

Choose a reason for hiding this comment

jbenet commented Sep 7, 2016 • edited Loading

whyrusleeping commented Nov 2, 2016

kulla Sep 8, 2016 •

edited

Loading

jbenet commented Sep 7, 2016 •

edited

Loading