Existing Installation Detection Enhancements #54
Conversation
github-actions
bot
requested review from
eddumelendez,
helpermethod and
marc0der
|
Note that |
| echo "Cannot install to requested location." | ||
| echo "" | ||
| echo "======================================================================================================" | ||
| echo " A file of the same name as the installation directory exists." |
There was a problem hiding this comment.
I really don't see why we should check for the presence of a file here. This seems like a very odd situation to be in and perhaps something that you've gotten yourself into by accidentally touching a file called ~/.sdkman. This seems highly unlikely to ever happen out in the wild and feels over-defensive to me.
There was a problem hiding this comment.
I understand the concern, just offering it up while I was in making enhancements. The installer makes a lot of assumptions about the availability of the installation directory and throws all sorts of errors if there is a file in the way. It's obviously your call, but what's the harm in better testing that is simple?
| echo "Looking for a previous installation of SDKMAN..." | ||
| if [ -d "$SDKMAN_DIR" ]; then | ||
| if [ -d "$SDKMAN_DIR" ] && [ ! -z $(ls -A "$SDKMAN_DIR") ]; then |
There was a problem hiding this comment.
Why would an empty ~/.sdkman directory be present? If it is empty, why not simply delete the empty directory and re-attempt the installation?
There was a problem hiding this comment.
I put a detailed explanation of why the ability to install into an existing, empty directory is necessary here: #53
Check out the "Why does this matter?" heading. Short story is, allows unprivileged users to install in privileged locations.
| @@ -9,10 +9,11 @@ SDKMAN_VERSION="@cliVersion" | |||
| SDKMAN_PLATFORM=$(uname) | |||
|
|
|||
| if [ -z "$SDKMAN_DIR" ]; then | |||
| SDKMAN_DIR="$HOME/.sdkman" | |||
| SDKMAN_DIR=$(readlink -f "$HOME/.sdkman") | |||
There was a problem hiding this comment.
If we are going to allow these changes, they should only be done in the beta hook until we can verify that they work on all platforms in the beta channel.
There was a problem hiding this comment.
I'm not sure if readlink -f works on Mac without installing coreutils.
|
Hesitation is understood. If you have ideas about a different method I could take to help provide some code to address this situation I'm happy to make changes. Perhaps we could introduce some sort of "force" flag for the installer that defaults to "no" that would instruct the installer to go ahead and keep going even if the install directory already exists. |
fixes #53