[#242] Make ClangdConfigurationFileManager configurable

[ghentschke]

since its needed by some vendors to overwrite certain methods it should be made available.

part of #276

[ruspl-afed]

We discussed this already: promoting [OSGi component] implementation to API won't do us any good. Because it will invite vendors to inherit our internal implementation and after this any change in our internal implementation will impact (break) vendors. Do we need this?

We already have public interface. We may supply vendors with building blocks for custom implementation. We may even introduce an abstract class with carefully defined hooks. But please don't invite vendors to inherit our internal implementation, it never ended well.

[ghentschke]

Because it will invite vendors to inherit our internal implementation and after this any change in our internal implementation will impact (break) vendors. Do we need this?

I know. It was a wish from @travkin79 to subclass the ClangdConfigurationFileManager. @travkin79 is is possible for to to implement the ClangdCProjectDescriptionListener interface as OSGi service and not extending ClangdConfigurationFileManager?

[travkin79]

I know. It was a wish from @travkin79 to subclass the ClangdConfigurationFileManager. @travkin79 is is possible for to to implement the ClangdCProjectDescriptionListener interface as OSGi service and not extending ClangdConfigurationFileManager?

I'd have to check the suggested interface to answer this, but I'm not in office for some time. Maybe you could check that using my requirements from the following, @ghentschke? I quickly looked at the interface and I don't see how I could achieve the following without needing to copy code from the original service implementation.

My goal is just to switch off the creation of .clangd files for certain projects (see issue #227), but keep the remaining default behavior and I'd like to avoid re-implementing (i.e. copying) code from clangd internal packages. It's not important how exactly I can achieve that goal. I don't need to subclass ClangdConfigurationFileManager if there is another simple way of achieving that.

[ddscharfe]

You could register your service manually in the start method of your Activator, something like this:

    public void start(BundleContext bundleContext) throws Exception {
              // ...
        ClangdCProjectDescriptionListener originalService = bundleContext.getService(bundleContext.getServiceReference(ClangdCProjectDescriptionListener.class));
        ClangdCProjectDescriptionListener decoratedService = ...; // your own service implementation which can use originalService
              Hashtable<String, Object> properties = new Hashtable<>();
              properties.put("service.ranking", 10000);
              context.registerService(ClangdCProjectDescriptionListener.class.getName(), decoratedService, properties);     
    }

@ruspl-afed is this the right way to do this with OSGi? Another way might be to use the @Reference annotation to inject the original service into the decorator service directly, but this lead to cyclic dependency errors when I tested it.

[travkin79]

I checked the code. Sorry that it took so long.

I know. It was a wish from @travkin79 to subclass the ClangdConfigurationFileManager. @travkin79 is is possible for to to implement the ClangdCProjectDescriptionListener interface as OSGi service and not extending ClangdConfigurationFileManager?

Hello @ghentschke, @ddscharfe, and @ruspl-afed, technically, I think, I could implement the interface ClangdCProjectDescriptionListener instead of sub-classing ClangdConfigurationFileManager which is a class implementing the same interface. But I see a major problem with that solution:

What I need, is basically exactly the behavior implemented in ClangdConfigurationFileManager with a very small change. For a certain set of projects, I want to avoid the creation of .clangd files. By sub-classing ClangdConfigurationFileManager I only had to override one method (see code example below) and register an OSGi services replacing the original ClangdConfigurationFileManager.

    @Override
    public boolean enableSetCompilationDatabasePath(IProject project) {
        // do not create a .clangd file for certain set of projects
        return !CustomProjectSelector.isSpecialProject(project)
                && super.enableSetCompilationDatabasePath(project);
    }

When implementing the interface ClangdCProjectDescriptionListener instead of sub-classing ClangdConfigurationFileManager, I would basically have to copy all the code from ClangdConfigurationFileManager and doing so introduce a much tighter coupling to the CDT LSP internals than before. Copying code for re-use is definitely not a good idea and leads to maintenance nightmares.

I think, we need another solution here to make this customization less painful. Maybe we need an additional interface & (replaceable) OSGi service that can be used to decide whether to create a .clangd file or not and that service would be used by the ClangdConfigurationFileManager (favor composition over inheritance). This way, ClangdConfigurationFileManager's main behavior could be kept unchanged and kept not being part of the public API.

Maybe you have other, better ideas? Maybe some of these options mentioned by @ruspl-afed?

• We may supply vendors with building blocks for custom implementation.
• We may even introduce an abstract class with carefully defined hooks.

[ddscharfe]

  public void start(BundleContext bundleContext) throws Exception {
              // ...
      ClangdCProjectDescriptionListener originalService = bundleContext.getService(bundleContext.getServiceReference(ClangdCProjectDescriptionListener.class));
      ClangdCProjectDescriptionListener decoratedService = ...; // your own service implementation which can use originalService
              Hashtable<String, Object> properties = new Hashtable<>();
              properties.put("service.ranking", 10000);
              context.registerService(ClangdCProjectDescriptionListener.class.getName(), decoratedService, properties);       
  }

@travkin79 have you tried my suggested solution? This way you should be able to implement the interface, do the checks and delegate to the default implementation without copying any implementation.

[travkin79]

@travkin79 have you tried my suggested solution? This way you should be able to implement the interface, do the checks and delegate to the default implementation without copying any implementation.

Hi @ddscharfe, I guess, I do not completely understand your proposal.

• If ClangdConfigurationFileManager is not public API and the interface ClangdCProjectDescriptionListener stays as it is, I don't see how I could reuse the existing service. There is no method, I could use to tell the original service to switch off creation of .clangd files for certain projects. The only visible method in this case is handleEvent(CProjectDescriptionEvent event, MacroResolver macroResolver). I could use that method to check the project (given in the event), but I don't see how to tell the original service not to create the .clangd file for that case.
• Besides, replacing the original service during plug-in start-up sounds to me like kind of a hacky solution. Doesn't it mess up the OSGi service lifecycles?

[ddscharfe]

Hi @travkin79,

My idea was: your service implements ClangdCProjectDescriptionListener and therefore provides a handleEvent method. In this method you check the project and only delegate to the default service if your check succeeds. If I understand your use case correctly, I think this would work?

Besides, replacing the original service during plug-in start-up sounds to me like kind of a hacky solution. Doesn't it mess up the OSGi service lifecycles?

I agree, it feels a little hacky. I don't know enough about OSGi services, maybe @ruspl-afed can help. The basic use case however seems legit to me: Register a service with a higher priority, but delegate to the "default" service. A more declarative way would be to use the @Reference annotation to inject the default ClangdCProjectDescriptionListener into your ClangdCProjectDescriptionListener, but I couldn't find a way to do this without introducing a cycle.

[travkin79]

Hi @ddscharfe, Now I understand your proposal, but I think, it's not what I want.

Your assumption is that for my set of "special" projects I don't have to handle the events in ClangdCProjectDescriptionListener#handleEvent(...) at all.

I think that assumption is wrong. I think, I have to handle the event exactly like the ClangdConfigurationFileManager does it, i.e. setting the compilation data base and all the rest that ClangdConfigurationFileManager usually does, just not creating the .clangd files. (In our case, a compile_commands.json will be found in one of the project's parent folders.)

[ddscharfe]

I think that assumption is wrong. I think, I have to handle the event exactly like the ClangdConfigurationFileManager does it, i.e. setting the compilation data base and all the rest that ClangdConfigurationFileManager usually does, just not creating the .clangd files. (In our case, a compile_commands.json will be found in one of the project's parent folders.)

AFAIU ClangdConfigurationFileManager does nothing besides setting the path to the compile_commands.json in the .clangd file.

[travkin79]

Hi @ddscharfe, It seems, you're right. Your proposal might really work, although it still looks hacky. Though, I'll try to implement your approach and will test it.

[travkin79]

Hi @ddscharfe, I implemented your proposal, but it doesn't work.

• I removed my custom, higher priority service, so that the original OSGi service should be found.
• In my plug-in activator's start method I try to access the original service, i.e. the ClangdConfigurationFileManager, by calling bundleContext.getServiceReference(ClangdCProjectDescriptionListener.class), but that results in the following exception. It seems to be too early to access the service.

...
    ... 95 more
Root exception:
java.lang.NullPointerException: Cannot invoke "org.eclipse.core.resources.IWorkspace.addResourceChangeListener(org.eclipse.core.resources.IResourceChangeListener)" because "this.workspace" is null
    at org.eclipse.cdt.lsp.internal.clangd.editor.CompileCommandsMonitor.start(CompileCommandsMonitor.java:167)
    at org.eclipse.cdt.lsp.internal.clangd.editor.ClangdPlugin.start(ClangdPlugin.java:52)
    at org.eclipse.osgi.internal.framework.BundleContextImpl$2.run(BundleContextImpl.java:818)
    at org.eclipse.osgi.internal.framework.BundleContextImpl$2.run(BundleContextImpl.java:1)
...

I also did some experiments with naming the original service (similar to other dependency injection mechanisms) and trying to reference it from my custom service, but the injection did not happen. The reference stayed being null. It seems, there is only one instance of that service allowed. Retrieving the service using a ServiceCaller and a name filter didn't work either.

[ddscharfe]

That's unfortunate, but now as you mention it, it kinda makes sense that the default ClangdCProjectDescriptionListener is not available yet. I'm sorry I cannot help at this point without further investigation, maybe @ruspl-afed can. I'd be really interested if this is principally possible with OSGi, which I could imagine it is.

If we cannot solve it the way I thought, having another service that can be used to decide whether to create a .clangd file or not, as you proposed, makes sense to me. Or maybe the additional service could provide the detection of the compile_commands.json path (e.g. in your special project case "../compile_commands.json). This would allow more customization. Another possibility would be a project setting, where the automatic compile_commands.json detection can be disabled/enabled project wise.

[travkin79]

Thanks @ddscharfe, Yes, I think, an additional service used to decide whether to create a .clangd file or not is the simplest way to solve that.

Actually, we don't need a custom detection of the compile_commands.json, since CDT LSP has a fallback strategy where it looks for the file in one of the projects' parent folders. That's exactly what we need and are already using.

The last option, enabling / disabling detection of the compile_commands.json project-wise, would also have several drawbacks for our use case. We have really many projects and would like to avoid having to configure each one of them. Besides, in our case, it would rather have to be enabling / disabling creation of .clangd files per project.

So, I would prefer option 1, an additional service that checks for a given project if a .clangd file is to be created. :-D

[ghentschke]

Yes, I think, an additional service used to decide whether to create a .clangd file or not is the simplest way to solve that.

I'll modify this PR in a way that it provides a service for a custom implementation for boolean enableSetCompilationDatabasePath(IProject project)

[ghentschke]

@travkin79 I think the last commit should be what you need. @ruspl-afed could you please review?

[travkin79]

@travkin79 I think the last commit should be what you need.

Thank you, @ghentschke. That works for me.