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

Integrate rubocop-daemon #10706

Merged
merged 2 commits into from Jun 10, 2022
Merged

Conversation

koic
Copy link
Member

@koic koic commented Jun 9, 2022

This PR has two parts committed. That is, the import part and the integration part of rubocop-daemon. Please keep these commits.

Part 1: Import rubocop-daemon

Preparing to resolve #9503.

This PR merges https://github.com/fohte/rubocop-daemon into RuboCop core.

The rubocop-daemon's original license (MIT) is written at the beginning of the imported files.
@fohte @gurgeous

Part 2: Integrate rubocop-daemon

The integrated daemon feature is described in the docs/modules/ROOT/pages/usage/server.adoc included in this PR.

Server options

% bundle exec rubocop --help
(snip)

Server Options:
        --[no-]server                If the server process has not started yet, start the
                                     server process and execute inspection with server.
                                     Default is false.
                                     You can specify the daemon host and port with
                                     the $RUBOCOP_SERVER_HOST and the $RUBOCOP_SERVER_PORT
                                     environment variables.
        --restart-server             Restart server process.
        --start-server               Start server process.
        --stop-server                Stop server process.
        --server-status              Show server status.

The server mode --server is disabled by default because it is not yet stable as an integrated feature.

Design note

RuboCop::Daemon::CLI's daemon options are not integrated into RuboCop::CLI and RuboCop::Options.
Because RuboCop::CLI and RuboCop::Options classes have many dependencies and require the rubocop client (exe/rubocop) to make unnecessary require '...'.

It's important to be lightweight because the daemon mode runs fast, so this design decided to have command line daemon options own (lightweight) option parse logic.

And this design only exe/rubocop and lib/rubocop/options.rb files have changed, So almost all daemon code keep independent.

NOTE: This feature cannot be used on JRuby and Windows that do not support fork.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@koic koic force-pushed the integrate_rubocop_daemon branch 4 times, most recently from 55e0acc to 44185e4 Compare Jun 9, 2022
@gurgeous
Copy link

@gurgeous gurgeous commented Jun 9, 2022

Hey, this is great @koic! Thank you for taking charge. I'm currently deep in Typescript land, but I'll let my team know and see if I can dig up a volunteer to try out the PR.

One suggestion... I have a bit of experience responding to github issues with the vscode ruby plugin. I recommend adding some logging to help people debug problems that might arise. Stale daemons/pid, versioning issues, wrong .rubocop.yml, etc. Those will be easier to identify with logging.

@koic
Copy link
Member Author

@koic koic commented Jun 9, 2022

@gurgeous Thank you for your advice. This PR is the starting point for minimal integration, and will continue to be adjusted and improved with subsequent patches. And, your comment at #9503 (reply in thread) is very helpful :-)

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 9, 2022

I think the daemon terminology is mostly dead these days, so my preference would be to name this "server" instead. I think this will make it much clearer what exactly this is.

I'm also wondering whether it would be better to move the server commands to a dedicated "binary" - e.g. rubocop-server start/stop/restart.

I like the general direction, but I think we (@rubocop/rubocop-core) should spend a bit of time thinking on/polishing the design instead of just integrating the existing project as is.

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 9, 2022

The rubocop-daemon's original license (MIT) is written at the beginning of the imported files.

It doesn't change, btw - RuboCop itself is licensed under the MIT license.

The following is an example:

