-
-
Couldn't load subscription status.
- Fork 260
warn user that installation will fail if she doesn't have necessary write permissions #466
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes requested. git commit --amend and force push the branch, instead of closing the PR.
Also, this needs unit-tests. Luckily, you can copy test/cli-tests/no_reinstall_test.sh and adapt the test file. The test file just needs to do chmod -w $test_install_dir in the setUp() function before the tests.
1aef8e6 to
a17f424
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes.
a17f424 to
57bb374
Compare
57bb374 to
f8c9c33
Compare
|
Can you review the last commit? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thought of an edge-case where this would break for new users that do not have /opt/rubies or ~/.rubies on their system's yet.
bin/ruby-install
Outdated
|
|
||
| init || exit $? | ||
|
|
||
| if [[ ! -w "$install_dir" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized an edge-case. If $install_dir doesn't exist yet, such as when running ruby-install for the first time on a new system that doesn't have /opt/rubies or ~/.rubies yet, it will consider the $install_dir non-writable. We should also check for it's existence and if it's not-writable.
if [[ -d "$install_dir" ]] && [[ ! -w "$install_dir" ]]; then
...
fiThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might cause problems if the user specifies a really long --install-dir who's parent directories do not exist yet.
ruby-instlal --install-dir `/does/not/exist/yet` ruby
Since the top-most parent directory / is not writable by a regular user, than creating /does would fail.
We could move this logic into share/ruby-install/functions.sh pre_install which already does a mkdir -p "${install_dir%/*}" || return $?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's an issue I think with this, for example if install_dir is of this form : /this/part/exists/this/part/exists/not, we should recursively (iteratively) return backwards to check for the existing part, before testing if it was writable or not; then we proceed with our normal operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we could take advantage of mkdir -p which recursively creates the directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it won't create them if we don't have write permissions over /this/part/exists/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the mkdir -p fails because the last existing sub-directory isn't writable then the command will fail, which could be detected, an error message printed, and an error code returned.
bf564dd to
a17854f
Compare
a17854f to
52ebbea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a few issues with the code. I also still think the while loop can be replaced with mkdir -p.
| status=$? | ||
| fi | ||
|
|
||
| mkdir -p "${install_dir%/*}" || return $status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're going to hate me for constantly suggesting alternative approaches, but I think recursively checking each parent directory is unnecessary:
local parent_dir="${install_dir%/*}"
if [[ ! -d "$parent_dir" ]]; then
mkdir -p "$parent_dir" || fail "cannot create installation directory"
elif [[ ! -w "$parent_dir" ]]; then
fail "installation directory is not writable"
fi- If the parent directory does not exist, try creating it with
mkdir -p. Ifmkdir-pfails, this means that one of the parent directories is not writable, and hence all sub-directories would also not be writable. This basically offloads the recursive checking logic tomkdir -p. - If the parent directory does exist, then test if it is writable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the above while loop is a bit too complex and think checking mkdir -p would have the same effect.
| done | ||
|
|
||
| if [ ! -w "$exisiting_part_dir" ]; then | ||
| fail "Installation directory is not writable by the user : ${exisiting_part_dir}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fail calls exit -1, so status=$? has no effect. Maybe you want to use error and then return 1?
| status=$? | ||
| fi | ||
|
|
||
| mkdir -p "${install_dir%/*}" || return $status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be return $??
|
|
||
| . ./test/helper.sh | ||
|
|
||
| test_install_dir="$test_fixtures_dir/no_write_permissions_test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to make this test more explicit, you could add another variable called non_writable_parent_dir="$test_fixtures_dir/..." and set test_install_dir="$non_writable_parent_dir/ruby".
added the guard logic against absent write permissions in
bin/ruby-install