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

Rework wrapper scripts a bit #190

Merged
merged 5 commits into from
Apr 11, 2020
Merged

Rework wrapper scripts a bit #190

merged 5 commits into from
Apr 11, 2020

Conversation

cdegroot
Copy link
Contributor

@cdegroot cdegroot commented Apr 7, 2020

Note: currently just for review of the scripts, I want to do some more testing and also document the ~/.elixir_ls_setup.sh script. Roughly as discussed on Slack. This will do the right thing on, say, docker containers that only have a minimal Posix shell but also use asdf-vm when available; on top of that, added a hook that will allow kiex/... users to make sure the scripts do what they want.


did_relaunch=$1

asdf_vm="${HOME}/.asdf/asdf.sh"
Copy link
Member

@NobbZ NobbZ Apr 8, 2020

Choose a reason for hiding this comment

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

Am I missing something or will this only work for "user" installations, but not for system wide ones?

Eg. the Arch Linux package puts that script to /opt/asdf-vm/asdf.sh.

Copy link
Member

@axelson axelson Apr 8, 2020

Choose a reason for hiding this comment

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

I don't think that asdf is geared towards system-wide installation. The installation instructions all refer to $HOME/.asdf: https://asdf-vm.com/#/core-manage-asdf-vm

If you want to setup asdf for a non-standard configuration, I would think that editing ~/.config/elixir_ls/setup.sh would be a good solution.

# give them the chance here. ELS_MODE will be set for
# the really complex stuff. Use an XDG compliant path.

els_setup="${HOME}/.config/elixir_ls/setup.sh"
Copy link

Choose a reason for hiding this comment

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

If you are trying to properly support XDG, this should be able to read from $XDG_CONFIG_HOME, or use $HOME/.config if that variable is not set.

Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

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

Thanks for this! ❤️

I'll take care of @jswny's feedback in a separate PR

@axelson axelson merged commit 48c7f25 into elixir-lsp:master Apr 11, 2020
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.

4 participants