ClangD support for Eclipse Che #7516

hkolvenbach · 2017-11-22T14:59:34Z

What does this PR do?

This PR adds initial support for C/C++ Intellisense by using clangd as a language server. It supports starting the language server and connecting to the clangd in a hardcoded path within the docker image

There is still work to do, but I would like to use this PR to discuss how to best continue.
Open Questions:

Which clangd binary to integrate? Stable release (5.0.1) or latest svn (6.x)
How to provide the clangd binary? Who is maintaining the eclipse/cpp_gcc docker image and should we integrate clangd into it?
How should we handle the compile_commands.json? ClangD expects this to do proper "gotoDefinition" handling, however this file needs to be generated e.g. by cmake and passed as an argument to the language server

ToDo:

Che6 Integration

What issues does this PR fix or reference?

#4991

Release Notes

Docs PR

hkolvenbach · 2017-11-23T08:32:55Z

@AndrienkoAleksandr should i squash or rewrite your commits to fix the ip validation error?

AndrienkoAleksandr · 2017-11-23T09:37:14Z

@hkolvenbach ok. You can do all necessary
.

tolusha · 2017-11-23T13:13:18Z

Hello. Here is my humble opinions.

Which clangd binary to integrate? Stable release (5.0.1) or latest svn (6.x)

It depends. If the latest version provides considerably more functionality then it make sense, otherwise I would prefer the stable version.

How to provide the clangd binary?

We can host them as we host other binaries for language servers.

Who is maintaining the eclipse/cpp_gcc docker image and should we integrate clangd into it?

I would say no.

How should we handle the compile_commands.json? ClangD expects this to do proper "gotoDefinition" handling, however this file needs to be generated e.g. by cmake and passed as an argument to the language server

@evidolob may be could help here.

evidolob · 2017-11-23T13:26:57Z

How should we handle the compile_commands.json? ClangD expects this to do proper "gotoDefinition" handling, however this file needs to be generated e.g. by cmake and passed as an argument to the language server

I guess, we need to generate that file after project creation/clone.

AndrienkoAleksandr · 2017-11-23T15:18:21Z

@hkolvenbach About generation json, we can try workaround. I think we can install cmake with help installation script org.eclipse.che.ls.clangd.sh. And than we can run command for generation json like we do it for csharp-ls: "dotnet restore on start csharp-ls" . For csharp we run command "dotnet restore" to restore dotnet dependencies, without them cshape intellisense doesn't work. For clangD we can run cmake command to generate compilation data-source json.
And as I know cmake support generation such json from some version. We should check installation cmake in the container and version cmake. In case if cmake was not installed than install this third-party program.
What do you think @tolusha ?

tolusha · 2017-11-28T14:50:44Z

@AndrienkoAleksandr I don't mind.

hkolvenbach · 2017-12-05T08:24:05Z

@tolusha @AndrienkoAleksandr @evidolob thanks for your responses, we are now continuing to work on it. just another question: what would be the most appropriate way to add the --compile-commands-dir=<compile_commands.json> parameter to the agent launch script? just write a new launch.sh script with the specific compile_commands.json parameter before https://github.com/eclipse/che/pull/7516/files#diff-b0a97402fc623235c7707c20684cfe61R75 for each project?

FYI @thierryk12

tolusha · 2017-12-06T15:28:43Z

IMHO it is better choice:

just write a new launch.sh script with the specific compile_commands.json parameter before https://github.com/eclipse/che/pull/7516/files#diff-b0a97402fc623235c7707c20684cfe61R75 for each project?

hkolvenbach · 2017-12-12T09:14:33Z

just a quick update: we have a working compile_commands.json integration internally, but noticed that clangd (or the communication from eclipse che to clangd) is crashing regularly after a few minutes. currently investigating if this is a clangd or eclipse che problem.

hkolvenbach · 2017-12-12T09:16:02Z

by the way: am i correct that eclipse che is not restarting a crashed language server? the VSCode plugin for clangd restarts it once it crashes.

tolusha · 2017-12-12T12:25:04Z

@hkolvenbach
You are right, LS won't be restarted in case of crashing.

hkolvenbach · 2018-01-09T16:11:03Z

