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

Existing Installation Detection Enhancements #54

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+24 −6
Open

Existing Installation Detection Enhancements #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96f50b3
Canonicalize SDKMAN_DIR
rgajason Mar 17, 2022
5baa847
Add check for existing file conflicting with installation location
rgajason Mar 17, 2022
7f4b79a
Update install location check to recognize empty top-level directory …
rgajason Mar 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions app/views/includes/sanity.scala.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,23 @@
# Sanity checks

echo "Validating installation location..."
if [ -e "$SDKMAN_DIR" ] && [ ! -d "$SDKMAN_DIR" ]; then
echo "Cannot install to requested location."
echo ""
echo "======================================================================================================"
echo " A file of the same name as the installation directory exists."
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 " File in conflict is:"
echo ""
echo " ${SDKMAN_DIR}"
echo ""
echo " Restart after removing the existing file or choosing a different installation location."
echo "======================================================================================================"
echo ""
exit 1
fi

echo "Looking for a previous installation of SDKMAN..."
if [ -d "$SDKMAN_DIR" ]; then
if [ -d "$SDKMAN_DIR" ] && [ ! -z $(ls -A "$SDKMAN_DIR") ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Why would an empty ~/.sdkman directory be present? If it is empty, why not simply delete the empty directory and re-attempt the installation?

Copy link
Author

Choose a reason for hiding this comment

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

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.

echo "SDKMAN found."
echo ""
echo "======================================================================================================"
Expand Down Expand Up @@ -84,4 +100,4 @@ else
echo ""
exit 1
fi
fi
fi
5 changes: 3 additions & 2 deletions app/views/install_beta.scala.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ SDKMAN_NATIVE_VERSION="@cliNativeVersion"
SDKMAN_PLATFORM=$(uname)

if [ -z "$SDKMAN_DIR" ]; then
SDKMAN_DIR="$HOME/.sdkman"
SDKMAN_DIR=$(readlink -f "$HOME/.sdkman")
SDKMAN_DIR_RAW='$HOME/.sdkman'
else
SDKMAN_DIR_RAW="$SDKMAN_DIR"
SDKMAN_DIR=$(readlink -f $SDKMAN_DIR)
fi

# Local variables
Expand Down Expand Up @@ -140,4 +141,4 @@ fi

}

@includes.install_message(beta)
@includes.install_message(beta)
5 changes: 3 additions & 2 deletions app/views/install_stable.scala.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if readlink -f works on Mac without installing coreutils.

SDKMAN_DIR_RAW='$HOME/.sdkman'
else
SDKMAN_DIR_RAW="$SDKMAN_DIR"
SDKMAN_DIR=$(readlink -f $SDKMAN_DIR)
fi

# Local variables
Expand Down Expand Up @@ -136,4 +137,4 @@ fi

}

@includes.install_message(beta)
@includes.install_message(beta)