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

Pass environment as a hash to system or exec calls #841

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

vinistock
Copy link
Member

Motivation

Closes #840

Passing the environment variables in front of the command does not work on every operating system, such as Windows. We should rely on Ruby to do the correct thing for each OS.

I believe that this PR with Shopify/vscode-ruby-lsp#712 will finally make the Ruby LSP work on Windows.

Implementation

The entire exec family accepts extra environment variables as a hash for the first argument. Just started using that instead of concatenating the variables as a string.

Automated Tests

Changed the tests to reflect the new approach.

Passing the environment variables in front of the command does not work
on every operating system, such as Windows
@vinistock vinistock added the bugfix This PR will fix an existing bug label Jul 27, 2023
@vinistock vinistock added this to the 2023-Q3 milestone Jul 27, 2023
@vinistock vinistock requested a review from a team as a code owner July 27, 2023 21:08
@vinistock vinistock self-assigned this Jul 27, 2023
@github-actions
Copy link
Contributor

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.332082 std_dev: 0.011872
          textDocument/diagnostic average: 0.061404 std_dev: 0.012936
          textDocument/definition average: 0.005684 std_dev: 0.003567
      textDocument/selectionRange average: 0.004779 std_dev: 0.000751
 textDocument/semanticTokens/full average: 0.002486 std_dev: 0.000544
        textDocument/documentLink average: 0.002471 std_dev: 0.000449
            textDocument/codeLens average: 0.002467 std_dev: 0.00037
   textDocument/documentHighlight average: 0.002447 std_dev: 0.000502
      textDocument/documentSymbol average: 0.002399 std_dev: 0.00046
        textDocument/foldingRange average: 0.002028 std_dev: 0.00029
textDocument/semanticTokens/range average: 0.001142 std_dev: 0.000333
               codeAction/resolve average: 0.000803 std_dev: 0.000196
           textDocument/inlayHint average: 0.000765 std_dev: 0.000158
               textDocument/hover average: 0.000763 std_dev: 0.00022
    textDocument/onTypeFormatting average: 0.000105 std_dev: 0.000106
          textDocument/formatting average: 9.8e-05 std_dev: 0.000392
          textDocument/codeAction average: 5.7e-05 std_dev: 5.0e-05


================================================================================
Comparison with main branch:

 textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range unchanged
      textDocument/documentSymbol unchanged
        textDocument/foldingRange unchanged
          textDocument/formatting unchanged
          textDocument/diagnostic unchanged
        textDocument/documentLink unchanged
           textDocument/inlayHint unchanged
      textDocument/selectionRange unchanged
   textDocument/documentHighlight unchanged
               textDocument/hover unchanged
          textDocument/codeAction unchanged
    textDocument/onTypeFormatting unchanged
               codeAction/resolve unchanged
          textDocument/completion unchanged
            textDocument/codeLens unchanged
          textDocument/definition unchanged


================================================================================
Missing benchmarks:

RubyLsp::Requests::ShowSyntaxTree

@blm768
Copy link

blm768 commented Jul 28, 2023

This seems to let ruby-lsp get past the point where it was stuck before. Here's the output of running ruby-lsp directly:

Ruby LSP> Skipping custom bundle setup since .ruby-lsp/Gemfile.lock already exists and is up to date
Ruby LSP> Running bundle install for the custom bundle. This may take a while...
Could not find gem 'debug'.
Starting Ruby LSP...

After that, the process keeps running until interrupted. I don't know if the failure to find the debug gem is fatal or not, but the VS Code extension is still in a crash loop, so maybe it is.

@vinistock
Copy link
Member Author

As far as I know, the debug gem works on Windows, so I'm not sure why it would fail to install.

When you try it with the extension, is there any output either in

  1. The Debug console of the VS Code window for vscode-ruby-lsp
  2. The Output tab and Ruby LSP channel for the VS Code window for ruby-lsp

Copy link
Contributor

@egiurleo egiurleo left a comment

Choose a reason for hiding this comment

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

Nice! This seems a bit cleaner anyway.

@vinistock
Copy link
Member Author

Merging this for now since it fixes the linked issue at least.

@vinistock vinistock merged commit c3159eb into main Jul 31, 2023
25 checks passed
@vinistock vinistock deleted the vs/use_environment_for_system_calls branch July 31, 2023 20:32
@shopify-shipit shopify-shipit bot temporarily deployed to production August 1, 2023 19:05 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic setting of BUNDLE_GEMFILE fails on Windows
3 participants