Quick update (as already told on the community call yesterday):
I will soon update the PR with a Che6 version of the clangd integration (we won't continue on che5)

What is currently working:

go to definition
autodetect compile_commands.json (clangd feature)
refactoring
code completion & templates like "for" loops
warnings and errors

clangd very much depends on the compile_commands.json file, but if it is present, the results are generally very good. regarding #4991, a quick comparison to cquery also didn't show much differences (comparing latest clangd 6 svn).

There are some open issues regarding the languageserver implemention of che though, which I would like to discuss before we propose changes that break in other languages:

code completion appends instead of replacing the text (see the for loop example, for becomes forfor) We think the issue is here https://github.com/eclipse/che/blob/af0376de7f0d0aaadc90647fe4e54e9ad6040938/plugins/plugin-languageserver/che-plugin-languageserver-ide/src/main/java/org/eclipse/che/plugin/languageserver/ide/editor/codeassist/CompletionItemBasedCompletionProposal.java#L230-L239. @gorkem @tsmaeder is there any reason to have this else case? where is offset coming from? if we use this https://github.com/eclipse/che/blob/af0376de7f0d0aaadc90647fe4e54e9ad6040938/plugins/plugin-languageserver/che-plugin-languageserver-ide/src/main/java/org/eclipse/che/plugin/languageserver/ide/editor/codeassist/CompletionItemBasedCompletionProposal.java#L226 instead, it works correctly
the autocompletion uses a fuzzy search https://github.com/eclipse/che/blob/af0376de7f0d0aaadc90647fe4e54e9ad6040938/plugins/plugin-languageserver/che-plugin-languageserver-ide/src/main/java/org/eclipse/che/plugin/languageserver/ide/editor/codeassist/LanguageServerCodeAssistProcessor.java#L136, compared to VScode this generates a lot of noise and printf, which actually starts with pri is not even in the first few results (see screenshots)
Eclipse Che:

VSCode:

CC @thierryk12

hkolvenbach · 2018-01-09T17:19:30Z

the PR is updated, to test it you need to enable the clangd language server under installers and use silexica/tools:nightly as a base image (it has an integrated clangd binary)

tolusha · 2018-01-10T10:08:59Z

agents/ls-clang/src/main/resources/installers/1.0.0/org.eclipse.che.ls.clang.script.sh

+#    }
+#
+## Red Hat Enterprise Linux 6
+#############################


Ideally we have to support all possible linux distributions

yes, i will think about a way to install it through the script

tolusha · 2018-01-10T10:20:00Z

@hkolvenbach
I've tested the PR. It seems working well.

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

dkuleshov · 2018-01-24T13:37:02Z

...he-plugin-clangd-lang-server/src/main/java/org/eclipse/plugin/clangd/inject/ClangModule.java

+      };
+  private static final String MIME_TYPE = "text/x-cpp";
+
+  // @Override


I just would like to remind that we're trying to avoid having comments of such kind when we do merge to master, do you think this section could be removed?

good point, i will go through it and remove those

dkuleshov · 2018-01-24T13:40:40Z

...he-plugin-clangd-lang-server/src/main/java/org/eclipse/plugin/clangd/inject/ClangModule.java

+import org.eclipse.che.plugin.cpp.projecttype.CppProjectType;
+import org.eclipse.plugin.clangd.languageserver.ClangDLanguageServerLauncher;
+
+/** @author Alexander Andrienko */


Seems like copy/pasted author?

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

hkolvenbach · 2018-01-29T15:14:34Z

@dkuleshov I removed the comments. I think the PR is ready to be merged, if there are no more suggestions from you or the Che team.

BTW: I picked clang 6 (will be released soon anyway), because clangD 5 was not working properly with the Che language protocol client

tolusha · 2018-01-30T07:01:58Z

agents/ls-clang/src/main/resources/installers/1.0.0/org.eclipse.che.ls.clang.json

+    "description": "Clangd intellisense for C/C++ projects.",
+    "dependencies": [],
+    "properties": {}
+  }


It is supposed to be a new line.

tolusha · 2018-01-30T07:05:34Z

agents/ls-clang/src/main/resources/installers/1.0.0/org.eclipse.che.ls.clang.script.sh

+command -v tar >/dev/null 2>&1 || { PACKAGES=${PACKAGES}" tar"; }
+command -v curl >/dev/null 2>&1 || { PACKAGES=${PACKAGES}" curl"; }
+command -v wget >/dev/null 2>&1 || { PACKAGES=${PACKAGES}" wget"; }
+test "$(id -u)" = 0 || SUDO="sudo -E"


Have a look at
https://github.com/eclipse/che/blob/2c2bd6637c8169018e2b33397f9d738bbf82a0b1/agents/ls-csharp/src/main/resources/installers/1.0.1/org.eclipse.che.ls.csharp.script.sh#L12

The scripts have been modified. For instance

is_current_user_root() { test "$(id -u)" = 0 } is_current_user_sudoer() { sudo -n true > /dev/null 2>&1 } set_sudo_command() { if is_current_user_sudoer && ! is_current_user_root; then SUDO="sudo -E"; else unset SUDO; fi }

have been added to test if user is sudo or isn't

thanks, i will take a look!

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

tolusha

Pls, update pom.xml to set 6.1.0-SNAPSHOT version.

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

hkolvenbach · 2018-02-12T15:02:33Z

done!

tolusha · 2018-02-13T06:47:35Z

@vparfonov What are the next steps here?

skabashnyuk · 2018-02-13T10:10:09Z

pom.xml

