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

fix: add pwsh support for zip decompression, zls wrong bin path #4569

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

jinzhongjia
Copy link
Contributor

@jinzhongjia jinzhongjia commented Sep 30, 2024

The zip decompression of lsp-mode under the windows platform is damaged.

Emacs under windows will have a strange problem when using shell-command to execute bash commands. I have not found the reason to solve it.
Just like this:

(shell-command "bash -c 'echo hello && echo kk'")

Run it on emacs with Windows, and emacs will report error:

/bin/bash: -c: line 1: unexpected EOF while looking for matching `''

But this will work fine:

(shell-command "bash -c 'echo hello")

Then because emacs will give priority to using bash to create the directory and call unzip to perform the decompression operation (if unzip exists)

Feats:

  • add powershell support for zip decompression
  • fix zls path problem

@github-actions github-actions bot added the client One or more of lsp-mode language clients label Sep 30, 2024
@jcs090218
Copy link
Member

I think PR #4472 will perhaps solve part of this issue.

@jinzhongjia
Copy link
Contributor Author

oh, yes
The pr you mentioned does a better job than me
However, I plan to modify it and add support for pwsh. In theory, pwsh is more efficient and avoids powershell.

@jcs090218
Copy link
Member

Can you resolve the conflict (revert changes from lsp-mode.el)? I would like to merge the other part! :D

@jinzhongjia
Copy link
Contributor Author

I'm doing this, wait a minute

@jinzhongjia jinzhongjia changed the title fix: zip decompression error under windows, zls wrong bin path fix: add pwsh support for zip decompression, zls wrong bin path Oct 1, 2024
@jinzhongjia
Copy link
Contributor Author

If the ci test is normal after completion, it can be merged.

@jinzhongjia
Copy link
Contributor Author

Great, looks like it's ready to be merged

lsp-mode.el Show resolved Hide resolved
lsp-ext-pwsh-script)
((and (eq system-type 'windows-nt)
(executable-find "powershell"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is for powershell 5 @jcs090218

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this! I guess line 8472 is no longer needed? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for adding this! I guess line 8472 is no longer needed? 🤔

Sorry, I forgot to modify line 8472. Powershell 5 only supports windows. This line should use pwsh.

Copy link
Contributor Author

@jinzhongjia jinzhongjia Oct 3, 2024

Choose a reason for hiding this comment

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

I resquashed and the problem was fixed
The current logic is that:
if you are on the windows platform, the priority of decompressing the zip is pwsh->powershell->unzip(bash)->pwsh(Do we still need to skip this step through the detection platform?)
other platform: unzip(bash)->pwsh

@jinzhongjia
Copy link
Contributor Author

gnu is down again, causing the current ci test to not proceed normally

@jcs090218
Copy link
Member

ELPA is down, which causes the CI to fail. 😕

@jcs090218 jcs090218 merged commit 66739e5 into emacs-lsp:master Oct 3, 2024
11 of 13 checks passed
@jcs090218
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client One or more of lsp-mode language clients documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants