search

eclipse-che / che

Kubernetes based Cloud Development Environments for Enterprise Teams
http://eclipse.org/che
Eclipse Public License 2.0
6.93k stars 1.19k forks source link

Java LS "Classpath is incomplete" warning when loading petclinic #13427

Open l0rd opened 5 years ago

l0rd commented 5 years ago

Description

After I have created a workspace using the following devfile

---
specVersion: 0.0.1
name: petclinic-dev-environment
projects:
  - name: petclinic
    source:
      type: git
      location: 'https://github.com/spring-projects/spring-petclinic.git'
components:
  - type: kubernetes
    alias: mvn-container
    reference: https://gist.githubusercontent.com/l0rd/52eda1d1b57878a218cba17c0c93a477/raw/9a2d56e777d22e77c25c45744caea35bb62d8688/petclinic.yaml
    selector:
      app.kubernetes.io/component: webapp
  - type: kubernetes
    reference: https://gist.githubusercontent.com/l0rd/52eda1d1b57878a218cba17c0c93a477/raw/9a2d56e777d22e77c25c45744caea35bb62d8688/petclinic.yaml
    selector:
      app.kubernetes.io/component: database
  - type: chePlugin
    id: redhat/java/0.45.0
  - type: chePlugin
    id: redhat/vscode-yaml/0.4.0
commands:
  - name: build
    actions:
      - type: exec
        component: mvn-container
        command: mvn package
        workdir: /projects/spring-petclinic

a classpath incomplete warning is shown every time a Java file is opened:

image

Reproduction Steps

Start a workspace using this devfile:

chectl workspace:start --devfile=https://gist.githubusercontent.com/l0rd/52eda1d1b57878a218cba17c0c93a477/raw/8ef7ff447a7164215395d6bc7d70b49363ebae86/devfile.yaml

OS and version:

macOS / minikube / nightly che-server / chectl / helm / jtd ls 0.45.0

l0rd commented 5 years ago

cc @tsmaeder @tolusha

svor commented 5 years ago

It looks like the project which was cloned after initializing jdt.ls workspace, isn't recognized as maven/gradel project. After reloading the browser tab or restarting jdt.ls (Clean the Java Language server workspace command), it works well. It is also reproducible in local theia instance: just need to start Theia with empty workspace folder and clone some java project.

l0rd commented 5 years ago

So it looks like a concurrency problem: if the project takes time to be cloned, and JDT LS in the meantime get started, the project won't be indexed correctly. A possible solution would be to guarantee that a LS is started only once the cloning operation has completed: @evidolob @benoitf what's your opinion?

tolusha commented 5 years ago

From my point of view, JDT LS should assume that project might not be ready when it starts and try to initialize it later

benoitf commented 5 years ago

@l0rd I think the LS is receiving multiple events. I'm unsure that it's possible to wait clone before sending events. All is asynchronous so you could have multiple clones over the lifecycle and you may have finished to clone a project before cloning a new one so it's better to work totally asynchronously.

tsmaeder commented 5 years ago

@benoitf @l0rd this is really the expected behaviour: many language servers (jdt.ls being one) will not pick up on new projects being created in the workspace root (in our case: /projects). The user is expected to reopen the folder or "add folder to workspace". Expecting the language servers to pick this case up consistently is going to be a losing battle, IMO. We currently have some kind of franken-UI: we still have the "Open Folder" menu, but we really expect projects to be in /projects and changes there to be picked up automatically. I think there are a couple of things we could do:

  1. When we start a workspace with projects, do the cloning before we start up the IDE. If we can't do this at the che container engine level, we might put a wait into the IDE for this condition. The code that clones the projects would have to notify that it's done.
  2. We could detect folder creation in /projects in the IDE and offer the user the option to reload the window. This would restart all plugins and make them pick up the changes.
tsmaeder commented 5 years ago

Alternative would be to treat each folder in /projects as a workspace root. When detecting a new folder, we would notify the IDE that a new root has been added. But:

  1. This would still be timing dependent: the LS needs metadat (pom.xml, etc.) to detect a project
  2. We would have to analyse how multi-root is different from single-root for language servers. Is the beaviour the one we want for common language servers?
l0rd commented 5 years ago

@tsmaeder I don't want to clone the git repo before the IDE is started because that means that the start of a workspace may take even more. What about restarting Theia (just restarting the pod or restarting the node app) once the cloning is done?

tsmaeder commented 5 years ago

We don't need to restart the pod or back end, a reload of the window is sufficient (reopen the /projects folder programmatically, even).

fbricon commented 5 years ago

Please test https://github.com/eclipse/eclipse.jdt.ls/pull/901, ask @snjeza for details

