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: launch LS even if path contains escapable characters #694

Merged
merged 1 commit into from
Jul 19, 2021

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Jul 19, 2021

Unblocks #691


This fixes a long undiscovered bug which would cause the LS to never launch
in situation where there is an escapable character (such as whitespace)
in the default path leading to the LS binary.

The root cause is string concatenation of full commands (and
child_process.exec API expecteding already escaped string)
when checking LS version, instead of passing arguments individually.

This problem would also never be surfaced to the user due to some other
issues with error reporting (that can be addressed in a separate PR).

Users Affected

This affects all v2 versions from what I can tell (based on the codepath).

Reproduction

First time launch after fresh installation (or upgrade) on Windows with account called Radek Simko (with whitespace) + empty settings:

Screenshot 2021-07-19 at 16 10 34

After restarting VS Code:
Screenshot 2021-07-19 at 16 12 12

This fixes a long undiscovered bug which would cause the LS to never launch
in situation where there is an escapable character (such as whitespace)
in the *default* path leading to the LS binary.

The root cause is string concatenation of full commands (and
`child_process.exec` API expecteding already escaped string)
when checking LS version, instead of passing arguments individually.

This problem would also never be surfaced to the user due to some other
issues with error reporting (that can be addressed in a separate PR).
@radeksimko radeksimko added the bug Something isn't working label Jul 19, 2021
@radeksimko radeksimko requested a review from ansgarm July 19, 2021 15:17
Copy link
Member

@ansgarm ansgarm left a comment

Choose a reason for hiding this comment

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

Ah nice! I think I need to write down a todo to check that we also do this everywhere in the CDKTF 😄

@radeksimko radeksimko merged commit 9afe443 into main Jul 19, 2021
@radeksimko radeksimko deleted the b-fix-exec branch July 19, 2021 16:57
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants