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

Stop creating .clangd file per Eclipse project if the config is defined elsewhere #227

Closed
travkin79 opened this issue Jan 11, 2024 · 20 comments · Fixed by #229
Closed

Stop creating .clangd file per Eclipse project if the config is defined elsewhere #227

travkin79 opened this issue Jan 11, 2024 · 20 comments · Fixed by #229
Milestone

Comments

@travkin79
Copy link
Contributor

travkin79 commented Jan 11, 2024

In our team we'd like to replace CDT's classic C/C++ editor with the LSP-based C/C++editor. But at least for our C/C++ projects we face a problem that complicates usage of clangd and the LSP-based C/C++ editor.

Issue
CDT-LSP (class org.eclipse.cdt.lsp.internal.clangd.ClangdConfigurationManager) assumes that each C/C++ Eclipse project uses a .clangd file in its root folder. But for our hierarchical projects that assumption leads to creation of .clangd files per project with wrong settings. These files break our clangd configuration.

Our project setup

  • We have a folder hierarchy with multiple Eclipse C/C++ projects in sub-directories.
  • Instead of having a .clangd file or a compile_commands.json per project, we have a compile_commands.json in our root folder with details for all our projects in the sub-directories. It seems, clangd does not need our compile_commands.json, since its default behavior is smart enough to find the relevant files in parent folders.
  • We also have a bash script file in our root folder that sets the clangd path and some arguments for clangd. We use that file in our preferences: Window -> preferences -> Editor (LSP) -> clangd -> Path.
  • In our case, the compile_commands.json is being created outside Eclipse.

Do you have an idea how CDT-LSP could drop the false assumption and avoid erroneously creating .clangd files?

@jonahgraham
Copy link
Member

@travkin79 I don't have an immediate answer for you on this. However at our CDT call yesterday I think something similar was being discussed by @Kummallinen and @ghentschke in case they have input here.

@jens-elmenthaler
Copy link

I would possibly argue that the problem is in org.eclipse.cdt.lsp.internal.clangd.CProjectChangeMonitor, where the assumption is that if there are build settings for a given, then this project needs 1) a .clangd file (wrong in our case) and 2) to tell clangd where to look for the corresponding compile_commands.json (also wrong in our case).
I understand, of course, that there are cases where you want to have this .clangd file with exactly the resulting content. I just wonder how to distinguish properly.
I would believe CDT LSP should take care of this only if it also controls the existing of the compile_commands.json. Is there something like this?

@ghentschke
Copy link
Contributor

ghentschke commented Jan 11, 2024

My first idea is to prevent the creation of the .clangd file via an OSGi service component wich can be implemented by other bundles.

@ghentschke
Copy link
Contributor

ghentschke commented Jan 11, 2024

What about:

public interface ClangdConfiguration {
// return true if the .clangd file should be generated in the project root folder if it's missing.
boolean createClangdFile();
}

@jens-elmenthaler
Copy link

The decision should be per project, I would believe. Is this service obtained per project? Otherwise we would possibly add the project as parameter:
boolean createClangdFile(IProject project);

@jens-elmenthaler
Copy link

In general, however, I tend to argue that the default behavior of clangd to look for compile_command.json in one of the parent directories of the processed source files is probably just perfect for most of the projects. So, this should also be a default setup (or at least to be achievable without writing your own plugin) with CDT LSP.

@ghentschke
Copy link
Contributor

ghentschke commented Jan 12, 2024

In general, however, I tend to argue that the default behavior of clangd to look for compile_command.json in one of the parent directories of the processed source files is probably just perfect for most of the projects. So, this should also be a default setup (or at least to be achievable without writing your own plugin) with CDT LSP.

That's right. But cmake generates the compile_commands.json file in the build folder. This is unfortunately not a parent folder in CDTs cmake project.
@travkin79 : How do you generate the compile_commands.json in the root folder? Do you have multiple build configs?

One purpose of the org.eclipse.cdt.lsp.internal.clangd.ClangdConfigurationManager is to detect the correct compile_commands.json in a multi-buildconfig-environment. If the active build config changes, the path to the compile_commands.json in the .clangd file is adjusted to the compile_commands.json in the active build folder. clangd detects the changes and uses the new compile_commands.json.

I think the mentioned behavior above is needed by @Kummallinen

The decision should be per project, I would believe. Is this service obtained per project? Otherwise we would possibly add the project as parameter:
boolean createClangdFile(IProject project);

Yes, you are right. The project is needed to determine if it's the root project.

@travkin79
Copy link
Contributor Author

Hi @ghentschke,
We use Bazel for building our projects. Our bazel script creates the compile_commands.json and moves it to our multi-project repository's root directory. For creating the compile commands for all our Eclipse projects we use the open source project https://github.com/kiron1/bazel-compile-commands. With bazel we could use multi-build config, but we don't. Instead, our compile_commands.json includes all the details for all our Eclipse projects and we use the same compile_commands.json for each of our projects in that particular repository.

ghentschke added a commit to ghentschke/eclipse-cdt-lsp that referenced this issue Jan 14, 2024
- Allows vendors to overwrite default behavior

fixes eclipse-cdt#227
@ghentschke
Copy link
Contributor

@travkin79 please check if the PR #229 would fulfill your needs.

ghentschke added a commit to ghentschke/eclipse-cdt-lsp that referenced this issue Jan 15, 2024
@travkin79
Copy link
Contributor Author

Hello @ghentschke,
Thank you very much for the PR. I'll check if the changes satisfy our needs and will let you know.

ghentschke added a commit to ghentschke/eclipse-cdt-lsp that referenced this issue Jan 15, 2024
ghentschke added a commit to ghentschke/eclipse-cdt-lsp that referenced this issue Jan 15, 2024
- Add org.eclipse.cdt.cmake.core to target definition

fixes eclipse-cdt#227
@travkin79
Copy link
Contributor Author

travkin79 commented Jan 15, 2024

Hello @ghentschke,
The PR #229 looks promising. Thank you.

I'm trying to implement a custom ClangdConfigurationFileManager OSGi service. I get some discouraged access warnings. I think, it's because of the following x-internal:=true entries in the MANIFEST.MF. Is that not public API by intention? I think, sub-classing ClangdConfigurationFileManager and overriding some of the methods is the easiest way of customization in our case.

Export-Package: org.eclipse.cdt.lsp.clangd;x-internal:=true,
 org.eclipse.cdt.lsp.internal.clangd;x-internal:=true

What is the best way to also customize the clangd path for certain projects and leave the settings for all other projects unchanged?

I also found a minor issue with the clangd plug-in activator. The following code produces some exceptions during Eclipse start. The call of PlatformUI.getWorkbench() happens before the Workbench is started and leads to IllegalStateException.

org.eclipse.cdt.lsp.internal.clangd.editor.ClangdPlugin.java:

cProjectChangeMonitor = Optional.ofNullable(
        PlatformUI.getWorkbench().getService(CProjectChangeMonitor.class))
        .map(m -> m.start());

Stacktrace:

Caused by: java.lang.IllegalStateException: Workbench has not been created yet.
	at org.eclipse.ui.PlatformUI.getWorkbench(PlatformUI.java:119)
	at org.eclipse.cdt.lsp.internal.clangd.editor.ClangdPlugin.start(ClangdPlugin.java:60)
	at org.eclipse.osgi.internal.framework.BundleContextImpl$2.run(BundleContextImpl.java:818)
	at org.eclipse.osgi.internal.framework.BundleContextImpl$2.run(BundleContextImpl.java:1)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:569)
	at org.eclipse.osgi.internal.framework.BundleContextImpl.startActivator(BundleContextImpl.java:810)
	... 53 more

I think, I fixed the IllegalStateException in org.eclipse.cdt.lsp by changing the method ClangdPlugin#start() to the following:

	@Override
	public void start(BundleContext context) throws Exception {
		super.start(context);
		plugin = this;
		ServiceTracker<IWorkspace, IWorkspace> workspaceTracker = new ServiceTracker<>(context, IWorkspace.class, null);
		workspaceTracker.open();
		workspace = workspaceTracker.getService();
		compileCommandsMonitor = new CompileCommandsMonitor(workspace).start();

		ServiceTracker<CProjectChangeMonitor, CProjectChangeMonitor> cProjectChangeMonitorTracker = new ServiceTracker<>(
				context, CProjectChangeMonitor.class,
				new ServiceTrackerCustomizer<CProjectChangeMonitor, CProjectChangeMonitor>() {

					@Override
					public CProjectChangeMonitor addingService(ServiceReference<CProjectChangeMonitor> reference) {
						CProjectChangeMonitor result = context.getService(reference);
						cProjectChangeMonitor = Optional.ofNullable(result);
						cProjectChangeMonitor.ifPresent(m -> m.start());
						return result;
					}

					@Override
					public void modifiedService(ServiceReference<CProjectChangeMonitor> reference,
							CProjectChangeMonitor service) {
					}

					@Override
					public void removedService(ServiceReference<CProjectChangeMonitor> reference,
							CProjectChangeMonitor service) {
						cProjectChangeMonitor = Optional.empty();
					}
				});
		cProjectChangeMonitorTracker.open();
		cProjectChangeMonitor = Optional.ofNullable(cProjectChangeMonitorTracker.getService());
	}