tsmaeder commented 5 years ago

We've done some experiments, and the verdict is mixed: the following language servers pick up new projects: PHP, Go, Typescript the following do not: Java, Python, C#

The picture is evenly split, I don't think we can expect language servers to pick up changes in general. That means we'll have to restart the language servers, which usually is done by doing a reload of the front end (which will trigger new copies of the back end plugins)

l0rd commented 5 years ago

@tsmaeder Java, with @snjeza fix, is now is picking up the new projects. But we still have Python and C# that do not behave as we would expect. So we should decide if, having an automatic refresh when one of these 2 LS is enabled, should be considered mandatory or not for Che 7 GA right?

tsmaeder commented 5 years ago

@l0rd jdt.ls does not detect this: there is a long-standing PR, but it would need some serious attention and testing before it can be merged. And as I argued above, it's not a solution for any other language servers out there. I would be strongly opposed to any solution that involves knowledge of a particular language server: we need to get away from the assumption that we know the set of plugins that are running on Che. So if we do a refresh, we need to do it for every type of project.

l0rd commented 5 years ago

@tsmaeder what's your proposal to mitigate the problem for Che 7.0.0?

tsmaeder commented 5 years ago

As we fix https://github.com/eclipse/che/issues/13784, I would replace the "confusing" actionable notification that says something like: "Some tools will not detect newly detect cloned projects: would you like to refresh the IDE?" I don't see that as mitigation to be fixed later, but as the right thing to do, as I've argued in a mail to the che-dev mailing list.

snjeza commented 5 years ago

@fbricon @tsmaeder @tolusha @svor @l0rd I have updated: https://github.com/eclipse/eclipse.jdt.ls/pull/901 and https://github.com/redhat-developer/vscode-java/pull/746

"Some tools will not detect newly detect cloned projects: would you like to refresh the IDE?"

You can use the "Detect and import Java projects" command.

tsmaeder commented 5 years ago

No, I cannot, because it won't work for anything other than Java. Let's stop thinking like a product and start thinking like a platform. @snjeza I greatly applaud your efforts to make this happen in jdt.ls, but it won't change anything for python, c# and other language servers. The fact that the git plugin explicitly wants to add newly cloned stuff to the VS Code workspace is another indicator to me that we need to accept the fact that language servers not picking up new stuff is not a bug, but to be expected and dealt with.

svor commented 4 years ago

@tsmaeder @snjeza I've tested it on the next way:

  1. Create an empty workspace in minishift
  2. Import 2 projects into the workspace
  3. Click Now on the windoe with text: A new file was added. Do you want to import projects?

Verdict: After some time all projects were detected and imported but there was no information about the process of importing and it was hard to understand that jdt.ls was working and importing dependencies.

snjeza commented 4 years ago

@svor I see the Java LS progress messages ( Importing Maven Projects, Building workspace ...) when testing in VS Code or theia. Will check in minishift.

JPinkney commented 4 years ago

Since this is the last blocker I wanted to do a write up explaining everything I’ve found so far:

First, I’m not sure if the issue has disappeared as a result of some other fix or if there’s another issue that’s the problem such as internet speed etc because I cannot reproduce on 7.0.0 rc 5 with latest che-theia using the devfile given. However, I can reproduce the issue by changing the devfile to clone no projects and then running > git clone https://github.com/spring-projects/spring-petclinic in the command palette when theia is loaded.

But in an attempt to switch to multi root workspaces to mitigate this in the future, with the multi root workspace code Thomas gave me [1] there seems to be a race condition between when the git clone happens and when the java language server registers the workspace/didUpdateWorkspaceFolders with the client. It seems to me that if a project is finished cloning before the java language server sends client/registerCapability with workspace/didUpdateWorkspaceFolders to the client then the client doesn’t actually send the changed folder to the java language server. I believe this is the reason why cloning a small java project such as the https://github.com/che-samples/console-java-simple will get a classpath issue with these new code changes. However, if you are trying to clone a larger java project, such as https://github.com/angelozerr/lsp4xml it does not seem to be the case and you won’t get the classpath issue.

I believe this issue is caused because che-7.0.0 theia branch does not have the vscode activation events implemented and thus vscode-java is getting started on workspace start which is then starting the java language server, rather than vscode-java waiting to start by using the activation events defined in the plugin. My thoughts are that if activation events were working the java language server would not start eagerly. Then we can trigger the cloning of all projects and then update the workspace root. Then and only then, the language server initialization would start (because the activation events would be triggered), rather than it starting eagerly like it does now. If all the projects are cloned before the server is started then the workspaceFolders in initialialization will be sent correctly to the server and then all will proceed as expected. Additionally, if any projects are cloned after this, the client would then send a workspace/updateWorkspaceFolders event and the language server will handle the new folder. I believe this should resolve the race condition.

Just from quickly inspecting a few of the vscode language plugins supported in Che, it seems like they have reasonable activation events such as activating when a language (such as python) is selected, or activating on command, workspace contains etc.

I’d like to know what other peoples opinions are on this.

[1] - https://github.com/JPinkney/che-theia/commits/workspace_roots_che

gorkem commented 4 years ago

@JPinkney Is there a branch where the activation events are implemented?

l0rd commented 4 years ago

@JPinkney @tsmaeder I have tested with Che nightly and the stack "Java with SpringBoot and MySQL" and I am not able to reproduce this issue anymore.

@rhopp can you verify if this issue is still reproduced in the Che 7 happy path scenario?

rhopp commented 4 years ago

@l0rd Will do that today.

JPinkney commented 4 years ago

Hello,

I've just tested the changes with some of the stacks. I tried to test as many language servers as I could and here are the result with the code changes:

👍 - means that validation, hover, document symbols, auto completion are working for the stack

Stack Working or Problems/Comments
Java Maven We believe this has a race condition. Investigating whether the race is in languageserver-node library or jdt ls
Python 👍
.NET Core Nothing is working
Go 👍
NodeJS Express Web Application 👍
PHP validation and auto completion are working. Document symbols and hover were not

Stacks were tested by adding a reference to a meta.yaml:

- type: cheEditor
  reference: >-
      https://gist.githubusercontent.com/JPinkney/fd653195c71604e3b3002e46393b0f18/raw/a06f2f9d762f37138de18919365ea915e2020bb6/reference.yaml

into the devfile.

The meta.yaml references a test image called jpinkney/che-theia-test11:latest which is just the resulting image of building the che-theia theia dockerfile with these changes

tsmaeder commented 4 years ago

We're running into https://github.com/eclipse/eclipse.jdt.ls/issues/1148

tsmaeder commented 4 years ago

Also see https://github.com/OmniSharp/omnisharp-vscode/issues/1889

tsmaeder commented 4 years ago

@JPinkney what were you steps to test the language servers? How did you create workspaces, projects, etc?

tsmaeder commented 4 years ago

Timing issue is well known: https://github.com/microsoft/vscode-languageserver-node/issues/451

nickboldt commented 4 years ago

Based on length conversations today, this isn't ready for 7.0. Will aim for 7.1 but might be 7.2 depending on delivery of upstream requirements.

l0rd commented 4 years ago

@nickboldt @rhopp @slemeur does it mean that this is not considered a blocker anymore (we are ok to release 7.0.0 even if the issue is still here) or that the issue described above is not reproducible anymore (we keep it open just to track the work on multi-root workspaces)?

l0rd commented 4 years ago

@rhopp @dmytro-ndp @eclipse/eclipse-che-qa are you still able to reproduce this issue?

dmytro-ndp commented 4 years ago

@l0rd: we didn't encounter that issue in Happy path tests since Che 7.0.0.GA.

sunix commented 4 years ago

@rhopp @dmytro-ndp @eclipse/eclipse-che-qa @l0rd I guess we can close that issue and do not merge https://github.com/eclipse/che-theia/pull/412

l0rd commented 4 years ago

I have tested on che.openshift.io but not able to reproduce there neither. Closing.

tsmaeder commented 4 years ago

We still want to move to a system that manages workspace roots: language servers are not certain to pick up new projects in a root folder.

sunix commented 4 years ago

@tsmaeder it looks like you are talking about a different issue. This one is about Java LS "Classpath is incomplete".

tsmaeder commented 4 years ago

We need to wait for a new release of vscode-languageserver-node and a rebuild of vscode-java in order to proceed with moving to a multi-root based che workspace

tsmaeder commented 4 years ago

Reactivating: I suspect with the later activation of the plugins, our use cases should work. Retest and move towards a merge: However: need to collect feedback from dev team about readiness for multi-root workspace.

tsmaeder commented 4 years ago

Blocked until tasks work with multi-root.

che-bot commented 3 years ago

Issues go stale after 180 days of inactivity. lifecycle/stale issues rot after an additional 7 days of inactivity and eventually close.

Mark the issue as fresh with /remove-lifecycle stale in a new comment.

If this issue is safe to close now please do so.

Moderators: Add lifecycle/frozen label to avoid stale mode.

ericwill commented 3 years ago

Blocked until tasks work with multi-root.

This is no longer the case right? I think we should be unblocked now cc @tsmaeder

tsmaeder commented 3 years ago

This should be fixed.