-
Notifications
You must be signed in to change notification settings - Fork 12
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
[#408] add clangd command line options validator #411
[#408] add clangd command line options validator #411
Conversation
if clangd version >= 11, then the clangd binary will be called with configured command line options plus --check and --log=error. If the stderr stream remains empty everything is fine and we can go on. Otherwise we through an IOException in the start() method. This leads to an error dialog with the strings returned from stderr stream. fixes eclipse-cdt#408
I want to add some unit tests. |
to support for example `Ubuntu clangd version`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghentschke I have left some line comments. However I have concerns that the overall design of this is too harsh. By that I mean it can stop LSP4E from trying to start clangd because clangd --check
reports an error.
There are a few cases that concern me:
- This use of temp files means
.clangd
and.clang-format
will be picked up from/tmp
on Linux or even home directory on Windows. This will cause false negatives if that clangd/clangformat file has validation errors. - It seems reasonable that there are some arrangement of command line options that may cause error logging, but not stop functioning of clangd
For 1. here is some terminal commands I tried:
$ echo "nonesense" > .clangd
$ clangd --check=a.c --log=error
E[15:27:07.145] config error at /tmp/.clangd:1:0: Config should be a dictionary
$ rm .clangd
$ echo "nonesense" > .clang-format
$ clangd --check=a.c --log=error
/tmp/.clang-format:1:1: error: not a mapping
nonesense
^~~~~~~~~
/tmp/.clang-format:1:1: error: not a mapping
nonesense
^~~~~~~~~
And with the .clangd left in /tmp this is what I see in the UI:
The mitigating factor is that the error message shows that there is an errant dot file in /tmp (or presumably home directory on Windows)
...dt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/internal/config/ClangdCommandLineValidator.java
Show resolved
Hide resolved
...dt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/internal/config/ClangdCommandLineValidator.java
Outdated
Show resolved
Hide resolved
...dt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/internal/config/ClangdCommandLineValidator.java
Outdated
Show resolved
Hide resolved
...dt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/internal/config/ClangdCommandLineValidator.java
Outdated
Show resolved
Hide resolved
...dt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/internal/config/ClangdCommandLineValidator.java
Outdated
Show resolved
Hide resolved
I agree. I'll add a regex to filter only for the |
Maybe we should add an option flag to enable this pre-launch check. But I would enable it as default. When the check causes trouble on some OS or clangd versions it can be deactivated. |
* @param clangdBinaryPath absolute path to clangd binary | ||
* @return IStatus.OK if validation is supported for the given clangd binary, otherwise IStatus.CANCEL | ||
*/ | ||
public IStatus supportsValidation(final String clangdBinaryPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can remove this method to simplify API and return IStatus.CANCEL
from validateCommandLineOptions
if binary doesn't support validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add an option flag to enable this pre-launch check. But I would enable it as default. When the check causes trouble on some OS or clangd versions it can be deactivated.
Sounds like a good idea - definitely default to on, but this does allow us to field disable an over aggressive command line validation failure.
BTW it would be great if LSP4E could detect the excessive restart problem and prompt user to disable language server. Not sure what that would take.
...dt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/internal/config/ClangdCommandLineValidator.java
Outdated
Show resolved
Hide resolved
...dt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/internal/config/ClangdCommandLineValidator.java
Outdated
Show resolved
Hide resolved
Any objections @jonahgraham or @ruspl-afed ? Otherwise I would merge it |
@jonahgraham and @ruspl-afed Thank you both for your fast and excellent reviews! |
if clangd version >= 11, then the clangd binary will be called with configured command line options plus --check and --log=error. If the stderr stream remains empty everything is fine and we can go on. Otherwise we through an IOException in the start() method. This leads to an error dialog with the strings returned from stderr stream.
fixes #408