Activate Ruby environment using version managers #113

vinistock · 2023-01-16T20:07:39Z

Closes #21

Add Ruby environment activation through version managers. This PR

Exposes a new configuration to select a version manager
Implements how to activate the Ruby environment based on each manager
Starts activating the environment before trying to run the rdbg executable

ko1 · 2023-02-14T03:51:45Z

Sorry I forget to response on it.

(1) do you plan to make a npm package for it?
(2) For "none" I think it is enough to use injectRubyEnvironment without rbenv (and so on) is enough (and remove all other shell dependent code), at least on rbenv. What do you think about it? In other words, I don't want to ask setting rdbg.rubyVersionManager if not needed.

ko1 · 2023-02-14T04:44:22Z

Another question.

On rbenv, RBENV_VERSION is specified at first setting. if .ruby-version is rewritten, it seems it doesn't applied for VSCode without restarting the VSCode process. Is it intentional? I'm not sure how often such a case happened.

vinistock · 2023-02-14T16:06:44Z

I've rebased the PR and modified the changes in #124, since we want all shells to use interactive mode to load rbenv, chruby or any other manager.

Concerning your questions

Yes, we hope to turn the activation into a package so that we don't have to keep duplicating code between the Ruby LSP, RDBG and any other extension that wants to launch Ruby
I'm not sure I understand this one. When the version manager is none (which is the default), we don't invoke injectRubyEnvironment
If you want to re-activate the Ruby environment and inject the variables into the node process when .ruby-version changes, then we have to use a VS Code file watcher to track changes to the file and then trigger re-activation if any modifications happen

ko1 · 2023-02-20T08:09:28Z

Thank you for your quick reply.

Yes, we hope to turn the activation into a package so that we don't have to keep duplicating code between the Ruby LSP, RDBG and any other extension that wants to launch Ruby

If it will be released soon, I want to wait for it.

I'm not sure I understand this one. When the version manager is none (which is the default), we don't invoke injectRubyEnvironment

Yes. My idea is "assuming users use rbenv and injectRubyEnvironment for none will setup rbenv environment variables and no need to call shell for the following commands. But it has an issue for (3).

If you want to re-activate the Ruby environment and inject the variables into the node process when .ruby-version changes, then we have to use a VS Code file watcher to track changes to the file and then trigger re-activation if any modifications happen

I understand the technique. My question was it is intentional or not for .ruby-version changes. As I wrote " I'm not sure how often such a case happened." so if the real case is not many, I agree we can ignore it.

vinistock · 2023-02-21T20:48:19Z

If it will be released soon, I want to wait for it.

Unfortunately, we're not going to be able to work on it immediately. I would recommend merging this for now to make it work on any version manager and then we can refactor once the NPM package is released.

Yes. My idea is "assuming users use rbenv and injectRubyEnvironment for none will setup rbenv environment variables and no need to call shell for the following commands. But it has an issue for (3).

The other managers have a lot of users as well - in particular asdf and chruby. For our team, shadowenv is extremely important given that it is the one we use in every project. I don't think we can escape using shell commands if we want to support a variety of managers and make sure the extension works for all Ruby developers.

I understand the technique. My question was it is intentional or not for .ruby-version changes. As I wrote " I'm not sure how often such a case happened." so if the real case is not many, I agree we can ignore it.

I pushed another commit adding a file watcher. I think it's worth supporting the case of updating .ruby-version, Gemfile and Gemfile.lock. It's probably fairly common.

ono-max · 2023-03-01T14:08:38Z

src/extension.ts

