Fix CI setup for Windows #352

andyw8 · 2024-04-24T19:54:12Z

Closes #351 but we still need to address #348.

andyw8 · 2024-04-25T18:55:29Z

test/ruby_lsp_rails/runner_client_test.rb

-        File.chmod(0o755, "bin/rails")
-
-        # The error message is slightly different on Ubuntu, so we need to allow for that
+        FileUtils.mv("test/dummy/config/application.rb", "test/dummy/config/application.rb.bak")


Picked this as an arbitrary file to remove to verify boot failure.

andyw8 · 2024-04-25T18:57:56Z

test/test_helper.rb

@@ -32,7 +32,11 @@ class TestCase
    include RubyLsp::TestHelper

    def dummy_root
-      File.expand_path("#{__dir__}/dummy")
+      result = File.expand_path("#{__dir__}/dummy")
+      unless result[0] == "/"


The path returned on windows doesn't have the leading /, it starts with the drive letter. So we need to 'normalize' it for the tests.

But why? The correct path on Windows wouldn't have that leading forward slash. Is it because of some URI being incorrect? If that's the case, then the solution is to use the Ruby LSP's URI builder URI::Generic.from_path, which already handles Windows URIs.

Now fixed so that isn't needed.

Spoke too soon, still getting some windows failures.

andyw8 · 2024-04-25T18:58:38Z

lib/ruby_lsp/ruby_lsp_rails/runner_client.rb

@@ -46,10 +46,15 @@ def initialize
          Process.setsid
        rescue Errno::EPERM
          # If we can't set the session ID, continue
+        rescue NotImplementedError
+          # setpgrp() may be unimplemented on some platform
+          # https://github.com/Shopify/ruby-lsp-rails/issues/348


