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

Multiple LS server instances per workspace #25

Closed
wants to merge 1 commit into from

Conversation

ghentschke
Copy link
Contributor

@ghentschke ghentschke commented Mar 3, 2023

This PR would be a basic implementation for #43

@ghentschke ghentschke force-pushed the multiple-servers branch 4 times, most recently from a22f720 to 654c683 Compare March 8, 2023 06:46
@ghentschke ghentschke marked this pull request as draft March 8, 2023 08:30
@ghentschke
Copy link
Contributor Author

I think there is no certain need to run one language server per C/C++project, because clangd determines the respective compile_comands.json for each project file. Either by defining a path to it in a .clangd file in the source folder or by copying the compile_commands.json from build folder into the source tree after building. These actions should be part of the toolchain and/or builder.
The current bug is, that the compile_commands.json used when opening a file outside of the project (e.g. standard lib header) is not always inferred from the right project file. We need a strategy for this here.

@ghentschke ghentschke force-pushed the multiple-servers branch 3 times, most recently from 9a805b4 to c029313 Compare March 13, 2023 07:29
import org.eclipse.core.runtime.IPath;
import org.eclipse.core.runtime.preferences.IEclipsePreferences;

public class PropertiesCompileCommandsDirLocator implements ICompileCommandsDirLocator {
Copy link
Member

Choose a reason for hiding this comment

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

If this class was an OSGi component, it could just bind required IWorkspace service

Copy link
Contributor Author

@ghentschke ghentschke Apr 27, 2023

Choose a reason for hiding this comment

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

It seem to be not a simple add:

 @Reference
 private IWorkspace workspace;

does not work. (from doc https://help.eclipse.org/latest/nftopic/org.eclipse.platform.doc.isv/reference/api/org/eclipse/core/resources/IWorkspace.html)

Copy link
Contributor Author

@ghentschke ghentschke Apr 27, 2023

Choose a reason for hiding this comment

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

also when adding @ Component:

@Component
public class PropertiesCompileCommandsDirLocator implements ICompileCommandsDirLocator {
	@Reference
	private IWorkspace workspace;

I think it has something to do how the class is instantiated by org.eclipse.core.runtime.IConfigurationElement.createExecutableExtension(String)

Copy link
Member

@ruspl-afed ruspl-afed Apr 27, 2023

Choose a reason for hiding this comment

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

It seem to be not a simple add

Yes, you are right, it requires a bit deeper redesign. We need to:

  1. define an interface that does what we need
  2. provide implementation for this interface via @Component
  3. obtain in instance of this interface and use it

For 1) I would expect an interface that has a method similar with current LSPPlugin#getCompileCommandsDirLocators, like

public interface CompileCommandsLocations {

   List<CompileCommandsLocation> locations();
}

that uses

public interface CompileCommandsLocation {

   Optional<IPath> resolve(URI document);

}

Then 2) in this case we may add one component to implement CompileCommandsLocations (currently it is CompileCommandsDirLocatorRegistry) and a few components to implement CompileCommandsLocation (currently it is PropertiesCompileCommandsDirLocator). We may optionally keep extension point as an alternative way to fulfill CompileCommandsLocations

And, 3) we can use ServiceCaller to interact with an instance of CompileCommandsLocations interface. No need to have these public synchronized methods in *Plugin classes. No need to obtain and keep an instance of IWorkspace

ServiceTracker<IWorkspace, IWorkspace> workspaceTracker = new ServiceTracker<>(context, IWorkspace.class, null);
workspaceTracker.open();
workspace = workspaceTracker.getService();
getCLanguageServerProvider();
Copy link
Member

Choose a reason for hiding this comment

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

Here we have a mix of low level OSGi API with public accessors to stateful activator. It looks more consistent to use OSGi components for CLanguageServerRegistry and CompileCommandsDirLocatorRegistry

bundles/pom.tycho Outdated Show resolved Hide resolved
@ghentschke
Copy link
Contributor Author

ghentschke commented Apr 28, 2023

I've to find a solution for opening external files (e.g. system header files) which will be opened by LSP4E in case of a Go-to-declaration event (F3). Currently LSP4E starts a new server for every external file. This server has no idea where to find a compile_commands.json since there is no in the system include path. My initial approach to change the LSP4E behavior in a way that external files will be opened in the same server context as the origin file has been rejected, because it's too server specific.

A possible solution has been proposed here by @ddscharfe

@ghentschke

This comment was marked as duplicate.

@ruspl-afed
Copy link
Member

This PR would be a basic implementation for #43

Shouldn't it go to PR description?

@ruspl-afed
Copy link
Member

@ghentschke should I try to restart this work using current state of the code?

@ghentschke
Copy link
Contributor Author

should I try to restart this work using current state of the code?

Any work is appreciated here! As I wrote yesterday, there should be some work done on LSP4E to support multiple language server properly. See LSP4E issue #694

@ghentschke
Copy link
Contributor Author

ghentschke commented Jul 19, 2023

A solution for the singleton attribute in the LSP4E LS definition extension might be, to define 2 LS: one singleton and one not singleton. They should be interlocked so that only one server definition is enabled at once.
The decision could be linked to the Enable project-specific settings and Prefer C/C++ Editor (LSP) flags in the project properties. If the flags are set, the non-singleton server definition is used. Otherwise not.

The external file problem for multiple servers could be solved by setting the path to the compile_commands.json in the clangd initializationOptions : https://clangd.llvm.org/extensions#compilation-commands

@ruspl-afed
Copy link
Member

@ghentschke is it still relevant?

@ghentschke
Copy link
Contributor Author

@ghentschke is it still relevant?

No, I think this feature has actually a very low priority. And I cannot see any advantage of multiple LS per workspace anymore.

@ghentschke ghentschke closed this May 29, 2024
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.

None yet

2 participants