Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactoring install.sh #3194

Merged
merged 8 commits into from
Sep 9, 2016
Merged

Refactoring install.sh #3194

merged 8 commits into from
Sep 9, 2016

Conversation

kulla
Copy link
Contributor

@kulla kulla commented Sep 6, 2016

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.

@kulla kulla force-pushed the install-sh-refactoring branch 2 times, most recently from 3dc0fb3 to 224134b Compare September 6, 2016 13:59
@kulla
install.sh: Add for loop to shorten script.
1f81933 
License: MIT
Signed-off-by: Stephan Kulla <git.mail@kulla.me>
@kulla
install.sh: Add error message at the end.
fc20fe8 
License: MIT
Signed-off-by: Stephan Kulla <git.mail@kulla.me>
whyrusleeping
whyrusleeping reviewed Sep 6, 2016
View reviewed changes
cmd/ipfs/dist/install.sh
fi
for binpath in $binpaths; do
if [ -d "$binpath" ]; then
mv "$bin" "$binpath/$bin"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kulla
install.sh: Treat $binpath as target directory.
28a69ee 
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 <git.mail@kulla.me>
@kulla
install.sh: Check whether installation succeeds.
f845841 
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 <git.mail@kulla.me>
@kulla
Copy link
Contributor Author

kulla commented Sep 7, 2016 •
edited
Loading

@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
Copy link
Member

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
Copy link
Member

Kubuxu commented Sep 7, 2016

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
Copy link
Contributor Author

kulla commented Sep 7, 2016

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 kulla mentioned this pull request Sep 7, 2016
Closed
@kulla
Copy link
Contributor Author

kulla commented Sep 7, 2016 •
edited
Loading

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
Copy link
Member

Kubuxu commented Sep 7, 2016

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
Copy link
Member

Kubuxu commented Sep 7, 2016

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

@kulla
Copy link
Contributor Author

kulla commented Sep 7, 2016

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

@whyrusleeping
Copy link
Member

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

@jbenet
Copy link
Member

jbenet commented Sep 7, 2016

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.

kulla added a commit to kulla/go-ipfs that referenced this pull request Sep 8, 2016
@kulla
install.sh: Recommend using sudo on error.
d6b6b20 
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 <git.mail@kulla.me>
@kulla
Copy link
Contributor Author

kulla commented Sep 8, 2016

@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).

@kulla
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>
@kulla
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 <git.mail@kulla.me>
@kulla
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 <git.mail@kulla.me>
@whyrusleeping
Copy link
Member

@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
Copy link
Contributor Author

kulla commented Sep 8, 2016 •
edited
Loading

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". 😄

@kulla
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 <git.mail@kulla.me>
@jbenet
Copy link
Member

jbenet commented Sep 8, 2016

From other thread: use mv -t "$bin" "$binpath"
On Wed, Sep 7, 2016 at 8:05 PM Jeromy Johnson notifications@github.com
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
Copy link
Member

This LGTM

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

@jbenet
Copy link
Member

jbenet commented Sep 9, 2016

@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
jbenet reviewed Sep 9, 2016
View reviewed changes
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay, good explanation 👍

@whyrusleeping
Copy link
Member

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

@whyrusleeping whyrusleeping merged commit e30576d into ipfs:master Sep 9, 2016
@kulla kulla deleted the install-sh-refactoring branch September 10, 2016 10:25
@kulla
Copy link
Contributor Author

kulla commented Sep 10, 2016 •
edited
Loading

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

@whyrusleeping whyrusleeping mentioned this pull request Nov 2, 2016
Closed
@ghost ghost mentioned this pull request Dec 23, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kulla @whyrusleeping @Kubuxu @jbenet