ghentschke added a commit to ghentschke/eclipse-cdt-lsp that referenced this issue Jan 17, 2024
- Fix bundle activator/Service Component Runtime (SCR) problem: The
Bundle Activator's start method must not rely upon SCR having activated
any of the bundle's components. However, the components can rely upon
the Bundle Activator's start method having been called. That is, there
is a happens-before relationship between the Bundle Activator's start
method being run and the components being activated.

fixes eclipse-cdt#227
@ghentschke
Copy link
Contributor

@travkin79 please take a look at my last commit in the PR #229.

I'm trying to implement a custom ClangdConfigurationFileManager OSGi service. I get some discouraged access warnings. I think, it's because of the following x-internal:=true entries in the MANIFEST.MF. Is that not public API by intention?

Yes, we haven't tagged it as a stable API right now. The org.eclipse.cdt.lsp.clangd package should become public. You should not use the *.internal.* packages, since they can be changed. Other projects which are much older (like LSP4E) haven't done that at all. I don't know.

I think, sub-classing ClangdConfigurationFileManager and overriding some of the methods is the easiest way of customization in our case.

Yes, that's what I had on my mind as I wrote that code. You should simply extend ClangdConfigurationFileManager for your purposes and publish it as a service component with a service.ranking > 0.

I also found a minor issue with the clangd plug-in activator. The following code produces some exceptions during Eclipse start. The call of PlatformUI.getWorkbench() happens before the Workbench is started and leads to IllegalStateException.

This should be fixed with my last commit now.

What is the best way to also customize the clangd path for certain projects and leave the settings for all other projects unchanged?

That's not possible at the moment, since we are supporting only one clangd LS per workspace. There is still this open issue #43

@travkin79
Copy link
Contributor Author

travkin79 commented Jan 17, 2024

Thank you very much, @ghentschke!

Yes, we haven't tagged it as a stable API right now. The org.eclipse.cdt.lsp.clangd package should become public.

Ok. I used only classes from that package anyway. Then, we'll wait for that package to become public API and will ignore the warnings until then.

I think, sub-classing ClangdConfigurationFileManager and overriding some of the methods is the easiest way of customization in our case.

Yes, that's what I had on my mind as I wrote that code. You should simply extend ClangdConfigurationFileManager for your purposes and publish it as a service component with a service.ranking > 0.

That's exactly what I did in a first prototype. It works as expected.

This should be fixed with my last commit now.

Yes, the exceptions do not occur anymore.

What is the best way to also customize the clangd path for certain projects and leave the settings for all other projects unchanged?

That's not possible at the moment, since we are supporting only one clangd LS per workspace. There is still this open issue #43

Oh, that's good to know. Thank you for the hint. We didn't know there can be only one language server per workspace.

We saw the language server extension point and thought we could provide a custom language server with adapted clangd path settings for our selected projects and use the default language server for all other projects. Our customized server provider was meant to be a sub-class of the original language server provider, but the class ClangdLanguageServerProvider comes from an internal package. Just implementing the interface ICLanguageServerProvider could be a work-around, but we'd have to copy most of the code from ClangdLanguageServerProvider. Because of #43, it seems, that approach wouldn't work.

Maybe, the clangd path could be customized in the same way as we customize the creation of .clangd files, i.e. by sub-classing the default implementation and programmatically select the projects where to apply the customization? I mean, I don't know, if the same language server could be used with paths or arguments varying per project. If that's possible, we wouldn't need to have multiple language servers.

We'll watch the issue #43.

ghentschke added a commit to ghentschke/eclipse-cdt-lsp that referenced this issue Jan 17, 2024
- make methods protected since they aren't part of the interface
anymore.

