standardrb / vscode-standard-ruby

The official VS Code extension for the Standard Ruby linter and code formatter
Other
101 stars 7 forks source link

Trying to run standardrb inside our existing Docker Compose setup with difficulty #13

Open bjeanes opened 1 year ago

bjeanes commented 1 year ago

Hello!

Because stock Ruby on macOS is old (2.6.10p210 on latest Ventura) and not everybody on our team who works on our app is a Ruby dev who can be expected to maintain a local matching Ruby version, and because our Gemfile fails to install on the host by VSCode out of the box, we're trying to get standard to run inside our existing docker setup.

In Vim, this was possible with:

au User lsp_setup call lsp#register_server({
      \ 'name': 'standardrb',
      \ 'cmd': ['docker', 'compose', 'run', '--rm', 'app', 'standardrb', '--lsp'],
      \ 'allowlist': ['ruby'],
      \ })

However, with this VSCode extension, things have been a bit rockier.

First, I had to create a shim .vscode/standardrb to set standardRuby.commandPath to (I had initially tried to set this to docker compose run --rm app standardrb, but it expects the path to be a command on the path and just tried to invoke docker, afaict). So my shim essentially is docker compose run --rm app standardrb $*.

However, it appears that the extension is passing absolute path names to the LSP, as evidenced by this error output:

[server] Standard Ruby v1.29.0 LSP server initialized, pid 1
[server] Error RuboCop::Error No such file or directory: /Users/bjeanes/Code/the-app-in-question/app/core/legal/reap_closed_accounts.rb
[server] ["/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:478:in `rescue in get_processed_source'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:475:in `get_processed_source'", ...]

Would it be possible to have these files passed in with root-relative paths (even if gated behind a configuration option)?

searls commented 1 year ago

Hey Bo! Always a pleasure to hear from you.

Two thoughts without trying to replicate anything myself. This is just fact-finding and context. Not sure what the ideal solution is yet or what prior art / best practices for extensions like this is.

Customizing the command path

You can customize the command path with this property here. This may partly fix the issue but it looks like there are references to getCwd() that will get in your way depending on other settings (like here)

getCwd()

All of your suspicions are probably correct. Reviewing now, the only way we grab the current working directory is this getCwd() function here that looks for the topmost workspace folder path—clearly not always the right answer.

I am a docker noob, but if there is an addressable file path for invoking the docker version of standard seems like a dupe of #9, to customize that CWD value through a configuration.

bjeanes commented 1 year ago

Hey Justin!

Yeah the command path thing seems solvable. At first I tried it and it seemed to only be taking the first component of the string, but after having read through the code, that doesn't seem right, so that might have been PEBCAK.

Either way, that's not the root problem as even using a shim would be a fair enough solution there.

The root issue seems to be that the extension passes an absolute path over stdio to that process, but that process is running in the context of a virtualised filesystem. That is, linting/formatting/diagnosing /Users/bjeanes/Code/foobar/app/models/user.rb, and the local directory is mounted into the local container at /app (as it is for us), then the path that standardrb needs to look for is actually /app/app/models/user.rb.

I think the heard of it is that the extension is invoking document.uri.toString(): https://github.com/standardrb/vscode-standard-ruby/blob/841e780f16225b6b161adcadb516f6190e12d571/src/extension.ts#L395

I am not familiar with VSCode's API, but had a squiz through the docs and there didn't seem an obvious alternative to get a workspace-relative path from the document object, though I may have missed it.

So there seems to be two alternatives as I see it:

  1. Everywhere where document.uri.toString() is called transforms strips out the leading getCwd(), though given this is a URI and not a path, this might have some sharp edges.
  2. Pass the getCwd() into the invocation of LSP so that the LSP can adjust for the root (e.g. stripping the CWD from paths it receives as part of commands on stdin and working only with relative paths). This would require bumping standardrb itself and might mean this extension must depend on a newer version, unless --cwd /foo/bar would be silently ignored by earlier versions today.

Neither solution seems delightful to me though. WDYT?

searls commented 1 year ago

Hey Bo, heads up that I'm not likely to have time to fix this for a while.

