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.

Sign up for GitHub

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

Jump to bottom

Set server pipes to binmode #366

Merged
merged 1 commit into from
May 2, 2024
Merged

Set server pipes to binmode #366

merged 1 commit into from
May 2, 2024

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented May 2, 2024 •
edited

There are a few Windows related fixes in this PR:

  1. The STDIN and STDOUT pipes must be set to binmode to prevent conversion of carriage returns. Otherwise, the server communication is broken
  2. There were some tests making assertions against URIs/paths that weren't valid on Windows
  3. When splitting the file path on : to separate the line number, we need to account for the fact that Windows' paths begin with C:/, which was resulting in the wrong path and line number
  4. SIGTERM cannot be used on Windows. Windows only responds to KILL, which we can also use on Linux/Mac to terminate a hanging server process

@vinistock vinistock self-assigned this May 2, 2024
@vinistock vinistock added the bugfix This PR fixes an existing bug label May 2, 2024
@vinistock vinistock force-pushed the vs/set_server_pipes_in_binmode branch 5 times, most recently from 7bc10a3 to 36b8832 Compare May 2, 2024 20:12
@vinistock vinistock marked this pull request as ready for review May 2, 2024 20:23
@vinistock vinistock requested a review from a team as a code owner May 2, 2024 20:23
@vinistock vinistock requested review from andyw8 and st0012 May 2, 2024 20:23
andyw8
andyw8 approved these changes May 2, 2024
View reviewed changes
Copy link
Contributor

@andyw8 andyw8 left a comment

Choose a reason for hiding this comment

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

Yay!

lib/ruby_lsp/ruby_lsp_rails/definition.rb Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/runner_client.rb Show resolved Hide resolved
@andyw8 andyw8 mentioned this pull request May 2, 2024
Closed
@vinistock vinistock force-pushed the vs/set_server_pipes_in_binmode branch from 36b8832 to 8fe22d5 Compare May 2, 2024 20:41
@vinistock vinistock enabled auto-merge (squash) May 2, 2024 20:41
@vinistock vinistock merged commit 9147481 into main May 2, 2024
43 checks passed
@vinistock vinistock deleted the vs/set_server_pipes_in_binmode branch May 2, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyw8 andyw8 andyw8 approved these changes

@st0012 st0012 st0012 approved these changes

Assignees

@vinistock vinistock

Labels
bugfix This PR fixes an existing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vinistock @andyw8 @st0012