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 #683 Restore VsCode 1.91 behavior of allowing scripts as executable #708

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

JVApen
Copy link
Contributor

@JVApen JVApen commented Oct 25, 2024

In VsCode 1.72 Node was updated to version 20, before this upgrade, it was possible to execute cmd/bat scripts as executable.

After this update this was suddenly broken.
Our use-case for using such a script has to do with the consistency of our clang-tooling.
We build clang-format, clang-tidy, clangd ... all together and put this in a package manager.
In the script, we first download these executables (when needed) and than start the downloaded clangd.
As such, when fixing an issue in clang-tidy, the same issue will get fixed in the clangd exe.
Without this script, it is impossible to automatically trigger this download and it introduces the risk that we do not update all the tooling to the latest version.

…s upgrade, it was possible to execute cmd/bat scripts as executable.

     After this update this was suddenly broken.
     Our use-case for using such a script has to do with the consistency of our clang-tooling.
     We build clang-format, clang-tidy, clangd ... all together and put this in a package manager.
     In the script, we first download these executables (when needed) and than start the downloaded clangd.
     As such, when fixing an issue in clang-tidy, the same issue will get fixed in the clangd exe.
     Without this script, it is impossible to automatically trigger this download and it introduces the risk that we do not update all the tooling to the latest version.
@JVApen JVApen changed the title #683 Restore VsCode 1.71 behavior of allowing scripts as executable fix #683 Restore VsCode 1.71 behavior of allowing scripts as executable Oct 25, 2024
@JVApen
Copy link
Contributor Author

JVApen commented Oct 25, 2024

fixes #683

@HighCommander4
Copy link
Contributor

Looks reasonable to me, using shell: true was recommended by a vscode-languageclient maintainer in microsoft/vscode-languageserver-node#1556 (comment).

I assume the PR title should read "vscode 1.91 behavior" (rather than 1.71)?

Do you know if using shell: true also means that on Unix-like systems, ~ in "clangd.path" or "clangd.arguments" will now be expanded to the user's home directory?

@JVApen JVApen changed the title fix #683 Restore VsCode 1.71 behavior of allowing scripts as executable fix #683 Restore VsCode 1.91 behavior of allowing scripts as executable Oct 28, 2024
@JVApen
Copy link
Contributor Author

JVApen commented Oct 28, 2024

@HighCommander4 I don't have a Linux system at hand to test that. Though my understanding of the option is that it enables scripts. Instead of just regular exes. Inside the script, you have access to the env variables.

As such, by using a clangd.sh with ~/clangd $@ should make it possible to use enable it.

@JVApen
Copy link
Contributor Author

JVApen commented Oct 31, 2024

@HighCommander4 What should still happen before this can land?

@HighCommander4
Copy link
Contributor

Do you know if using shell: true also means that on Unix-like systems, ~ in "clangd.path" or "clangd.arguments" will now be expanded to the user's home directory?

I did check on Linux and expansion of ~ does not happen in either "clangd.path" or "clangd.arguments".

It wasn't obvious to me whether that would be the case, because I went digging in the vscode-languageclient source code to see what it does with the shell: true parameter, and found that it forwards it to the nodejs API child_process.spawn, for which the documentation of the parameter says:

runs command inside of a shell

That's a bit vague -- it could well include performing shell expansion of the command / its arguments... but I guess not.

Anyways, I think this patch is fine regardless. I mostly wanted to document these findings for the benefit of anyone in the future who might want to get shell expansion to work or wonders why it doesn't already.

@HighCommander4 HighCommander4 merged commit 0c2d40d into clangd:master Nov 1, 2024
1 check passed
@JVApen JVApen deleted the bugs/683_ScriptsAsExe branch November 1, 2024 07:34
HighCommander4 added a commit to HighCommander4/vscode-clangd that referenced this pull request Nov 13, 2024
HighCommander4 added a commit that referenced this pull request Nov 13, 2024
JVApen pushed a commit to JVApen/vscode-clangd that referenced this pull request Nov 18, 2024
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.

2 participants