I agree your options aren't without downsides, but one thing I'm sorta stumped by the error message you opened with. I was sure that the LSP being passed a file URL for a non-existent path actually would actually be OK when in STDIN mode (which is the way the server invokes rubocop without reinstantiating it for each request). The file path and full text document are both passed to the server over STDIO by the LSP client and it doesn't read the source listings from the file system directly (see this and this).

I also recall reading in the RuboCop source that it doesn't care what the path is in STDIN mode and it only requires it b/c some cops work differently based on the path/file name (Gemfile stuff, maybe?)

To test this, I just ran this:

$ rubocop --stdin "/some/fake/file/path/fake" < foo.rb 
Inspecting 1 file
W

Offenses:

/some/fake/file/path/fake:1:1: W: Lint/EmptyFile: Empty file detected.

This exited nonzero, but fixing the file exited cleanly/0. As a result, I don't know what the nature of the error or issue is.

$ rubocop --stdin "/some/fake/file/path/fake" < foo.rb 
Inspecting 1 file
.

1 file inspected, no offenses detected

This was running rubocop@1.53.0, which seems to be newer than 15.2.1 from your report. Without diving into the RuboCop source and maybe throwing a binding.irb to grab the backtrace, I'm sort of stumped off hand

bjeanes commented 1 year ago

Hey Bo, heads up that I'm not likely to have time to fix this for a while.

Ack.

I was sure that the LSP being passed a file URL for a non-existent path actually would actually be OK when in STDIN mode (which is the way the server invokes rubocop without reinstantiating it for each request). The file path and full text document are both passed to the server over STDIO by the LSP client and it doesn't read the source listings from the file system directly (see this and this).

Huh. OK 🤔. I'll run this again and post the full stacktrace. That way we might learn what the actual codepath which is trying to read the file from disk is.

bjeanes commented 1 year ago

Actually, there's no need, because it is directly been raised in that first line before I truncated the stack trace.

For posterity, this is the code (from the version of Rubocop in my stacktrace) :

      processed_source = if @options[:stdin]
                           ProcessedSource.new(@options[:stdin], ruby_version, file)
                         else
                           begin
                             ProcessedSource.from_file(file, ruby_version)
                           rescue Errno::ENOENT
                             raise RuboCop::Error, "No such file or directory: #{file}"
                           end
                         end

So, for whatever reason, when it hits this part @options[:stdin] is NOT truthy.

I'm going to upgrade to the latest Rubocop and see if the issue persists.

bjeanes commented 1 year ago

Yeah the command path thing seems solvable. At first I tried it and it seemed to only be taking the first component of the string, but after having read through the code, that doesn't seem right, so that might have been PEBCAK.

Yeah, OK, setting standardRuby.commandPath to docker compose run --rm --no-deps app standardrb does indeed invoke properly. I remember now that at first I was using a make target (make shell COMMAND=standardrb) and this probably failed because it couldn't append flags.

I'm going to upgrade to the latest Rubocop and see if the issue persists.

Ah I can't because of Standard's requirement on the minor version.

I'll run this again and post the full stacktrace

/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:478:in `rescue in get_processed_source'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:475:in `get_processed_source'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:273:in `do_inspection_loop'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:164:in `block in file_offenses'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:189:in `file_offense_cache'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:163:in `file_offenses'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:154:in `process_file'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:135:in `block in each_inspected_file'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:134:in `each'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:134:in `reduce'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:134:in `each_inspected_file'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:120:in `inspect_files'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:73:in `run'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/cli/command/execute_runner.rb:26:in `block in execute_runner'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/cli/command/execute_runner.rb:52:in `with_redirect'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/cli/command/execute_runner.rb:25:in `execute_runner'
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/cli/command/execute_runner.rb:17:in `run'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/runners/rubocop.rb:15:in `call'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/standardizer.rb:62:in `capture_rubocop_stdout'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/standardizer.rb:31:in `offenses'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/routes.rb:164:in `diagnostic'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/routes.rb:53:in `block in <class:Routes>'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/server.rb:25:in `call'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/server.rb:25:in `block in start'
/usr/local/bundle/gems/language_server-protocol-3.17.0.3/lib/language_server/protocol/transport/io/reader.rb:20:in `read'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/server.rb:21:in `start'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/runners/lsp.rb:7:in `call'
/usr/local/bundle/gems/standard-1.29.0/lib/standard/cli.rb:14:in `run'
/usr/local/bundle/gems/standard-1.29.0/exe/standardrb:7:in `<top (required)>'
/usr/local/bundle/bin/standardrb:25:in `load'
/usr/local/bundle/bin/standardrb:25:in `<main>'

So in putting a few debug statements through some of these lines, I realised something new: I am actually getting lint output in the Problems tab and I only see the No such file or directory error when opening a new file for the first time.

By adding some STDERR.puts debug lines throughout the routes, it appears that this exception is only triggering in the textDocument/diagnostic handler.

Further, it appears that :stdin key is present in the options, but its value is nil (debug lines printing out the route handler and state of @options[:stdin] in a few key places in above stack trace):

initialize

initialized
[server] Standard Ruby v1.29.0 LSP server initialized, pid 1

textDocument/didOpen
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:61 initialize :: @options[:stdin]="Premailer::Rails.config.merge!(preserve_styles: true, remove_ids: true)\n"
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:164 file_offenses :: @options[:stdin]="Premailer::Rails.config.merge!(preserve_styles: true, remove_ids: true)\n"
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:166 file_offenses block :: @options[:stdin]="Premailer::Rails.config.merge!(preserve_styles: true, remove_ids: true)\n"
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:277 do_inspection_loop :: @options.keys=[:force_exclusion, :todo_file, :todo_ignore_files, :stdin, :autocorrect, :safe_autocorrect, :formatters, :format]
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:476 get_processed_source :: @options[:stdin]="Premailer::Rails.config.merge!(preserve_styles: true, remove_ids: true)\n"

textDocument/diagnostic
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:61 initialize :: @options[:stdin]=nil
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:164 file_offenses :: @options[:stdin]=nil
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:166 file_offenses block :: @options[:stdin]=nil
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:277 do_inspection_loop :: @options.keys=[:force_exclusion, :parallel, :todo_file, :todo_ignore_files, :stdin, :autocorrect, :safe_autocorrect, :formatters, :format]
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:476 get_processed_source :: @options[:stdin]=nil
[server] Error RuboCop::Error No such file or directory: /Users/bjeanes/Code/the-app-in-question/config/initializers/premailer_rails.rb
[server] ["/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:484:in `rescue in get_processed_source'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:481:in `get_processed_source'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:278:in `do_inspection_loop'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:167:in `block in file_offenses'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:193:in `file_offense_cache'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:165:in `file_offenses'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:155:in `process_file'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:136:in `block in each_inspected_file'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:135:in `each'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:135:in `reduce'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:135:in `each_inspected_file'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:121:in `inspect_files'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:74:in `run'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/cli/command/execute_runner.rb:26:in `block in execute_runner'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/cli/command/execute_runner.rb:52:in `with_redirect'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/cli/command/execute_runner.rb:25:in `execute_runner'", "/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/cli/command/execute_runner.rb:17:in `run'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/runners/rubocop.rb:15:in `call'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/standardizer.rb:62:in `capture_rubocop_stdout'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/standardizer.rb:31:in `offenses'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/routes.rb:175:in `diagnostic'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/routes.rb:57:in `block in <class:Routes>'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/server.rb:25:in `call'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/server.rb:25:in `block in start'", "/usr/local/bundle/gems/language_server-protocol-3.17.0.3/lib/language_server/protocol/transport/io/reader.rb:20:in `read'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/lsp/server.rb:21:in `start'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/runners/lsp.rb:7:in `call'", "/usr/local/bundle/gems/standard-1.29.0/lib/standard/cli.rb:14:in `run'", "/usr/local/bundle/gems/standard-1.29.0/exe/standardrb:7:in `<top (required)>'", "/usr/local/bundle/bin/standardrb:25:in `load'", "/usr/local/bundle/bin/standardrb:25:in `<main>'"]

textDocument/didOpen
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:61 initialize :: @options[:stdin]="Premailer::Rails.config.merge!(preserve_styles: true, remove_ids: true)\n"
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:164 file_offenses :: @options[:stdin]="Premailer::Rails.config.merge!(preserve_styles: true, remove_ids: true)\n"
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:166 file_offenses block :: @options[:stdin]="Premailer::Rails.config.merge!(preserve_styles: true, remove_ids: true)\n"
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:277 do_inspection_loop :: @options.keys=[:force_exclusion, :todo_file, :todo_ignore_files, :stdin, :autocorrect, :safe_autocorrect, :formatters, :format]
/usr/local/bundle/gems/rubocop-1.52.1/lib/rubocop/runner.rb:476 get_processed_source :: @options[:stdin]="Premailer::Rails.config.merge!(preserve_styles: true, remove_ids: true)\n"
bjeanes commented 1 year ago