```console
% RUBOCOP_SERVER_PORT=98989 rubocop --start-daemon
Copy link
Collaborator

@bbatsov bbatsov Jun 9, 2022

Choose a reason for hiding this comment

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

Generally it's not a bad idea to be able to specify the host and port directly via command-line flags as well. That's part of the reason why I like a separate command for managing the server - otherwise we end with a ton of extra options in rubocop itself.

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 9, 2022

We should also mark everything related to the server functionality as experimental and the APIs as private, so it's easy for us to adjust them in subsequent versions. I guess the CLI should stay stable, but we'll need some freedom to polish the internals.

@koic koic force-pushed the integrate_rubocop_daemon branch from 44185e4 to a0ad50a Compare Jun 10, 2022
@koic
Copy link
Member Author

@koic koic commented Jun 10, 2022

@bbatsov Thank you for the great comments! There are several topics about the discussion, so let's categorize them.

License

It doesn't change, btw - RuboCop itself is licensed under the MIT license.

Yes, the license is the same MIT. But with the addition of the original licensed author name.
Please point out any other suitable files to write the original licensed author name. :-)

API stability

I guess the CLI should stay stable, but we'll need some freedom to polish the internals.

I completely agree and I also think it will take some time to polish. These new APIs are unstable, so I marked @api private.

Terminology

I think the daemon terminology is mostly dead these days, so my preference would be to name this "server" instead.

I'm not sure whether daemon is an old terminology 😅 In any case, I also think it's good to have consistent terminology.
So, consistent terminology by replacing daemon with server. I've replaced daemon with server.

NOTE: The RuboCop::Daemon module has been renamed RuboCop::Server. Note that RuboCop::Server::Server is redundant, so RuboCop::Daemon::Server has been renamed to RuboCop::Server::Core.

Command line API design

I'm also wondering whether it would be better to move the server commands to a dedicated "binary" - e.g. rubocop-server start/stop/restart.

Yeah, this is the most difficult...
I could separate rubocop-server from rubocop. But I'm not sure if it's the best yet.

For example, some existing rubocop-daemon user seems to want to start the server with the rubocop command (if the server is not started). That's why rubocop-daemon-wrapper is provided. This asks the user for the following steps and more (that seems frustrating to rubocop-daemon user):
https://github.com/fohte/rubocop-daemon#more-speed

Therefore, the integrated rubocop --daemon new command is meant to resolve this frustrating. That does not require any new settings.

Here's a question.
If server commands split into rubocop-server, will user execute "one command" with rubocop-server start && rubocop? If the bin command (rubocop) are separated (into rubocop and rubocop-server) , I'm wondering if there is a problem or not for integrated editor.

RuboCop server listen on 127.0.0.1:55772.
```

NOTE: The `rubocop` command is executed using server server when server process is started.
Copy link
Collaborator

@bbatsov bbatsov Jun 10, 2022

Choose a reason for hiding this comment

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

server server when server :-)

def start_server(host, port)
@server = TCPServer.open(host, port)

puts "RuboCop server listen on #{@server.addr[3]}:#{@server.addr[1]}."
Copy link
Collaborator

@bbatsov bbatsov Jun 10, 2022

Choose a reason for hiding this comment

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

A better message would be "RuboCop server started/running/listening on ...".

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 10, 2022

If server commands split into rubocop-server, will user execute "one command" with rubocop-server start && rubocop? If the bin command (rubocop) are separated (into rubocop and rubocop-server) , I'm wondering if there is a problem or not for integrated editor.

I was thinking you'd be able to do rubocop --server, but for everything else there would be the dedicated command rubocop-server. I don't feel strongly about this, though, so we can leave it as is.

@koic koic force-pushed the integrate_rubocop_daemon branch from a0ad50a to 0e9ff55 Compare Jun 10, 2022
Preparing to resolve rubocop#9503.

This PR imports https://github.com/fohte/rubocop-daemon into RuboCop core.

The rubocop-daemon's original license (MIT) is written at the beginning
of the imported files.
@koic koic force-pushed the integrate_rubocop_daemon branch 3 times, most recently from 86766a5 to cb90369 Compare Jun 10, 2022
@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 10, 2022

I noticed this in one commit message:

    --daemon-server              Show server status.

I guess you meant to write --server-status here or something like this.

@koic koic force-pushed the integrate_rubocop_daemon branch 2 times, most recently from ead7ca3 to d4a42a2 Compare Jun 10, 2022
@koic
Copy link
Member Author

@koic koic commented Jun 10, 2022

Oops, you're right. That is a typo and is correctly --server-status Show server status. and I've updated it.
I'm currently investigating a problem where tests aren't running on GitHub Actions (Windows).

@koic koic force-pushed the integrate_rubocop_daemon branch 5 times, most recently from 356613b to cba6866 Compare Jun 10, 2022
The integrated daemon feature is described in the docs/modules/ROOT/pages/usage/server.adoc
included in this PR.

## Daemon options

```console
% bundle exec rubocop --help
(snip)

Server Options:
        --[no-]server                If the daemon process has not started yet, start the
                                     server process and execute inspection with server.
                                     Default is false.
                                     You can specify the daemon host and port with
                                     the $RUBOCOP_SERVER_HOST and the $RUBOCOP_SERVER_PORT
                                     environment variables.
        --restart-server             Restart server process.
        --start-server               Start server process.
        --stop-server                Stop server process.
        --server-status              Show server status.
```

The daemon mode `--daemon` is disabled by default because it is not yet
stable as an integrated feature.

## Design note

`RuboCop::Daemon::CLI`'s daemon options are not integrated into
`RuboCop::CLI` and `RuboCop::Options`.
Because `RuboCop::CLI` and `RuboCop::Options` classes have many dependencies
and require the rubocop client (`exe/rubocop`) to make unnecessary `require '...'`.

It's important to be lightweight because the daemon mode runs fast,
so this design decided to have command line daemon options
own (lightweight) option parse logic.

And this design only exe/rubocop and lib/rubocop/options.rb files have changed,
So almost all daemon code keep independent.

NOTE: This feature cannot be used on JRuby and Windows that do not support fork.
@koic koic force-pushed the integrate_rubocop_daemon branch from cba6866 to c76ff8d Compare Jun 10, 2022
@koic
Copy link
Member Author

@koic koic commented Jun 10, 2022

I'm currently investigating a problem where tests aren't running on GitHub Actions (Windows).

I've solved this issue. I dive to ssh to GitHub Actions and found that there was unnecessary access to the server cache directory. Windows never starts the server, so it patched to avoid this process.

runneradmin@fv-az252-858 MINGW64 /d/a/rubocop/rubocop
# ./exe/rubocop lib/rubocop.rb
C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/3.1.0/fileutils.rb:243:in `mkdir': Invalid argument @ dir_s_mkdir - C:/Users/runneradmin/.cache/rubocop_cache/server/:+a+rubocop+rubocop (Errno::EINVAL)
        from C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/3.1.0/fileutils.rb:243:in `fu_mkdir'
        from C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/3.1.0/fileutils.rb:221:in `block (2 levels) in mkdir_p'
        from C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/3.1.0/fileutils.rb:219:in `reverse_each'
        from C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/3.1.0/fileutils.rb:219:in `block in mkdir_p'
        from C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/3.1.0/fileutils.rb:211:in `each'
        from C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/3.1.0/fileutils.rb:211:in `mkdir_p'
        from C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/3.1.0/pathname.rb:585:in `mkpath'
        from D:/a/rubocop/rubocop/lib/rubocop/server/cache.rb:43:in `block in dir'
        from <internal:kernel>:90:in `tap'
        from D:/a/rubocop/rubocop/lib/rubocop/server/cache.rb:42:in `dir'
        from D:/a/rubocop/rubocop/lib/rubocop/server.rb:28:in `running?'
        from ./exe/rubocop:11:in `<main>'

@koic
Copy link
Member Author

@koic koic commented Jun 10, 2022

FYI, a benchmark comparison of require 'rubocop' vs require 'rubocop/server'.

require 'rubocop'

% ruby -rbenchmark -e '$LOAD_PATH.unshift("/path/to/rubocop/lib"); Benchmark.bm { |x| x.report { require "rubocop" }}'
       user     system      total        real
   0.573636   0.217166   0.790802 (  0.815585)

require 'rubocop/server'

% ruby -rbenchmark -e '$LOAD_PATH.unshift("/path/to/rubocop/lib"); Benchmark.bm { |x| x.report { require "rubocop/server" }}'
       user     system      total        real
   0.000386   0.000587   0.000973 (  0.000969)

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 10, 2022

Epic difference! Let's take this brand new server for spin!

@bbatsov bbatsov merged commit c846697 into rubocop:master Jun 10, 2022
30 checks passed
@koic koic deleted the integrate_rubocop_daemon branch Jun 10, 2022
@gurgeous
Copy link

@gurgeous gurgeous commented Jul 18, 2022

I just spent a few hours trying out the new server. From the command line, it's easy to measure the speedup. I am mirroring the way the vscode rubocop extension launches:

$ cat xxx.rb | time rubocop --stdin xxx.rb --force-exclusion --format json
...
0.66s user 0.22s system 87% cpu 1.008 total

$ rubocop --start-server
$ cat xxx.rb | time rubocop --stdin xxx.rb --force-exclusion --format json
...
0.10s user 0.12s system 43% cpu 0.503 total

Awesome, great work @koic!

Unfortunately I am having a lot of difficulty getting any kind of speedup from vscode. Formatting that same file in vscode seems to take around two seconds, regardless of whether or not I run rubocop --server first.

  1. Do you need to pass --server for each run of rubocop? Based on my experiments I don't think this is necessary (see above). Maybe we should clarify this in the docs. Also, it looks like rubocop doesn't load .rubocop or check RUBOCOP_OPTS when looking for --server. I don't think that's a bug, but it tripped me up a bit when I was trying to pass it in every time.

  2. Is there a way to verify that rubocop is connecting to the server? Some logging/debug would be helpful. I tried instrumenting rubocop itself and watching the cache dir. Keep in mind that the vscode ruby extension doesn't allow adding arguments.

  3. Assuming I am successfully using the server (which is unclear), any idea where the slowdown might be coming from? This is much slower than using the old standalone daemon, though I haven't actually used it for a few months... I could probably trace this further by instrumenting rubocop and/or the extension, but I'm out of time for today. Just another plea for a debug log :)

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jul 26, 2022

@koic I like the idea of adding some logging/debug messages.

@gurgeous My guess is that maybe the server wasn't started in the right directory, so afterwards the client is not connecting to it. I think it looks for a server running in the parent directories until it finds one.

@gurgeous
Copy link

@gurgeous gurgeous commented Jul 26, 2022

Pretty sure it's connecting to the server in the correct directory. I instrumented rubocop a bit... Anyway, this is a hairy problem and I totally understand people are hesitant to spend hours digging into it. A passing knowledge of vscode-rubocop, rubocop and possible vscode-ruby are all necessary. Plus a willingness to instrument and add temporary debug output. Vscode-ruby in particular is a beast to configure and really has to be disabled if you want to figure out what's happening with rubocop. Good luck :)

@vfonic
Copy link
Contributor

@vfonic vfonic commented Jul 29, 2022

Just came here to say: Thank you @koic! And thank you @fohte!

I haven't tried this yet, but I'm really looking forward to switching to more stable and hopefully longer-term supported solution.

I wanted to share my current setup for anyone interested:

I'm using rubocop-daemon from within the VS Code through misogi.ruby-rubocop extension with the following settings:

"[ruby]": {
    "editor.formatOnSave": true,
    "editor.defaultFormatter": "misogi.ruby-rubocop"
  },
  "ruby.rubocop.executePath": "/usr/local/bin/",

I'm pretty sure my "rubocop" binary here is "rubocop-daemon-wrapper":

> l /usr/local/bin/ | grep rubocop
.rwxr-xr-x  3.8k viktor  8 Oct  2021  rubocop
.rwxr-xr-x  3.8k viktor  6 Oct  2021  rubocop-daemon-wrapper

It works great until I open a ruby project that's using different ruby version than the previously opened ruby project.

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.

None yet

4 participants