[#227] Refactor ClangdConfigurationManager

ghentschke · ghentschke · commit 03354bf41456 · 2024-01-26T20:35:37.000+01:00
- 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
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/META-INF/MANIFEST.MF b/bundles/org.eclipse.cdt.lsp.clangd/META-INF/MANIFEST.MF
@@ -30,13 +30,10 @@ Require-Bundle: org.eclipse.cdt.lsp;bundle-version="0.0.0",
  org.eclipse.ui.ide;bundle-version="0.0.0",
  org.eclipse.ui.workbench;bundle-version="0.0.0",
  org.eclipse.ui.workbench.texteditor;bundle-version="0.0.0",
- org.eclipse.core.variables,
- org.eclipse.cdt.cmake.core
+ org.eclipse.core.variables;bundle-version="0.0.0"
 Service-Component: OSGI-INF/org.eclipse.cdt.lsp.clangd.BuiltinClangdOptionsDefaults.xml,
  OSGI-INF/org.eclipse.cdt.lsp.clangd.ClangdConfigurationFileManager.xml,
- OSGI-INF/org.eclipse.cdt.lsp.clangd.DefaultCProjectChangeMonitor.xml,
  OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.ClangdConfigurationAccess.xml,
  OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.ClangdFallbackManager.xml,
- OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.ClangdMetadataDefaults.xml,
- OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.DefaultMacroResolver.xml
+ OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.ClangdMetadataDefaults.xml
 Bundle-Activator: org.eclipse.cdt.lsp.internal.clangd.editor.ClangdPlugin
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/OSGI-INF/org.eclipse.cdt.lsp.clangd.DefaultCProjectChangeMonitor.xml b/bundles/org.eclipse.cdt.lsp.clangd/OSGI-INF/org.eclipse.cdt.lsp.clangd.DefaultCProjectChangeMonitor.xml
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.DefaultMacroResolver.xml b/bundles/org.eclipse.cdt.lsp.clangd/OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.DefaultMacroResolver.xml
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/CProjectChangeMonitor.java b/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/CProjectChangeMonitor.java
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdCProjectDescriptionListener.java b/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdCProjectDescriptionListener.java
@@ -14,44 +14,19 @@
 package org.eclipse.cdt.lsp.clangd;
 
 import org.eclipse.cdt.core.settings.model.CProjectDescriptionEvent;
-import org.eclipse.cdt.core.settings.model.ICProjectDescription;
-import org.eclipse.core.resources.IProject;
 
 /**
- * Vendors can implement this interface as OSGi service
+ * Vendors may implement this interface as OSGi service
  * with a service.ranking property > 0 to implement custom behavior
  * and to replace the {@link ClangdConfigurationFileManager}
  */
 public interface ClangdCProjectDescriptionListener {
-	String CLANGD_CONFIG_FILE_NAME = ".clangd"; //$NON-NLS-1$
 
 	/**
 	 * Called when the configuration of a CDT C/C++ project changes.
 	 * @param event
-	 * @param macroResolver
+	 * @param macroResolver helper class to resolve macros in builder CWD
 	 */
 	void handleEvent(CProjectDescriptionEvent event, MacroResolver macroResolver);
 
-	/**
-	 * Set the <code>CompilationDatabase</code> entry in the <code>.clangd</code> file which is located in the <code>project</code> root.
-	 * The <code>.clangd</code> file will be created, if it's not existing.
-	 * The <code>CompilationDatabase</code> points to the build folder of the active build configuration
-	 * (in case <code>project</code> is a managed C/C++ project).
-	 *
-	 * In the following example clangd uses the compile_commands.json file in the Debug folder:
-	 * <pre>CompileFlags: {CompilationDatabase: Debug}</pre>
-	 *
-	 * @param project managed C/C++ project
-	 * @param newCProjectDescription new CProject description
-	 * @param macroResolver helper to resolve macros in the CWD path of the builder
-	 */
-	void setCompilationDatabasePath(IProject project, ICProjectDescription newCProjectDescription,
-			MacroResolver macroResolver);
-
-	/**
-	 * Enabler for {@link setCompilationDatabasePath}. Can be overriden for customization.
-	 * @param project
-	 * @return true if the database path should be written to .clangd file in the project root.
-	 */
-	boolean enableSetCompilationDatabasePath(IProject project);
 }
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdConfigurationFileManager.java b/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdConfigurationFileManager.java
@@ -45,7 +45,7 @@
 
 /**
  * Default implementation of the {@link ClangdCProjectDescriptionListener}.
- * Can be replaced by vendors if needed. This implementation sets the path to
+ * Can be extended by vendors if needed. This implementation sets the path to
  * the compile_commands.json in the .clangd file in the projects root directory.
  * This is needed by CDT projects since the compile_commands.json is generated in the build folder.
  * When the active build configuration changes in managed build projects, this manager updates the path to the database in
@@ -55,6 +55,7 @@
  */
 @Component(property = { "service.ranking:Integer=0" })
 public class ClangdConfigurationFileManager implements ClangdCProjectDescriptionListener {
+	public static final String CLANGD_CONFIG_FILE_NAME = ".clangd"; //$NON-NLS-1$
 	private static final String COMPILE_FLAGS = "CompileFlags"; //$NON-NLS-1$
 	private static final String COMPILATTION_DATABASE = "CompilationDatabase"; //$NON-NLS-1$
 	private static final String SET_COMPILATION_DB = COMPILE_FLAGS + ": {" + COMPILATTION_DATABASE + ": %s}"; //$NON-NLS-1$ //$NON-NLS-2$
@@ -68,7 +69,19 @@ public void handleEvent(CProjectDescriptionEvent event, MacroResolver macroResol
 		setCompilationDatabasePath(event.getProject(), event.getNewCProjectDescription(), macroResolver);
 	}
 
-	@Override
+	/**
+	 * Set the <code>CompilationDatabase</code> entry in the <code>.clangd</code> file which is located in the <code>project</code> root.
+	 * The <code>.clangd</code> file will be created, if it's not existing.
+	 * The <code>CompilationDatabase</code> points to the build folder of the active build configuration
+	 * (in case <code>project</code> is a managed C/C++ project).
+	 *
+	 * In the following example clangd uses the compile_commands.json file in the Debug folder:
+	 * <pre>CompileFlags: {CompilationDatabase: Debug}</pre>
+	 *
+	 * @param project managed C/C++ project
+	 * @param newCProjectDescription new CProject description
+	 * @param macroResolver helper to resolve macros in the CWD path of the builder
+	 */
 	public void setCompilationDatabasePath(IProject project, ICProjectDescription newCProjectDescription,
 			MacroResolver macroResolver) {
 		if (project != null && newCProjectDescription != null) {
@@ -93,7 +106,11 @@ public void setCompilationDatabasePath(IProject project, ICProjectDescription ne
 		}
 	}
 
-	@Override
+	/**
+	 * Enabler for {@link setCompilationDatabasePath}. Can be overriden for customization.
+	 * @param project
+	 * @return true if the database path should be written to .clangd file in the project root.
+	 */
 	public boolean enableSetCompilationDatabasePath(IProject project) {
 		return Optional.ofNullable(LspPlugin.getDefault()).map(LspPlugin::getCLanguageServerProvider)
 				.map(provider -> provider.isEnabledFor(project)).orElse(Boolean.FALSE);
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/MacroResolver.java b/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/MacroResolver.java
@@ -13,12 +13,19 @@
 
 package org.eclipse.cdt.lsp.clangd;
 
+import org.eclipse.cdt.core.CCorePlugin;
 import org.eclipse.cdt.core.cdtvariables.CdtVariableException;
 import org.eclipse.cdt.core.settings.model.ICConfigurationDescription;
 
-public interface MacroResolver {
+/**
+ * Helper class to resolve macros in builder CWD
+ */
+public class MacroResolver {
 
 	public String resolveValue(String value, String nonexistentMacrosValue, String listDelimiter,
-			ICConfigurationDescription cfg) throws CdtVariableException;
+			ICConfigurationDescription cfg) throws CdtVariableException {
+		return CCorePlugin.getDefault().getCdtVariableManager().resolveValue(value, nonexistentMacrosValue,
+				listDelimiter, cfg);
+	}
 
 }
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/CProjectChangeMonitor.java b/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/CProjectChangeMonitor.java
@@ -11,44 +11,39 @@
  * Gesa Hentschke (Bachmann electronic GmbH) - initial implementation
  *******************************************************************************/
 
-package org.eclipse.cdt.lsp.clangd;
+package org.eclipse.cdt.lsp.internal.clangd;
 
 import org.eclipse.cdt.core.CCorePlugin;
 import org.eclipse.cdt.core.settings.model.CProjectDescriptionEvent;
 import org.eclipse.cdt.core.settings.model.ICProjectDescriptionListener;
-import org.osgi.service.component.annotations.Component;
-import org.osgi.service.component.annotations.Reference;
+import org.eclipse.cdt.lsp.clangd.ClangdCProjectDescriptionListener;
+import org.eclipse.cdt.lsp.clangd.MacroResolver;
+import org.eclipse.core.runtime.ServiceCaller;
 
 /**
- * This default monitor listens to C project description changes. Can be derived by vendors to add own listeners/behavior.
- * This can be done by using this class as superclass and add the new class as OSGi service with a service.ranking > 0.
+ * This monitor listens to C project description changes.
  */
-@Component(property = { "service.ranking:Integer=0" })
-public class DefaultCProjectChangeMonitor implements CProjectChangeMonitor {
+public class CProjectChangeMonitor {
+	MacroResolver macroResolver = new MacroResolver();
 
-	@Reference
-	MacroResolver macroResolver;
-
-	@Reference
-	private ClangdCProjectDescriptionListener clangdListener;
+	private final ServiceCaller<ClangdCProjectDescriptionListener> clangdListener = new ServiceCaller<>(getClass(),
+			ClangdCProjectDescriptionListener.class);
 
 	private final ICProjectDescriptionListener listener = new ICProjectDescriptionListener() {
 
 		@Override
 		public void handleEvent(CProjectDescriptionEvent event) {
-			clangdListener.handleEvent(event, macroResolver);
+			clangdListener.call(c -> c.handleEvent(event, macroResolver));
 		}
 
 	};
 
-	@Override
 	public CProjectChangeMonitor start() {
 		CCorePlugin.getDefault().getProjectDescriptionManager().addCProjectDescriptionListener(listener,
 				CProjectDescriptionEvent.APPLIED);
 		return this;
 	}
 
-	@Override
 	public void stop() {
 		CCorePlugin.getDefault().getProjectDescriptionManager().removeCProjectDescriptionListener(listener);
 	}
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/DefaultMacroResolver.java b/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/DefaultMacroResolver.java
diff --git a/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/editor/ClangdPlugin.java b/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/editor/ClangdPlugin.java
@@ -14,11 +14,8 @@
 
 package org.eclipse.cdt.lsp.internal.clangd.editor;
 
-import java.util.Optional;
-
-import org.eclipse.cdt.lsp.clangd.CProjectChangeMonitor;
+import org.eclipse.cdt.lsp.internal.clangd.CProjectChangeMonitor;
 import org.eclipse.core.resources.IWorkspace;
-import org.eclipse.ui.PlatformUI;
 import org.eclipse.ui.plugin.AbstractUIPlugin;
 import org.osgi.framework.BundleContext;
 import org.osgi.util.tracker.ServiceTracker;
@@ -29,7 +26,7 @@
 public class ClangdPlugin extends AbstractUIPlugin {
 	private IWorkspace workspace;
 	private CompileCommandsMonitor compileCommandsMonitor;
-	private Optional<CProjectChangeMonitor> cProjectChangeMonitor;
+	private CProjectChangeMonitor cProjectChangeMonitor;
 
 	// The plug-in ID
 	public static final String PLUGIN_ID = "org.eclipse.cdt.lsp.clangd"; //$NON-NLS-1$
@@ -51,15 +48,14 @@ public void start(BundleContext context) throws Exception {
 		workspaceTracker.open();
 		workspace = workspaceTracker.getService();
 		compileCommandsMonitor = new CompileCommandsMonitor(workspace).start();
-		cProjectChangeMonitor = Optional.ofNullable(PlatformUI.getWorkbench().getService(CProjectChangeMonitor.class))
-				.map(m -> m.start());
+		cProjectChangeMonitor = new CProjectChangeMonitor().start();
 	}
 
 	@Override
 	public void stop(BundleContext context) throws Exception {
 		plugin = null;
 		compileCommandsMonitor.stop();
-		cProjectChangeMonitor.ifPresent(m -> m.stop());
+		cProjectChangeMonitor.stop();
 		super.stop(context);
 	}
 
diff --git a/releng/org.eclipse.cdt.lsp.target/org.eclipse.cdt.lsp.target.target b/releng/org.eclipse.cdt.lsp.target/org.eclipse.cdt.lsp.target.target
@@ -45,7 +45,6 @@
 			<!-- We explicitly have CDT in target platform so that developers can develop org.eclipse.cdt.core/ui without requiring all the projects from CDT in their workspace. -->
 			<repository location="https://download.eclipse.org/tools/cdt/releases/11.3/cdt-11.3.0/" />
 			<unit id="org.eclipse.cdt.feature.group" version="0.0.0" />
-			<unit id="org.eclipse.cdt.cmake.core" version="0.0.0"/>
 		</location>
 		<location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit">
 			<repository location="https://download.eclipse.org/wildwebdeveloper/releases/latest/" />
diff --git a/tests/org.eclipse.cdt.lsp.clangd.tests/src/org/eclipse/cdt/lsp/clangd/tests/ClangdConfigurationFileManagerTest.java b/tests/org.eclipse.cdt.lsp.clangd.tests/src/org/eclipse/cdt/lsp/clangd/tests/ClangdConfigurationFileManagerTest.java
@@ -93,7 +93,7 @@ public void cleanUp() throws CoreException {
 	}
 
 	private static File createFile(File parent, String format, String cdbDirectoryPath) throws FileNotFoundException {
-		return createFile(parent, ClangdCProjectDescriptionListener.CLANGD_CONFIG_FILE_NAME, format, cdbDirectoryPath);
+		return createFile(parent, ClangdConfigurationFileManager.CLANGD_CONFIG_FILE_NAME, format, cdbDirectoryPath);
 	}
 
 	private static File createFile(File parent, String fileName, String format, String cdbDirectoryPath)
@@ -116,7 +116,7 @@ private static File createFile(File parent, String fileName, String format, Stri
 	 */
 	private IFile createConfigFile(String format, String cdbDirectoryPath)
 			throws UnsupportedEncodingException, IOException, CoreException {
-		var file = project.getFile(ClangdCProjectDescriptionListener.CLANGD_CONFIG_FILE_NAME);
+		var file = project.getFile(ClangdConfigurationFileManager.CLANGD_CONFIG_FILE_NAME);
 		try (final var data = new ByteArrayInputStream(
 				String.format(format, cdbDirectoryPath).getBytes(project.getDefaultCharset()))) {
 			if (!file.exists()) {
@@ -138,7 +138,7 @@ private IFile createConfigFile(String format, String cdbDirectoryPath)
 	@Test
 	void testCreateClangdConfigFileInProject() throws IOException, CoreException {
 		var projectDir = project.getLocation().toPortableString();
-		var configFile = new File(projectDir, ClangdCProjectDescriptionListener.CLANGD_CONFIG_FILE_NAME);
+		var configFile = new File(projectDir, ClangdConfigurationFileManager.CLANGD_CONFIG_FILE_NAME);
 		var refFile = createFile(TEMP_DIR, DEFAULT_CDB_SETTING, RELATIVE_DIR_PATH_BUILD_DEFAULT);
 		// The current working directory of the builder in the project is set to RELATIVE_DIR_PATH_BUILD_DEFAULT:
 		cwdBuilder = new Path(project.getLocation().append(RELATIVE_DIR_PATH_BUILD_DEFAULT).toPortableString());
@@ -191,7 +191,7 @@ void testEmptyClangdConfigFileInProject() throws IOException, CoreException {
 	@Test
 	void testUpdateClangdConfigFileInProject() throws IOException, CoreException {
 		var projectDir = project.getLocation().toPortableString();
-		var configFile = new File(projectDir, ClangdCProjectDescriptionListener.CLANGD_CONFIG_FILE_NAME);
+		var configFile = new File(projectDir, ClangdConfigurationFileManager.CLANGD_CONFIG_FILE_NAME);
 		var refFileDefault = createFile(TEMP_DIR, ".clangdDefault", DEFAULT_CDB_SETTING,
 				RELATIVE_DIR_PATH_BUILD_DEFAULT);
 		// Use MODIFIED_DEFAULT_CDB_SETTING here, because the org.yaml.snakeyaml.Yaml.dump appends a '\n' at the last line:
diff --git a/tests/org.eclipse.cdt.lsp.clangd.tests/src/org/eclipse/cdt/lsp/clangd/tests/MockMacroResolver.java b/tests/org.eclipse.cdt.lsp.clangd.tests/src/org/eclipse/cdt/lsp/clangd/tests/MockMacroResolver.java
@@ -17,7 +17,7 @@
 import org.eclipse.cdt.core.settings.model.ICConfigurationDescription;
 import org.eclipse.cdt.lsp.clangd.MacroResolver;
 
-public class MockMacroResolver implements MacroResolver {
+public class MockMacroResolver extends MacroResolver {
 
 	@Override
 	public String resolveValue(String value, String nonexistentMacrosValue, String listDelimiter,
diff --git a/tests/org.eclipse.cdt.lsp.clangd.tests/src/org/eclipse/cdt/lsp/clangd/tests/TestUtils.java b/tests/org.eclipse.cdt.lsp.clangd.tests/src/org/eclipse/cdt/lsp/clangd/tests/TestUtils.java
@@ -61,24 +61,6 @@ public static IProject createCProject(String projectName) throws CoreException {
 		return project;
 	}
 
-	//	static IProject createEmptyCMakeProject(String projectName) {
-	//		CMakeProjectGenerator generator = new CMakeProjectGenerator("templates/simple/manifest.xml");//"templates/simple/manifest.xml"); //$NON-NLS-1$
-	//		generator.setProjectName(projectName);
-	//		var uri = ResourcesPlugin.getWorkspace().getRoot().getLocation().append(projectName).toPath().toUri();
-	//		generator.setLocationURI(uri);
-	//		try {
-	//			generator.generate(new HashMap<>(), null);
-	//		} catch (CoreException e) {
-	//			e.printStackTrace();
-	//			return null;
-	//		}
-	//		IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(projectName);
-	//		if (project.exists()) {
-	//			return project;
-	//		}
-	//		return null;
-	//	}
-
 	public static void deleteProject(IProject project) throws CoreException {
 		if (project != null) {
 			project.delete(true, new NullProgressMonitor());
