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

Add experimental support for Bazel (eclipse#543) #1404

Closed
wants to merge 1 commit into from

Conversation

PMitrafanau
Copy link

Experimental support for Bazel (#543)

Introduces integration with the following plugin. Since it's in active development, the importer is disabled by default and has the lowest priority.

Currently the plugin does the following to provide rich development experience:

  • Project importer
  • Classpath container and its initializer

Signed-off-by: Pavel Mitrafanau [email protected]

@eclipse-ls-bot
Copy link

Can one of the admins verify this patch?

@fbricon
Copy link
Contributor

fbricon commented Apr 3, 2020

add to whitelist

@PMitrafanau
Copy link
Author

@fbricon Hello. I'm wondering if you have any ideas about failed test. Checked it locally, everything is ok.

@snjeza
Copy link
Contributor

snjeza commented Apr 3, 2020

test this please

@fbricon
Copy link
Contributor

fbricon commented Apr 3, 2020

flaky test, ignore it.

So, I've tested this PR with vscode-java, setting "java.import.bazel.enabled":true in settings.json and opened https://github.com/bazelbuild/examples/tree/master/java-tutorial.

It failed with:

[Error - 2:28:23 PM] 3-Apr-2020 2:28:23 PM Initialization failed 
null
java.lang.NullPointerException
	at com.salesforce.b2eclipse.config.ImportOrderResolver.resolveModulesImportOrder(ImportOrderResolver.java:60)
	at com.salesforce.b2eclipse.config.BazelEclipseProjectFactory.importWorkspace(BazelEclipseProjectFactory.java:180)
	at org.eclipse.jdt.ls.core.internal.managers.BazelProjectImporter.importToWorkspace(BazelProjectImporter.java:60)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.importProjects(ProjectsManager.java:104)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.initializeProjects(ProjectsManager.java:94)
	at org.eclipse.jdt.ls.core.internal.handlers.InitHandler$1.runInWorkspace(InitHandler.java:196)
	at org.eclipse.core.internal.resources.InternalWorkspaceJob.run(InternalWorkspaceJob.java:42)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

Now, more generally on Bazel support in jdt.ls, as I said before, I think this should be done as a jdt.ls extension. There's nothing in this PR that can't prevent a bazel jdt.ls extension to run, loaded by jdt.ls on startup. The importer is provided via an extension point, the preferences can be accessed through:

Map<> configuration = JavaLanguageServerPlugin.getInstance().getPreferencesManager().getPreferences().asMap();
boolean enabled = MapFlattener.getBoolean(configuration, IMPORT_BAZEL_ENABLED, false);

All you need to do is get that jdt.ls extension and its dependencies to be loaded by the client. In the case of vscode-java, you can list all your jars in package.json, like https://github.com/redhat-developer/vscode-java/wiki/Contribute-a-Java-Extension#modify-packagejson and that's it.

Having said that, I believe more changes will be needed to be made in jdt.ls core to make bazel support work properly, those I'm more inclined to get in:

We also need to be able to dynamically register listeners for specific build files (BUILD/WORKSPACE), so their modifications can be sent to jdt.ls

@fbricon
Copy link
Contributor

fbricon commented Apr 6, 2020

After disabling the bazel, importer, reopening the workspace threw some other error:

[Error - 5:39:16 PM] 6-Apr-2020 5:39:16 PM Problems occurred when invoking code from plug-in: "org.eclipse.core.resources".
Attempt to retrieve the classpath of a Bazel Java project prior to setting up the Bazel workspace.
java.lang.IllegalStateException: Attempt to retrieve the classpath of a Bazel Java project prior to setting up the Bazel workspace.
	at com.salesforce.b2eclipse.classpath.BazelClasspathContainer.getClasspathEntries(BazelClasspathContainer.java:120)
	at org.eclipse.jdt.internal.core.JavaModelManager.containerPutIfInitializingWithSameEntries(JavaModelManager.java:792)
	at org.eclipse.jdt.internal.core.SetContainerOperation.executeOperation(SetContainerOperation.java:53)
	at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:736)
	at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:802)
	at org.eclipse.jdt.internal.core.JavaModelManager.getClasspathContainer(JavaModelManager.java:2119)
	at org.eclipse.jdt.core.JavaCore.getClasspathContainer(JavaCore.java:3753)
	at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:3161)
	at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:3325)
	at org.eclipse.jdt.internal.core.JavaProject.getResolvedClasspath(JavaProject.java:2409)
	at org.eclipse.jdt.internal.core.DeltaProcessor.deleting(DeltaProcessor.java:1155)
	at org.eclipse.jdt.internal.core.DeltaProcessor.resourceChanged(DeltaProcessor.java:2072)
	at org.eclipse.jdt.internal.core.DeltaProcessingState.resourceChanged(DeltaProcessingState.java:477)
	at org.eclipse.core.internal.events.NotificationManager$1.run(NotificationManager.java:305)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.core.internal.events.NotificationManager.notify(NotificationManager.java:295)
	at org.eclipse.core.internal.events.NotificationManager.handleEvent(NotificationManager.java:271)
	at org.eclipse.core.internal.resources.Workspace.broadcastEvent(Workspace.java:375)
	at org.eclipse.core.internal.resources.Resource.broadcastPreDeleteEvent(Resource.java:1846)
	at org.eclipse.core.internal.resources.Resource.delete(Resource.java:742)
	at org.eclipse.core.internal.resources.Project.delete(Project.java:320)
	at org.eclipse.jdt.ls.core.internal.managers.StandardProjectsManager.deleteInvalidProjects(StandardProjectsManager.java:140)
	at org.eclipse.jdt.ls.core.internal.managers.StandardProjectsManager.cleanInvalidProjects(StandardProjectsManager.java:109)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.initializeProjects(ProjectsManager.java:91)
	at org.eclipse.jdt.ls.core.internal.handlers.InitHandler$1.runInWorkspace(InitHandler.java:196)
	at org.eclipse.core.internal.resources.InternalWorkspaceJob.run(InternalWorkspaceJob.java:42)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

@PMitrafanau
Copy link
Author

Hi @fbricon,

Bazel java-tutorial project import fails as its build definition is in broken state. Moreover this was done intentionally. Currently our plugin doesn’t support “non-buildable” project import. We updated README to reflect this fact.
To make it more clear to a user, we made a few changes and now throw IllegalStateException with corresponding details in mentioned cases.

We fixed the exception after disabling the importer and reopening the project.

Currently we work on moving our changes from eclipse.jdt.ls into our plugin itself and package it as jdt.ls extension. Thank you for the valuable advice!

At a glance it’s not clear what is the purpose of IBuildSupport abstraction and how it is used inside jdt.ls core.

Also I would appreciate it if you can provide more details regarding changes in jdt.ls core (decoupling from build systems and new extension point). Do you have dedicated issues to track the process?

@fbricon
Copy link
Contributor

fbricon commented Apr 14, 2020

Basically IBuildSupport was introduced to be able to take care of project synchronization when a build file has changed, and you need to make some eclipse config changes accordingly (eg. need to compile to a different Java version, update classpath...).
Anything in jdt.ls that directly references Maven or Gradle should be refactored to be Build tool agnostic, hence the idea of that IBuildSupport.
When we first added IBuildSupport, we thought introducing an extension point would be overkill, as we didn't expect to open up jdt.ls to 3rd party tools, but things have changed obviously.
There are no dedicated issues related to decoupling yet, but I'll open one soon.

@PMitrafanau
Copy link
Author

Thanks for the update. I'm closing the PR. May I ask you to mention the issue in this thread after you create it?

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.

4 participants