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

add --dest flag for installer #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+17 −6
Open

add --dest flag for installer #4

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 17 additions & 6 deletions installer.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,19 @@
# NOTE: Heavily inspired by get-stack.hs script for installing stack.
# https://raw.githubusercontent.com/commercialhaskell/stack/stable/etc/scripts/get-stack.sh

HOME_LOCAL_BIN="$HOME/.local/bin"
HOME_LOCAL_SHARE="$HOME/.local/share"
# Default install directory
INSTALL_DIR="$HOME"

while getopts ":d:" opt; do
Copy link
Member

Choose a reason for hiding this comment

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

I believe this part is redundant since you already picked up the argument below, where we also read other arguments?

case $opt in
d) INSTALL_DIR="$OPTARG" ;;
*) echo "Invalid argument: -$OPTARG" >&2; exit 1 ;;
esac
done

# Assign the directories
HOME_LOCAL_BIN="$INSTALL_DIR/.local/bin"
HOME_LOCAL_SHARE="$INSTALL_DIR/.local/share"
Comment on lines +7 to +18
Copy link

@sodic sodic Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good idea with the installation destination!
How come you decided to do this, I'm guessing you wanted a global installation option?

Unfortunately, these locations (i.e., .local/something) are not arbitrary - the way we organize our files isn't independent of the top-level installation destination. In other words, we should probably avoid replicating their structure without considering where we're putting them.

Coincidentally, this is exactly what @infomiho and me have been talking about for the last couple of days! Check this comment for the full explanation (it's a whole thing with a long history): wasp-lang/wasp#953 (comment)

Anyway:

  • If you just wanted the option to install Wasp globally, the XDG standard tells us exactly what where we should put our files - we can simply add the -g/--global flag instead of this one and "hardcode" those paths.
  • If you wanted to give users full control over where Wasp is installed, we'll discuss how to best approach it.

EDIT: Also, what Miho said here: #4 (comment). It might make sense to postpone doing this in the interest of simplifying the (not-yet-existing) uninstallation process.

Copy link
Author

Choose a reason for hiding this comment

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

This was intended to make working with online IDEs (Replit, GitPod) a bit easier, as we sometimes need to change the installation destination to get it working correctly on these environements. See: https://github.com/wasp-lang/replit-template/tree/vince-add-replit-template

Copy link
Member

Choose a reason for hiding this comment

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

Yup, this was per my suggestion, and I didn't think much about uninstalling at that point!

I would suggest the following, to keep it simple: we go with this, but don't tell users about it, so it is mostly just for us and potentially for some power users.

The first version of uninstall, the simple one, will work for everyone that use default paths. It won't work for us and maybe for some very rare power users, but in that case we assume us/them know what they are doing anyway, for now.

Once we do a solution with XDG paths, we can refactor all of this, but then we can do it in one coherent go. I wouldn't let that stop us right now though to get out what we need. But would certainly, in an issue for XDG paths, mention that right now we have an option to install in different than default path, that we need it for replit, and that we should make sure we can still do what we need for replit.

WASP_TEMP_DIR=
VERSION_ARG=

Expand All @@ -15,10 +26,10 @@ RESET="\033[0m"

while [ $# -gt 0 ]; do
case "$1" in
# -d|--dest)
# DEST="$2"
# shift 2
# ;;
-d|--dest)
INSTALL_DIR="$2"
shift 2
;;
-v|--version)
VERSION_ARG="$2"
shift 2
Expand Down