-		switch (true) {
-			case shell && (shell.endsWith("bash") || shell.endsWith("fish")):
-				return shell + " -l -c '" + cmd + "'";
-			case shell && shell.endsWith("zsh"):
-				// As the recommended way, initialization commands for rbenv are written in ".zshrc".
-				// However, it's not loaded on the non-interactive shell.
-				// Thus, we need to run this command as the interactive shell.
-				// FYI: https://zsh.sourceforge.io/Guide/zshguide02.html
-				return shell + " -l -c -i '" + cmd + "'";
-			default:
-				return cmd;
+
+		if (this.support_login(shell)) {
+			return shell + " -lic '" + cmd + "'";
+		} else {
+			return cmd;


Can you cut out this part to another PR? I can merge it soon.

Why not merge this PR instead?

Sorry, I can't merge this PR since @ko1-san is reviewing this PR, and he decides it. And the reason why I ask you to cut this part to another PR is that it is significant to merge this part regardless of this feature.

But, it's just a suggestion, I'll leave it up to you.

I understand your point of view, but my sincere opinion is that we should focus on getting this PR merged.

Using Ruby version managers is common practice on the community and making sure the environment is activated properly is essential to get all of the benefits of RDBG in the editor.

ko1 · 2023-03-07T09:35:39Z

I think no configuration is better but I feel current proposed documentation asks user to write it if they uses a version manager. So I want to rewrite it after merge.
@vinistock could you allow me to modify them after merging?

vinistock · 2023-03-09T14:46:57Z

Can you clarify what you mean by no configuration? Do you mean trying to guess which version manager the developer uses?

Totally okay with you refactoring as needed. Just trying to understand your perspective since the long term vision is for the Ruby LSP and RDBG to both use an NPM package for Ruby environment activation.

vinistock · 2023-03-09T21:30:57Z

We're adding the auto option as the default for the Ruby LSP, which automatically detects the user's version manager. Would that what you're thinking about?

ko1 · 2023-03-22T07:07:44Z

@vinistock
Sorry for the late response but it seems conflicting.
Could you rebase it or can I make a patch?

vinistock · 2023-03-22T13:56:55Z

I rebased. Let me know if you want to include the auto option. We've been using it in the Ruby LSP and it's very convenient to avoid configuration.

firien · 2023-03-25T02:07:41Z

promisify was removed with #41. I have nothing against promisify - it was really the embedded require that caught my eye and provided impetus for e1040ba.
For consistency, I think this should adopt the Promise wrapped callback style, or #41 should revert to promisify version. As noted in #41 - there are also event based versions of child_process.exec in here - so this would introduce yet another flavor of child_process.exec

ono-max · 2023-03-25T06:09:20Z

Thanks @firien san.

I'm ok with using promisify, so let's use promisify in #41, in another PR.

vinistock force-pushed the vs/introduce_ruby_version_managers branch from ad16140 to 12172dc Compare January 18, 2023 19:49

vinistock marked this pull request as ready for review January 18, 2023 20:35

vinistock force-pushed the vs/introduce_ruby_version_managers branch from 12172dc to 8efa22d Compare February 14, 2023 16:02

johnvictorfs mentioned this pull request Feb 16, 2023

Can no longer properly use rbenv ruby version #112

Open

vinistock force-pushed the vs/introduce_ruby_version_managers branch 2 times, most recently from 7c6c7e1 to f136729 Compare February 21, 2023 20:45

ono-max reviewed Mar 1, 2023

View reviewed changes

vinistock force-pushed the vs/introduce_ruby_version_managers branch from f136729 to 037c575 Compare March 9, 2023 21:30

st0012 mentioned this pull request Mar 15, 2023

Create a debug executable for running the server with debugger Shopify/vscode-ruby-lsp#452

Merged

Activate Ruby environment using version managers

581d3a0

vinistock force-pushed the vs/introduce_ruby_version_managers branch from 037c575 to 581d3a0 Compare March 22, 2023 13:56

ko1 merged commit 69de9ea into ruby:master Mar 25, 2023

vinistock deleted the vs/introduce_ruby_version_managers branch March 27, 2023 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Activate Ruby environment using version managers #113

Activate Ruby environment using version managers #113

vinistock commented Jan 16, 2023

ko1 commented Feb 14, 2023 •

edited

Loading

ko1 commented Feb 14, 2023

vinistock commented Feb 14, 2023

ko1 commented Feb 20, 2023

vinistock commented Feb 21, 2023

ono-max Mar 1, 2023 •

edited

Loading

vinistock Mar 1, 2023

ono-max Mar 2, 2023

ono-max Mar 2, 2023

vinistock Mar 2, 2023

ko1 commented Mar 7, 2023 •

edited

Loading

vinistock commented Mar 9, 2023

vinistock commented Mar 9, 2023

ko1 commented Mar 22, 2023

vinistock commented Mar 22, 2023

firien commented Mar 25, 2023 •

edited

Loading

ono-max commented Mar 25, 2023

Activate Ruby environment using version managers #113

Activate Ruby environment using version managers #113

Conversation

vinistock commented Jan 16, 2023

ko1 commented Feb 14, 2023 • edited Loading

ko1 commented Feb 14, 2023

vinistock commented Feb 14, 2023

ko1 commented Feb 20, 2023

vinistock commented Feb 21, 2023

ono-max Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

vinistock Mar 1, 2023

Choose a reason for hiding this comment

ono-max Mar 2, 2023

Choose a reason for hiding this comment

ono-max Mar 2, 2023

Choose a reason for hiding this comment

vinistock Mar 2, 2023

Choose a reason for hiding this comment

ko1 commented Mar 7, 2023 • edited Loading

vinistock commented Mar 9, 2023

vinistock commented Mar 9, 2023

ko1 commented Mar 22, 2023

vinistock commented Mar 22, 2023

firien commented Mar 25, 2023 • edited Loading

ono-max commented Mar 25, 2023

ko1 commented Feb 14, 2023 •

edited

Loading

ono-max Mar 1, 2023 •

edited

Loading

ko1 commented Mar 7, 2023 •

edited

Loading

firien commented Mar 25, 2023 •

edited

Loading