+            <dependency>
+                <groupId>org.eclipse.che.plugin</groupId>
+                <artifactId>che-plugin-clangd-lang-server</artifactId>
+


redundant line

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

vparfonov · 2018-02-13T11:36:21Z

ci-build

codenvy-ci · 2018-02-13T15:16:18Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4469/

skabashnyuk · 2018-02-14T07:18:21Z

ci-test

codenvy-ci · 2018-02-14T12:22:11Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:7516
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

musienko-maxim · 2018-02-15T11:11:54Z

The selenium tests passed without regressions

hkolvenbach requested review from riuvshin, skabashnyuk, tolusha and vparfonov as code owners November 22, 2017 14:59

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Nov 22, 2017

hkolvenbach force-pushed the kolvenbach.clangd_v3 branch 3 times, most recently from f9a44e4 to b5c5681 Compare December 5, 2017 08:18

0Grit mentioned this pull request Dec 21, 2017

Investigate integration of clangd language server #4991

Closed

hkolvenbach force-pushed the kolvenbach.clangd_v3 branch from b5c5681 to 307ae28 Compare January 9, 2018 17:18

tolusha reviewed Jan 10, 2018

View reviewed changes

gazarenkov added kind/enhancement A feature request - must adhere to the feature request template. and removed kind/task Internal things, technical debt, and to-do tasks to be performed. labels Jan 14, 2018

Hanno Kolvenbach added 2 commits January 23, 2018 12:25

ClangD ported to Che6

d18da95

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

removed launch scripts

4238b40

dkuleshov reviewed Jan 24, 2018

View reviewed changes

removed comments, updated script for other OS

a876ac5

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

hkolvenbach changed the title ~~WIP: Initial ClangD support for Eclipse Che~~ ClangD support for Eclipse Che Jan 29, 2018

tolusha reviewed Jan 30, 2018

View reviewed changes

updated language server script

ce05ba8

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

hkolvenbach force-pushed the kolvenbach.clangd_v3 branch from 3e109f5 to fb9a34d Compare February 6, 2018 08:24

added new line

35ee27b

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

hkolvenbach force-pushed the kolvenbach.clangd_v3 branch from fb9a34d to 35ee27b Compare February 6, 2018 08:25

vparfonov approved these changes Feb 6, 2018

View reviewed changes

tolusha approved these changes Feb 8, 2018

View reviewed changes

merged master and updated version

b7e86a6

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

dkuleshov approved these changes Feb 13, 2018

View reviewed changes

skabashnyuk reviewed Feb 13, 2018

View reviewed changes

Hanno Kolvenbach added 2 commits February 13, 2018 11:35

updated version

a67e17d

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

fixed newline

9baa64a

Signed-off-by: Hanno Kolvenbach <kolvenbach@silexica.com>

skabashnyuk approved these changes Feb 13, 2018

View reviewed changes

vparfonov merged commit 0a7cf2e into eclipse-che:master Feb 15, 2018

riuvshin mentioned this pull request Feb 15, 2018

set correct version in newly contributed plugin #8807

Merged

hkolvenbach deleted the kolvenbach.clangd_v3 branch February 16, 2018 08:32

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 23, 2018

benoitf added this to the 6.2.0 milestone Feb 23, 2018

ClangD support for Eclipse Che #7516

ClangD support for Eclipse Che #7516

Conversation

hkolvenbach commented Nov 22, 2017 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

hkolvenbach commented Nov 23, 2017

AndrienkoAleksandr commented Nov 23, 2017 • edited Loading

tolusha commented Nov 23, 2017 • edited Loading

evidolob commented Nov 23, 2017

AndrienkoAleksandr commented Nov 23, 2017

tolusha commented Nov 28, 2017

hkolvenbach commented Dec 5, 2017 • edited Loading

tolusha commented Dec 6, 2017

hkolvenbach commented Dec 12, 2017

hkolvenbach commented Dec 12, 2017

tolusha commented Dec 12, 2017 • edited Loading

hkolvenbach commented Jan 9, 2018

hkolvenbach commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolusha commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkolvenbach commented Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolusha left a comment

Choose a reason for hiding this comment

hkolvenbach commented Feb 12, 2018

tolusha commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vparfonov commented Feb 13, 2018

codenvy-ci commented Feb 13, 2018

skabashnyuk commented Feb 14, 2018

codenvy-ci commented Feb 14, 2018

musienko-maxim commented Feb 15, 2018

hkolvenbach commented Nov 22, 2017 •

edited

Loading

AndrienkoAleksandr commented Nov 23, 2017 •

edited

Loading

tolusha commented Nov 23, 2017 •

edited

Loading

hkolvenbach commented Dec 5, 2017 •

edited

Loading

tolusha commented Dec 12, 2017 •

edited

Loading

hkolvenbach commented Jan 29, 2018 •

edited

Loading