Since textDocument/didOpen and textDocument/diagnostic handlers are actually identical, I had a look at the doc in each:

["textDocument/didOpen", {:uri=>"file:///Users/bjeanes/Code/the-app-in-question/config/initializers/premailer_rails.rb", :languageId=>"ruby", :version=>1, :text=>"Premailer::Rails.config.merge!(preserve_styles: true, remove_ids: true)\n"}]
["textDocument/diagnostic", {:uri=>"file:///Users/bjeanes/Code/the-app-in-question/config/initializers/premailer_rails.rb"}]

I looked in the extension.ts and don't see any indication that it creates these method calls itself, so I assume this is all behind VSCode's LanguageClient class. It might be that diagnostics never includes the text and Rubocop has been reading from disk the whole time here, but hidden from users where the file paths are identical.

I'm not sure where to go from here. I don't really understand what "diagnostics" means here or why it's important. .to_s on the :stdin option would likely suppress this error entirely, but not sure of the implications there.

bjeanes commented 1 year ago

I may have a workaround that works in my case but mightn't for others, but at least reduces any urgency on a resolution.

I am still testing, but I think I can get away with mounting my app into the Docker container using the same path as the host FS:

diff --git i/docker-compose.yml w/docker-compose.yml
index b406ee5e3..e242e93d8 100644
--- i/docker-compose.yml
+++ w/docker-compose.yml
@@ -13,8 +13,9 @@ services:
+    working_dir: "${PWD}"
     volumes:
-      - ./:/app
+      - "${PWD}:${PWD}"
       - bundle-data:/usr/local/bundle
     ports:
       - 127.0.0.1:3000:3000
@@ -44,6 +45,7 @@ services:
       # High start period to account for cold setup, where dependencies are being installed
       start_period: 300s
     environment:
+      - HOME=${PWD}
       - DEVELOPMENT_DATABASE_URL=postgres://postgres@db/db
       - TEST_DATABASE_URL=postgres://postgres@db/test-db
       - SELENIUM_URL=http://selenium:4444/wd/hub
jonleighton commented 1 year ago

Hi there! We came across some issues with the above workaround, and it also was quite a wide-reaching solution for a problem that only affects VSCode and the Standard Ruby extension. After a bunch of trial and error, we arrived on the following wrapper script, which solves the problem:

#!/bin/bash

exec docker compose \
  run \
  --rm \
  --no-deps \
  --no-TTY \
  --volume "$PWD:$PWD" \
  --name "${COMPOSE_PROJECT_NAME:-$(basename $PWD)}-standardrb-$$" \
  app \
  standardrb "$@"

It does so by mounting a volume at $PWD in the container, so that if the host file path is referenced inside the container, it also exists.

searls commented 12 months ago

Do you all use any other extensions that execute commands? This seems like something that would come up for almost every similar extension when run under Docker, right? It'd be helpful to find some prior art

bjeanes commented 12 months ago

I don't believe we do. @jonleighton actually uses Vim and just jumped in to save my arse haha

searls commented 11 months ago

Sorry, I have so little experience trying to integrate dev tools with Docker that I don't think I'll be able to implement a fix for this on my own without clearer direction on what changes to the code would actually solve the problem. Hopefully someone finds this and can point to another extension that works in a way that Docker agrees with

krystof-k commented 11 months ago

@jonleighton Thanks, I made it work using your example.

I just also needed to switch the mode to Run only globally, never via Bundler.

BTW it may be handy to have this in the README.md. I might create a PR.

willtcarey commented 4 months ago

I was running into a similar issue and before I found this thread (I thought the issue was directly related to VS Code LSP's implementation) I found people pointing to the uriConverters property of the VSCode LanguageClientOptions https://github.com/microsoft/vscode-languageserver-node/issues/746.

Apparently this let's you transform the paths that are coming in and out of the LSP? I'm not really sure. Just thought I'd leave this here for the next person to come across it. I found out that standard was actually linting after even with the errors as those above have stated.