(Let's look into this separately).

andyw8 · 2024-04-25T19:17:20Z

Still seeing failures with the higher timeout... perhaps we are failing to shutdown the server properly on Windows, and it's using a ton of memory, leading to the slowness.

andyw8 · 2024-04-26T17:46:02Z

.github/workflows/ci.yml

@@ -55,4 +55,4 @@ jobs:
        run: bin/rubocop

      - name: Run tests
-        run: bin/rails db:setup && bin/rails db:migrate && bin/rails test
+        run: bundle exec rails db:setup && bundle exec rails db:migrate && bundle exec rails test


bin/rails is failing on Windows but appears to still return a successful exit code, which was causing the build to be wrongly marked as passing.

andyw8 · 2024-04-26T17:58:00Z

lib/ruby_lsp/ruby_lsp_rails/runner_client.rb

        end

        stdin, stdout, stderr, wait_thread = Open3.popen3(
-          "bin/rails",
+          "bundle",


Until we better understand the bin/rails problem on Windows, this seems a safer approach.

This will bypass Spring and be extremely slow in Core. What exactly is the problem with using bin/rails on Windows?

This will bypass Spring and be extremely slow in Core.

I initially thought it would, but it seems does still use Spring:

%% bundle exec rails c Running via Spring preloader in process 889133 running spring after_fork Loading development environment (Rails 7.2.0.alpha)

I think that wasn't always the case, and behaviour has changed in Rails at some point?

What exactly is the problem with using bin/rails on Windows?

It exits right away:

https://github.com/Shopify/ruby-lsp-rails/actions/runs/8822162407/job/24219691837

andyw8 · 2024-04-26T17:59:10Z

@vinistock @st0012 this is ready for another look.

I propose that we get CI running for Windows again, then work on #348 separately.

andyw8 · 2024-04-26T18:00:14Z

Gemfile

@@ -20,3 +20,4 @@ gem "tapioca", "~> 0.13", require: false, platforms: :ruby
 gem "psych", "~> 5.1", require: false
 gem "rails"
 gem "webmock"
+gem "tzinfo-data"


Needed for Windows platforms. Trying to limit the platforms was causing some Bundler issues, so I've left it open.

What issues?

andyw8 · 2024-04-26T18:00:57Z

test/ruby_lsp_rails/runner_client_test.rb

@@ -4,6 +4,9 @@
 require "test_helper"
 require "ruby_lsp/ruby_lsp_rails/runner_client"

+# tests are hanging in CI. https://github.com/Shopify/ruby-lsp-rails/issues/348


I narrowed it down to this file.

Do we still need to skip this test on Windows? If so, let's ship this, but please prioritize unskipping it so that we don't go long with skipped tests.

andyw8 · 2024-04-27T19:51:12Z

Another possible approach is to prefix the binstub calls with ruby:
bf9324b

I've verified this works in CI. It does kinda make sense that the binstub on its own wouldn't work on windows since the #!/usr/bin/env ruby is likely ignored... so does this mean there's no real way to write a cross-platform binstub?

andyw8 · 2024-05-01T14:24:44Z

test/ruby_lsp_rails/code_lens_test.rb

@@ -36,7 +36,7 @@ class Test < ActiveSupport::TestCase
        # The last 3 are for the test declaration.
        assert_equal(6, response.size)
        assert_match("Run", response[3].command.title)
-        assert_equal("bin/rails test /fake.rb:2", response[3].command.arguments[2])
+        assert_match(%r{(ruby )?.*bin/rails test /fake\.rb:2}, response[3].command.arguments[2])


Due to #353

andyw8 · 2024-05-01T14:26:24Z

test/ruby_lsp_rails/code_lens_test.rb

@@ -36,7 +36,7 @@ class Test < ActiveSupport::TestCase
        # The last 3 are for the test declaration.
        assert_equal(6, response.size)
        assert_match("Run", response[3].command.title)
-        assert_equal("bin/rails test /fake.rb:2", response[3].command.arguments[2])
+        assert_match(%r{(ruby )?bin/rails test /fake\.rb:2}, response[3].command.arguments[2])


Due to #353

andyw8 · 2024-05-01T14:29:31Z

@vinistock all passing now.

vinistock · 2024-05-02T14:58:09Z

test/ruby_lsp_rails/runner_client_test.rb

@@ -4,6 +4,9 @@
 require "test_helper"
 require "ruby_lsp/ruby_lsp_rails/runner_client"

+# tests are hanging in CI. https://github.com/Shopify/ruby-lsp-rails/issues/348


Do we still need to skip this test on Windows? If so, let's ship this, but please prioritize unskipping it so that we don't go long with skipped tests.

andyw8 · 2024-05-02T17:33:59Z

I should've rebased before merging, as there are some failures in main now for Windows. Looking into.

andyw8 mentioned this pull request Apr 24, 2024

CI is marking failed Windows tests as passing #351

Closed

andyw8 force-pushed the andyw8/fix-ci-setup-for-windows branch 3 times, most recently from af72131 to 4c69cc8 Compare April 25, 2024 16:36

andyw8 commented Apr 25, 2024

View reviewed changes

andyw8 marked this pull request as ready for review April 25, 2024 18:58

andyw8 requested a review from a team as a code owner April 25, 2024 18:58

andyw8 requested review from st0012 and vinistock April 25, 2024 18:58

andyw8 added the chore Chore task label Apr 25, 2024

andyw8 marked this pull request as draft April 25, 2024 19:17

andyw8 force-pushed the andyw8/fix-ci-setup-for-windows branch 2 times, most recently from 7fb6a46 to 3950b0e Compare April 26, 2024 17:38

andyw8 commented Apr 26, 2024

View reviewed changes

andyw8 marked this pull request as ready for review April 26, 2024 17:59

andyw8 commented Apr 26, 2024

View reviewed changes

andyw8 force-pushed the andyw8/fix-ci-setup-for-windows branch from 15bd20b to 686ff88 Compare May 1, 2024 14:23

andyw8 commented May 1, 2024

View reviewed changes

andyw8 force-pushed the andyw8/fix-ci-setup-for-windows branch from 686ff88 to 613c85a Compare May 1, 2024 14:25

andyw8 commented May 1, 2024

View reviewed changes

andyw8 force-pushed the andyw8/fix-ci-setup-for-windows branch 2 times, most recently from be6666c to 318f642 Compare May 1, 2024 14:27

This was referenced May 1, 2024

Can't use on windows (due to setpgrp) #348

Closed

Investigate getpgid error. #291

Closed

Fixes for Windows

5a82483

andyw8 force-pushed the andyw8/fix-ci-setup-for-windows branch from 318f642 to 5a82483 Compare May 1, 2024 17:06

vinistock approved these changes May 2, 2024

View reviewed changes

andyw8 merged commit 8600f3b into main May 2, 2024

andyw8 deleted the andyw8/fix-ci-setup-for-windows branch May 2, 2024 15:15

andyw8 mentioned this pull request May 2, 2024

Temporarily skip another test on Windows #365

Closed

Fix CI setup for Windows #352

Fix CI setup for Windows #352

Uh oh!

Conversation

andyw8 commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyw8 Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyw8 Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyw8 commented Apr 25, 2024

Uh oh!

andyw8 Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyw8 commented Apr 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyw8 commented Apr 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyw8 commented May 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyw8 commented May 2, 2024

Uh oh!

Uh oh!

andyw8 commented Apr 24, 2024 •

edited

Loading

andyw8 Apr 25, 2024 •

edited

Loading

andyw8 Apr 26, 2024 •

edited

Loading

andyw8 Apr 26, 2024 •

edited

Loading