Refactoring install.sh #3194

kulla · 2016-09-06T13:53:35Z

Hello,

I made little changes for the installation script install.sh. The enhancements are:

I use a for loop for the binpaths (following the DRY principle)
An error message was added in the end to inform the user when the script couldn't be installed.

License: MIT Signed-off-by: Stephan Kulla <[email protected]>

whyrusleeping · 2016-09-06T18:21:14Z

cmd/ipfs/dist/install.sh

-fi
+for binpath in $binpaths; do
+  if [ -d "$binpath" ]; then
+    mv "$bin" "$binpath/$bin"


We should check if this succeeds or not and not print 'installed X' if it fails.

In case $binpath/$bin is an already existing directory, the command mv "$bin" "$binpath/$bin" would store the binary in the place $binpath/$bin/$bin. I guess this is not the expected behavior. Therefore I used the -t option of `mv' to specify the target directory explicitly. License: MIT Signed-off-by: Stephan Kulla <[email protected]>

I moved the mv command into the if-condition so that the script only succeeds when mv command ran properly. Thus, there is no need to check whether the mv command will succeed beforehand. License: MIT Signed-off-by: Stephan Kulla <[email protected]>

kulla · 2016-09-07T11:50:04Z

@whyrusleeping Thanks for your comments! I moved the mv command into the if condition so that the script only exits successfully when one of the mv commands succeeds. There is also no need to check whether $binpath exists or is writable beforehand because this is done by the mv command itself (if for example there is no $binpath directory, the mv command will exit with an non zero status code and the body of the if statement is not executed). So also rare cases are handled (for example when $binpath/$bin is an existing directory).

I also specified the target directory in the mv command explicitly. So $bin will not be moved when $binpath/$bin is a directory.

Sitenote: Shall cp be used instead of mv?!

whyrusleeping · 2016-09-07T14:19:35Z

I think youre right and cp should probably be used, Its kinda weird that the binary i downloaded would disappear after running 'install'.

I wonder how best to convey to the user that they should try running the install with sudo. This script will fail on pretty much all systems the user might run it on as the directories we try never have user level write permissions (for good reasons).

Kubuxu · 2016-09-07T14:30:34Z

We have two PRs like that already open: #2936 and #2504.

@whyrusleeping I think it would be best to just join them together, if @kulla wants to do it.
Both of them include valuable notes and comments.

kulla · 2016-09-07T14:42:52Z

Yes, it would be the best to join the PRs together. When I'll have time I will have a look on the other PRs and try to contribute there...

PS: mv is changed with cp.

kulla · 2016-09-07T15:06:35Z

I made comments in #2504. Everything else what I have added in this PR is already implemented in #2504. So it's okay for me to close the discussion and the PR here and to continue working on #2504.

Kubuxu · 2016-09-07T15:31:34Z

It is @whyrusleeping 's call.

Also I think #2504 is abandoned so if you would like to take it over and finish it, it would be great but let's wait for @whyrusleeping.

Kubuxu · 2016-09-07T16:05:34Z

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

kulla · 2016-09-07T16:10:36Z

@Kubuxu I moved my comment to #2504 (comment) 😄

whyrusleeping · 2016-09-07T18:05:16Z

I think this PR is on the right path. The others are very complicated, Simplicity is key here.

jbenet · 2016-09-07T18:40:50Z

Would be nice to detect the write perms fail and ask user to use sudo. this will catch a big fail for a lot of users that don't live in the command line.

Show error message that the user shall try running this script with sudo in case write permissions are missing. Implement proposal of comment ipfs#3194 (comment) License: MIT Signed-off-by: Stephan Kulla <[email protected]>

kulla · 2016-09-08T08:40:53Z

@jbenet Implementation of error message recommending sudo (if write permissions are missing) is done. Following #2504 (comment) I changed cp with mv. I also included #2504 (comment) (using of Moved ./ipfs to /usr/local/bin/ipfs as success message).

Implement proposal of the comment ipfs#2504 (comment) License: MIT Signed-off-by: Stephan Kulla <[email protected]>

Show error message that the user shall try running this script with sudo in case write permissions are missing. Implement proposal of comment ipfs#3194 (comment) License: MIT Signed-off-by: Stephan Kulla <[email protected]>

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 <[email protected]>

whyrusleeping · 2016-09-08T13:37:42Z

@kulla just to double check, the -t flag on mv works the same on both gnu and bsd tools? (we've had issues with this in the past)

kulla · 2016-09-08T14:51:03Z

Yeah. you're right. Concerning https://www.freebsd.org/cgi/man.cgi?query=mv&sektion=1 there is no -t parameter in bsd's mv command... I'll change the move command...

Done: Changed to mv "$bin" "$binpath/$bin". 😄

Return to the old definition of the mv command since there is no `-t` parameter in `mv` of BSD. Cf. https://www.freebsd.org/cgi/man.cgi?query=mv&sektion=1 License: MIT Signed-off-by: Stephan Kulla <[email protected]>

jbenet · 2016-09-08T18:15:49Z

From other thread: use mv -t "$bin" "$binpath"
On Wed, Sep 7, 2016 at 8:05 PM Jeromy Johnson [email protected]
wrote:

I think this PR is on the right path. The others are very complicated,
Simplicity is key here.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3194 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIcoeRB0FXgTPTX2CI1suYwsgKphf2bks5qnvzcgaJpZM4J13MF
.

whyrusleeping · 2016-09-09T18:12:12Z

This LGTM

@jbenet cant use -t, its not cross platform.

jbenet · 2016-09-09T21:04:51Z

@whyrusleeping yep, saw thanks. LGTM

(from my information reference frame, my comment happened before the others, but it was stuck in an email client for hours before reaching the rest of the world.)

jbenet · 2016-09-09T21:05:33Z

cmd/ipfs/dist/install.sh

+  echo "It seems that we do not have the necessary write permissions."
+  echo "Perhaps try running this script as a privileged user:"
+  echo
+  echo "    sudo $0"


yay, good explanation 👍

whyrusleeping · 2016-09-09T22:17:08Z

@kulla thank you so much! This has needed a nice clean update for a while.

kulla · 2016-09-10T10:30:09Z

Thanks to all for your comments! 😄 (and btw thanks for the great project!)

kulla force-pushed the install-sh-refactoring branch 2 times, most recently from 3dc0fb3 to 224134b Compare September 6, 2016 13:59

kulla added 2 commits September 6, 2016 16:04

install.sh: Add for loop to shorten script.

1f81933

License: MIT Signed-off-by: Stephan Kulla <[email protected]>

install.sh: Add error message at the end.

fc20fe8

License: MIT Signed-off-by: Stephan Kulla <[email protected]>

kulla force-pushed the install-sh-refactoring branch from 224134b to fc20fe8 Compare September 6, 2016 14:06

whyrusleeping reviewed Sep 6, 2016
View reviewed changes

kulla added 2 commits September 7, 2016 13:29

kulla mentioned this pull request Sep 7, 2016

install: Improve linux ipfs install script #2504

Closed

kulla force-pushed the install-sh-refactoring branch from edd9f30 to c604774 Compare September 8, 2016 08:35

kulla added 3 commits September 8, 2016 10:42

install.sh: Make success message clearer.

f00b8d5

Implement proposal of the comment ipfs#2504 (comment) License: MIT Signed-off-by: Stephan Kulla <[email protected]>

install.sh: Recommend using sudo on error.

81e40de

Show error message that the user shall try running this script with sudo in case write permissions are missing. Implement proposal of comment ipfs#3194 (comment) License: MIT Signed-off-by: Stephan Kulla <[email protected]>

install.sh: Enhance comments of the script.

7f194c9

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 <[email protected]>

kulla force-pushed the install-sh-refactoring branch from c604774 to 7f194c9 Compare September 8, 2016 08:43

install.sh: Remove -t parameter from mv command.

465e4b9

Return to the old definition of the mv command since there is no `-t` parameter in `mv` of BSD. Cf. https://www.freebsd.org/cgi/man.cgi?query=mv&sektion=1 License: MIT Signed-off-by: Stephan Kulla <[email protected]>

jbenet reviewed Sep 9, 2016
View reviewed changes

whyrusleeping merged commit e30576d into ipfs:master Sep 9, 2016

kulla deleted the install-sh-refactoring branch September 10, 2016 10:25

whyrusleeping mentioned this pull request Nov 2, 2016

Improve the install.sh script #2936

Closed

ghost mentioned this pull request Dec 23, 2016

Code Review v0.4.4...v0.4.5 #3534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring install.sh #3194

Refactoring install.sh #3194

kulla commented Sep 6, 2016

whyrusleeping Sep 6, 2016

kulla commented Sep 7, 2016 •

edited

Loading

whyrusleeping commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

kulla commented Sep 7, 2016

kulla commented Sep 7, 2016 •

edited

Loading

Kubuxu commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

kulla commented Sep 7, 2016

whyrusleeping commented Sep 7, 2016

jbenet commented Sep 7, 2016

kulla commented Sep 8, 2016

whyrusleeping commented Sep 8, 2016

kulla commented Sep 8, 2016 •

edited

Loading

jbenet commented Sep 8, 2016

whyrusleeping commented Sep 9, 2016

jbenet commented Sep 9, 2016

jbenet Sep 9, 2016

whyrusleeping commented Sep 9, 2016

kulla commented Sep 10, 2016 •

edited

Loading

Refactoring install.sh #3194

Refactoring install.sh #3194

Conversation

kulla commented Sep 6, 2016

whyrusleeping Sep 6, 2016

Choose a reason for hiding this comment

kulla commented Sep 7, 2016 • edited Loading

whyrusleeping commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

kulla commented Sep 7, 2016

kulla commented Sep 7, 2016 • edited Loading

Kubuxu commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

kulla commented Sep 7, 2016

whyrusleeping commented Sep 7, 2016

jbenet commented Sep 7, 2016

kulla commented Sep 8, 2016

whyrusleeping commented Sep 8, 2016

kulla commented Sep 8, 2016 • edited Loading

jbenet commented Sep 8, 2016

whyrusleeping commented Sep 9, 2016

jbenet commented Sep 9, 2016

jbenet Sep 9, 2016

Choose a reason for hiding this comment

whyrusleeping commented Sep 9, 2016

kulla commented Sep 10, 2016 • edited Loading

kulla commented Sep 7, 2016 •

edited

Loading

kulla commented Sep 7, 2016 •

edited

Loading

kulla commented Sep 8, 2016 •

edited

Loading

kulla commented Sep 10, 2016 •

edited

Loading