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

Add install helper scripts #1459

Closed
wants to merge 12 commits into from
Closed

Add install helper scripts #1459

wants to merge 12 commits into from

Conversation

cmoog
Copy link
Contributor

@cmoog cmoog commented Mar 27, 2020

Closes #1396

I'm in agreement that a simple one-line install command would greatly improve the user experience of getting started. A transparent bash script seems better than depending on npm for the reasons stated in the issue.

@cmoog cmoog requested review from code-asher and nhooyr as code owners March 27, 2020 19:30
@cmoog
Copy link
Contributor Author

cmoog commented Mar 27, 2020

Alternatively, I agree that brew would be far superior to this and to npm

@nhooyr
Copy link
Contributor

nhooyr commented Mar 30, 2020

I would be in favour of a brew package but the reason for the npm package is so that anyone can use whatever node runtime they want as well and they can easily uninstall/reinstall it without depending on our scripts.

@kylecarbs
Copy link
Member

I'm in the middle on this. There seems to be a good amount of value in easily spinning up code-server without effort. If we did something like curl https://code.cdr.dev | bash for the complete install I could see code-server being spun up even for quick reasons.

@cmoog
Copy link
Contributor Author

cmoog commented Apr 9, 2020

Also-- it'd be nice to have it independent of node and npm, which is easy considering it's already packaged with its compatible node runtime. The ability to install code-server on a bare ubuntu vm seems useful.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 9, 2020

I think most would just use sshcode for that though I can see the value in not having to install anything and just using curl.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 14, 2020

@cmoog we don't need a separate Mac/linux script, we can easily detect in a single script.

@cmoog
Copy link
Contributor Author

cmoog commented Apr 14, 2020

As kyle suggested, we'd probably want to add a proxy for https://code.cdr.dev to https://raw.githubusercontent.com/cdr/code-server/master/install_helper.sh , then update the README. https://install.cdr.dev could also work.

install_helper.sh Show resolved Hide resolved
install_helper.sh Outdated Show resolved Hide resolved
}

linux_install() {
bin_path=$HOME/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be installing it in ~/bin. It should be going into /usr/local/bin. Or just the current directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking it'd be nice to not require sudo perms... although requiring sudo seems better than installing everything into the current dir

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also use /opt which does not require sudo iirc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option is to create a new hidden dir ~/.code-server and put everything there. Then we could either tell the user to add it to their path or just do it ourselves?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets do whatever fzf does.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 14, 2020

Also, not sure if we should be using wget or curl.

bin_path=$HOME/bin
lib_path=$HOME/lib
bin_path=$HOME/.local/bin
lib_path=$HOME/.local/lib
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use ~/.local/share here instead as it's the XDG_DATA_HOME directory.

install_helper.sh Outdated Show resolved Hide resolved
install_helper.sh Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
install_helper.sh Outdated Show resolved Hide resolved
grep '"browser_download_url":\|"tag_name":'
}

linux_install() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mac and linux install functions are pretty much duplicates. Let's have a single one that uses the values of bin_path and lib_path to install.

Copy link
Contributor Author

@cmoog cmoog Apr 15, 2020

Choose a reason for hiding this comment

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

The subtle differences make me think it'll be error-prone if we mix the logic. .tar vs a .zip, the backup for mac, the release URL itself, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can easily check for the extension and what command to use to extract.

I think we shouldn't worry about Mac. A home-brew formula is pretty simple and will be a much better option. Linux's 100 package systems is our only problem. Let's remove the Mac install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed 👍


set -euo pipefail

RED=1
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put this into a function instead of a variable just so you don't have to remember tput setaf everywhere else.

DETECTED_PROFILE="$(detect_profile)"
SOURCE_STR="\nexport PATH=\"\$HOME/.code-server/bin:\$PATH\"\n"

if [ -z "${DETECTED_PROFIL}" ]; then
Copy link

Choose a reason for hiding this comment

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

shouldn't it be DETECTED_PROFILE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Nice catch.


get_releases() {
curl --silent "https://api.github.com/repos/cdr/code-server/releases/latest" |
grep '"browser_download_url":\|"tag_name":'
}

bin_dir=$HOME/.code-server/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the switch back to ~/.code-server over the XDG directories.

@cmoog cmoog closed this Apr 21, 2020
@nhooyr nhooyr deleted the issue-1396 branch May 12, 2020 01:32
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.

Add npm package to make code-server super simple to install
4 participants