fixes eclipse-cdt#227
@ghentschke
Copy link
Contributor

I mean, I don't know, if the same language server could be used with paths or arguments varying per project.

My question would be, why would you like to use different language servers? Could the different setting be solved by writing them in the .clangd file in the projects root? It think that's what it is intended to be used for.

The LS properties in the projects are a little misleading at the moment as they are only used when the project is the initial project. That means a file from within that project is the first file to be opened and triggers the LS to be started.

@travkin79
Copy link
Contributor Author

travkin79 commented Jan 18, 2024

I mean, I don't know, if the same language server could be used with paths or arguments varying per project.

My question would be, why would you like to use different language servers? Could the different setting be solved by writing them in the .clangd file in the projects root? It think that's what it is intended to be used for.

In think, we don't really need multiple language servers. I only used a custom language server provider to customize the clangd path for certain Eclipse projects (we just point to a bash script in the git repository's root folder, there we also have our compile_commands.json for all our projects), since I didn't find another way to customize the clangd path. In best case, we'd find a solution to customize the clangd path for certain projects and leave the default settings unchanged for all other projects (i.e. use the common clangd path preferences for all other projects).

Well, at least one reason for not using .clangd files to set our custom clangd path is that we would have to find all projects in our multi-project git repository, write or adapt the path settings in the .clangd files and repeat that each time when new projects in that git repository are created. Thus, we thought, it's easier to have one bash script in the git repository's root folder that prepares and configures clangd for all our projects in that git repository. These settings usually do not change. Please note that in our case all the projects in that particular git repository use exactly the same clangd settings (and compile_commands.json).

In our (quite special) case, we have quite many Eclipse projects in one single git repository (I know that's not the best idea, but there are many, mainly historical and complex reasons for that) and there are many developers working on them.

I think, @jens-elmenthaler is more familiar with the rationale and background. Maybe he can give us more insight here.

@jens-elmenthaler
Copy link

jens-elmenthaler commented Jan 22, 2024

@ghentschke , @travkin79: The attached pull request is completely sufficient for our purposes. We only need to suppress the creation of .clangd files for each of our Eclipse C++ projects.

Just for the sake of completeness, and to help sorting out some noise that has piled up, our setup looks like this:
repo-root/compile_commands.json
comp1/.project
.cproject
src/...
...
comp1000/.project
.cproject
src/...

The important facts:

  • We have a large code base of C++. Eclipse became sluggish when importing this much code, so we created 100s of smaller projects where developers only import what they are working on.
  • We now switch to clangd and CDT LSP. A single clangd instance for the entire code base already works nicely. No, need for multiple.
  • I don't want to pollute 100s of Eclipse projects with each their own .clangd files, this is not necessary, just error-prone in the best case. If .clangd is needed at all, it should be at the repo root.

@travkin79
Copy link
Contributor Author

Hello @ghentschke,
Do you think, your PR #229 could be merged and released soon?

@ghentschke
Copy link
Contributor

@ruspl-afed do you see any open issues on #229?

ghentschke added a commit that referenced this issue Jan 26, 2024
- Allows vendors to overwrite default behavior

fixes #227
ghentschke added a commit that referenced this issue Jan 26, 2024
ghentschke added a commit that referenced this issue Jan 26, 2024
- Support cmake projects as well

fixes #227
ghentschke added a commit that referenced this issue Jan 26, 2024
- Add org.eclipse.cdt.cmake.core to target definition

fixes #227
ghentschke added a commit that referenced this issue Jan 26, 2024
- Fix bundle activator/Service Component Runtime (SCR) problem: The
Bundle Activator's start method must not rely upon SCR having activated
any of the bundle's components. However, the components can rely upon
the Bundle Activator's start method having been called. That is, there
is a happens-before relationship between the Bundle Activator's start
method being run and the components being activated.

fixes #227
ghentschke added a commit that referenced this issue Jan 26, 2024
- make methods protected since they aren't part of the interface
anymore.

fixes #227
@travkin79
Copy link
Contributor Author

Hello @ghentschke,
Thank you for merging the PR. Do you think, an official cdt lsp release (e.g. vers. 1.0.1) could follow soon, too?

@ghentschke
Copy link
Contributor

Hi, I'am workin on 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 a pull request may close this issue.

4 participants