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.

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

warn user that installation will fail if she doesn't have necessary write permissions #466

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mizou04
Copy link

@Mizou04 Mizou04 commented Sep 5, 2023

added the guard logic against absent write permissions in bin/ruby-install

Copy link
Owner

@postmodern postmodern left a 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.

bin/ruby-install Outdated Show resolved Hide resolved
bin/ruby-install Outdated Show resolved Hide resolved
Copy link
Owner

@postmodern postmodern left a comment

Choose a reason for hiding this comment

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

Minor changes.

test/cli-tests/no_permission_warn_test.sh Outdated Show resolved Hide resolved
bin/ruby-install Outdated Show resolved Hide resolved
test/cli-tests/no_permission_warn_test.sh Outdated Show resolved Hide resolved
@Mizou04
Copy link
Author

Mizou04 commented Sep 9, 2023

Can you review the last commit?

Copy link
Owner

@postmodern postmodern left a 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
@@ -18,6 +18,10 @@ fi

init || exit $?

if [[ ! -w "$install_dir" ]]; then
Copy link
Owner

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
  ...
fi

Copy link
Owner

@postmodern postmodern Sep 9, 2023

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 $?.

Copy link
Author

@Mizou04 Mizou04 Sep 10, 2023

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

Copy link
Owner

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.

Copy link
Author

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/

Copy link
Owner

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.

@Mizou04 Mizou04 force-pushed the warn_if_nonroot branch 2 times, most recently from bf564dd to a17854f Compare September 10, 2023 20:35
Copy link
Owner

@postmodern postmodern left a 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
Copy link
Owner

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. If mkdir-p fails, 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 to mkdir -p.
  • If the parent directory does exist, then test if it is writable.

Copy link
Owner

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}"
Copy link
Owner

@postmodern postmodern Sep 17, 2023

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

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"
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants