Shopify / ruby-lsp-rails

A Ruby LSP addon for Rails
https://shopify.github.io/ruby-lsp-rails/
MIT License
518 stars 21 forks source link

CI is marking failed Windows tests as passing #351

Closed andyw8 closed 2 months ago

andyw8 commented 2 months ago

Originally posted in https://github.com/Shopify/ruby-lsp-rails/pull/350

Note: There are some green checks even though I'm pretty sure they should fail. I think tests doesn't run on windows right now because of how it is invoked in the workflow. It probably chokes on the &&.

https://github.com/Shopify/ruby-lsp-rails/actions/runs/8821159752

andyw8 commented 2 months ago

Fix: https://github.com/Shopify/ruby-lsp-rails/pull/352

It seems the problem relates to the bin/rails binstub, as it works with bundle exec, but I don't yet understand why.

The fix highlights that there are some legit test failures on Windows.

Earlopain commented 2 months ago

I remember this recent rails issue about the rubocop binstub not working on windows: https://github.com/rails/rails/issues/51618

Maybe bundle binstubs rails --force produces a meaningful result? Would probably need to be run on a windows machine, now that I think about it...

andyw8 commented 2 months ago

Also it seems a build that times out may still be marked as green:

https://github.com/Shopify/ruby-lsp-rails/actions/runs/8836233919/job/24262310426

andyw8 commented 2 months ago

This may also be relevant: https://blog.simplificator.com/2016/01/18/how-to-kill-processes-on-windows-using-ruby/

mohits commented 2 months ago

I remember this recent rails issue about the rubocop binstub not working on windows: https://github.com/rails/rails/issues/51618

Maybe bundle binstubs rails --force produces a meaningful result? Would probably need to be run on a windows machine, now that I think about it...

Hi, I opened https://github.com/rails/rails/issues/51618 and yes, bundle binstubs rails --force fixes it and everything runs fine after that.

Is there something you'd like me to try?

Thanks.

andyw8 commented 2 months ago

Thanks, but I think that's isn't the solution here.

If I run bundle binstubs railties --force it does update bin/rails, but then bin/rails c emits a warning advising against that:

Beginning in Rails 4, Rails ships with a `rails` binstub at ./bin/rails that
should be used instead of the Bundler-generated `rails` binstub.

If you are seeing this message, your binstub at ./bin/rails was generated by
Bundler instead of Rails.

You might need to regenerate your `rails` binstub locally and add it to source
control:

It then fails with:

/opt/rubies/3.3.0/lib/ruby/3.3.0/bundled_gems.rb:74:in `require': cannot load such file -- /Users/andyw8/src/github.com/Shopify/ruby-lsp-rails/config/boot (LoadError)

(this aspect is likely because this is a dummy app, so config/boot.rb is in test/dummy instead of the repo root).

andyw8 commented 2 months ago

@mohits I am curious about this though:

If you create a new Rails app on Windows when using Powershell, what does the default bin/rails look like? And does running bin/rails c succeed?

mohits commented 2 months ago

Hi @andyw8 - thanks! I did a fresh new app with rails main in Powershell.

rails new app_on_main --main --skip-test --javascript=esbuild --css=tailwind --database=sqlite3

Got through to the end, went into the application's directory and did:

ruby bin\rails g controller home welcome

Same issue:

D:/Ruby33-x64/lib/ruby/gems/3.3.0/bundler/gems/rails-fc4407eed00e/railties/lib/rails/configuration.rb:138:in `system': Exec format error - bin/rubocop -A --fail-level=E app/controllers/home_controller.rb app/helpers/home_helper.rb (Errno::ENOEXEC)
        from D:/Ruby33-x64/lib/ruby/gems/3.3.0/bundler/gems/rails-fc4407eed00e/railties/lib/rails/configuration.rb:138:in `block in apply_rubocop_autocorrect_after_generate!'
        from D:/Ruby33-x64/lib/ruby/gems/3.3.0/bundler/gems/rails-fc4407eed00e/railties/lib/rails/generators.rb:317:in `block in run_after_generate_callback'
        from D:/Ruby33-x64/lib/ruby/gems/3.3.0/bundler/gems/rails-fc4407eed00e/railties/lib/rails/generators.rb:316:in `each'
...

bin\rails is not exciting:

#!/usr/bin/env ruby.exe
APP_PATH = File.expand_path("../config/application", __dir__)
require_relative "../config/boot"
require "rails/commands"

I again did:

bundle binstubs rubocop --force

Then, ran the rails controller line and all is good.

I also tried ruby bin\rails c and it loaded although it gave me deprecation warnings and an error about loading my .irbrc but it came to the console UI fine. This was the same before and after the binstubs command above.

Hope this helps.

Earlopain commented 2 months ago

I created a new app on windows (with 7.1, not main) and bin/rails by itself is also not working for me. The same is also true for all the other binstubs that are available by default, except for bin/bundle. I believe the reason that bin/bundle works is because of the presence of bin/bundle.cmd (which all the others are missing). For reference, it looks exactly like bin/bundle but with the following extra content at the start:

@ruby -x "%~f0" %*
@exit /b %ERRORLEVEL%

Here's the logic for that: https://github.com/ruby/ruby/blob/29aaf4abe61e5ce24577eb3e8ccaa0a21934bb30/lib/bundler/installer.rb#L141-L145

And finally, here's what Rails itself has to say about running rails under windows:

If you are using Windows, you have to pass the scripts under the bin folder directly to the Ruby interpreter e.g. ruby bin\rails server.

https://guides.rubyonrails.org/getting_started.html#starting-up-the-web-server

If I create bin/rails.cmd with the prefix from above I am able to invoke bin/rails like usual.

andyw8 commented 2 months ago

@mohits thanks, I suspect that may need a fix in Rails, I've let @koic know here.

andyw8 commented 2 months ago

And finally, here's what Rails itself has to say about running rails under windows:

Thanks for finding that. The guides should probably be updated to reflect that this applies to any binstub provided by Rails, not just when starting the server.

Earlopain commented 2 months ago

I'd say it does that already?. bin/rails server is used as an example since that's what the guide needs at that point but the part before makes it clear you need to do this for all comands under bin.

andyw8 commented 2 months ago

Right, but it may be worth a mention in other places, e.g. https://guides.rubyonrails.org/command_line.html#command-line-basics

mohits commented 2 months ago

Hi @andyw8 - just want to be sure that we're on the same page.

For the issue mentioned here, I do use ruby bin\rails for whatever I do, and the failure occurs when rubocop is invoked. However, this is permanently fixed for the project after you do this once:

bundle binstubs rubocop --force

No other change is needed anywhere else. As long as the conclusion you reached and the activity of https://github.com/Shopify/ruby-lsp-rails/pull/353 mean the same, it's fine.

A more detailed walkthrough is on my page about this issue.

andyw8 commented 2 months ago

@mohits I've replied on the issue.

andyw8 commented 1 month ago

@mohits I didn't hear back from @koic, so I opened a PR: https://github.com/rails/rails/pull/51779