Add UI for specifying CMake tools and generators' locations #1067

DangMinhTam382 · 2025-01-29T18:11:22Z

Add UI for specifying CMake tools and generators locations

Adding UI into CMake Preference page that allow user to specify the
location of the CMake tool and the location of CMake generators.
Added CMakeBuildEnvironmentSupplier.class for ICBuildConfiguration

Addresses Issue: CDT CMake Improvements #1000, IDE-82683-REQ-004 and
IDE-82683-REQ-005

Adding UI into CMake Preference page that allow user to specify the location of the CMake tool and the location of CMake generators. Addresses Issue: CDT CMake Improvements eclipse-cdt#1000, IDE-82683-REQ-004 and IDE-82683-REQ-005

github-actions · 2025-01-29T20:44:36Z

Test Results

600 files - 34 600 suites - 34 13m 40s ⏱️ - 22m 30s
10 206 tests - 1 217 10 183 ✅ - 1 097 23 💤 - 120 0 ❌ ±0
10 244 runs - 1 194 10 221 ✅ - 1 076 23 💤 - 118 0 ❌ ±0

Results for commit 18ad0ef. ± Comparison against base commit 889a5f1.

This pull request removes 1217 tests.

org.eclipse.cdt.debug.gdbjtag.core.tests.jtagdevice.GDBJtagDeviceContributionTest ‑ testGdbJtagDeviceContribution
org.eclipse.cdt.debug.gdbjtag.core.tests.launch.GDBJtagLaunchTest ‑ testGdbJtagLaunch[gdb]
org.eclipse.cdt.debug.gdbjtag.core.tests.launch.GDBJtagLaunchTest ‑ testGdbJtagLaunch[gdbserver]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithQuotes[gdb]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithQuotes[gdbserver]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithSpecialSymbols[gdb]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithSpecialSymbols[gdbserver]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithSymbols[gdb]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithSymbols[gdbserver]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithTabs[gdb]
…

betamaxbandit · 2025-01-29T22:24:50Z

Thankyou @DangMinhTam382 , I will review tomorrow (Th).

betamaxbandit · 2025-01-30T20:37:41Z

Hi @DangMinhTam382 , I won't be able to review this until next week now.
Cheers John

jonahgraham

Thanks for this change. Please have a look at my comments and don't hesitate to reach out to me or @betamaxbandit if you need any clarification.

jonahgraham · 2025-01-29T20:30:59Z

cmake/org.eclipse.cdt.cmake.ui/src/org/eclipse/cdt/cmake/ui/internal/CMakePreferencePage.java

+		variablesButton = new Button(cmakeLocationButtonComp, SWT.PUSH);
+		variablesButton.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false));
+		variablesButton.setText(Messages.CMakePreferencePage_Variables);
+		variablesButton.setData(cmakeLocationTextBox);
+		variablesButton.addListener(SWT.Selection, e -> {
+			String x = null;
+			if (variablesButton.getData() instanceof Text t) {
+				x = getVariableDialog(getShell(), null);
+				if (x != null) {
+					t.insert(x);
+				}
+			}
+		});


The only getData on this button is in the listener - by doing it this way we need instanceof checks and we lose the natural connection. e.g. doing find references on cmakeLocationTextBox won't show the t on line 232. Also, if you have one access to the field (variablesButton) you may as well directly access the field you really wanted.

Here is a suggestion for this block of code, it does this following:

simplifies the code and improves readability by using cmakeLocationTextBox in the listener

removes need for instanceof check

removes unneeded extra assignment to x

renames x to a more descriptive name

Suggested change

variablesButton = new Button(cmakeLocationButtonComp, SWT.PUSH);

variablesButton.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false));

variablesButton.setText(Messages.CMakePreferencePage_Variables);

variablesButton.setData(cmakeLocationTextBox);

variablesButton.addListener(SWT.Selection, e -> {

String x = null;

if (variablesButton.getData() instanceof Text t) {

x = getVariableDialog(getShell(), null);

if (x != null) {

t.insert(x);

}

}

});

variablesButton = new Button(cmakeLocationButtonComp, SWT.PUSH);

variablesButton.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false));

variablesButton.setText(Messages.CMakePreferencePage_Variables);

variablesButton.addListener(SWT.Selection, e -> {

String variable = getVariableDialog(getShell(), null);

if (variable != null) {

cmakeLocationTextBox.insert(variable);

}

});

You can apply a similar change to the browseButton

The use of set/get data is suitable when there is some reuse of code. For example, if you used the same listener for a bunch of different Variables... buttons and wanted each one to work on its associated text you could do something like this:

/** * Opens the variable dialog and applies the result to the text box associated with the * widget that issued the selection event. */ Listener applyVariableToTextbox = e -> { if (e.widget.getData() instanceof Text t) { String variable = getVariableDialog(getShell(), null); if (variable != null) { t.insert(variable); } } }; cmakeVariablesButton = new Button(composite, SWT.PUSH); cmakeVariablesButton.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false)); cmakeVariablesButton.setText("Variables..."); cmakeVariablesButton.setData(cmakeLocationTextBox); cmakeVariablesButton.addListener(SWT.Selection, applyVariableToTextbox); anotherVariablesButton = new Button(composite, SWT.PUSH); anotherVariablesButton.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false)); anotherVariablesButton.setText("Variables..."); anotherVariablesButton.setData(anotherTextBox); anotherVariablesButton.addListener(SWT.Selection, applyVariableToTextbox);

jonahgraham · 2025-01-29T20:53:53Z

cmake/org.eclipse.cdt.cmake.ui/src/org/eclipse/cdt/cmake/ui/internal/CMakePreferencePage.java

+	private static final String ENABLE_USE_CMAKE_LOCATION = "enable_use_cmake_location"; //$NON-NLS-1$
+	private static final String CMAKE_LOCATION = "cmake_location"; //$NON-NLS-1$
+	private static final String CMAKE_GENERATOR_LOCATION = "cmake_generator_locations"; //$NON-NLS-1$
+	private static final String VALUE_DELIMITER = " || "; //$NON-NLS-1$


This comment is some commentary on the design to explain why I have requested changes about serializing and deserializing.

|| was chosen as a value delimiter based on its prior art in CPropertyVarsTab, specifically from

cdt/build/org.eclipse.cdt.managedbuilder.ui/src/org/eclipse/cdt/managedbuilder/ui/properties/CPropertyVarsTab.java

Line 90 in c252113

private static final String VALUE_DELIMITER = " || "; //$NON-NLS-1$

However in CPropertyVarsTab this was used strictly as a display convenience when displaying values in the GUI, it is not prior art for what you are doing here because you are using the || to serialize the values.

For example, the above setting in the screenshot is stored in .metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.cdt.core.prefs as string escaped XML file:

macros/workspace=<?xml version\="1.0" encoding\="UTF-8" standalone\="no"?>\n<macros>\n <stringListMacro name\="jonah" type\="VALUE_TEXT_LIST">\n <value name\="val1"/>\n <value name\="val2"/>\n </stringListMacro>\n</macros>\n

The code that converts the list/array to the serialized form is

cdt/core/org.eclipse.cdt.core/src/org/eclipse/cdt/internal/core/cdtvariables/StorableCdtVariable.java

Lines 149 to 153 in caf2292

for (int i = 0; i < fStringListValue.length; i++) {

ICStorageElement valEl = element.createChild(VALUE_ELEMENT_NAME);

if (fStringListValue[i] != null)

valEl.setAttribute(VALUE_ELEMENT_VALUE, fStringListValue[i]);

}

That ties quite tightly with CDT as it is part of the org.eclipse.cdt.core.settings.model.

jonahgraham · 2025-01-30T03:09:04Z

...eclipse.cdt.core/src/org/eclipse/cdt/internal/core/envvar/CMakeBuildEnvironmentSupplier.java

+	private static final String NODENAME = "cmake_environment"; //$NON-NLS-1$
+	private static final String ENABLE_USE_CMAKE_LOCATION = "enable_use_cmake_location"; //$NON-NLS-1$
+	private static final String CMAKE_LOCATION = "cmake_location"; //$NON-NLS-1$
+	private static final String CMAKE_GENERATOR_LOCATION = "cmake_generator_locations"; //$NON-NLS-1$


These should be public and shared with the preference page with the duplicates removed from the preference page.

jonahgraham · 2025-01-30T03:14:21Z

...eclipse.cdt.core/src/org/eclipse/cdt/internal/core/envvar/CMakeBuildEnvironmentSupplier.java

+	private boolean useCmakeToolLocation;
+	private String cmakeLocation;
+	private String[] generatorLocations;


Remove these fields and instead inline updateSupplier in getVariable() only and remove all the other calls to updateSupplier. The only place these are read is in getVariable() so all the other calls to updateSupplier are effective no-ops.

jonahgraham · 2025-01-30T03:44:56Z

...rg.eclipse.cdt.core/src/org/eclipse/cdt/internal/core/envvar/EnvironmentVariableManager.java

+		if (level instanceof ICBuildConfiguration || level instanceof IBuildConfiguration buildConfiguration
+				&& buildConfiguration.getAdapter(ICBuildConfiguration.class) != null) {


You can perform all this in one step with Adapters.adapt

Suggested change

if (level instanceof ICBuildConfiguration || level instanceof IBuildConfiguration buildConfiguration

&& buildConfiguration.getAdapter(ICBuildConfiguration.class) != null) {

if (Adapters.adapt(level, ICBuildConfiguration.class) != null) {

jonahgraham · 2025-01-30T03:48:49Z

...rg.eclipse.cdt.core/src/org/eclipse/cdt/internal/core/envvar/EnvironmentVariableManager.java

@@ -225,6 +227,10 @@ IEnvironmentContextInfo getDefaultContextInfo(Object level) {
 	 * or null if the the given level is not supported
 	 */
 	public IEnvironmentContextInfo getContextInfo(Object level) {
+		if (level instanceof ICBuildConfiguration || level instanceof IBuildConfiguration buildConfiguration
+				&& buildConfiguration.getAdapter(ICBuildConfiguration.class) != null) {
+			return new CMakeEnvironmentContextInfo(getDefaultContextInfo(level));


Can you explain what this code does a bit please as I don't understand full effect of this, but I do find it unexpected that we do getAdapter for ICBuildConfiguration, but then don't pass the ICBuildConfiguration to getDefaultContextInfo.

jonahgraham · 2025-02-03T14:50:43Z

cmake/org.eclipse.cdt.cmake.ui/src/org/eclipse/cdt/cmake/ui/internal/CMakePreferencePage.java

+		// Update Preferences for cmakeSupplier
+		getPreferences().putBoolean(ENABLE_USE_CMAKE_LOCATION, useCmakeToolLocation);
+		getPreferences().put(CMAKE_LOCATION, cmakeLocation);
+		getPreferences().put(CMAKE_GENERATOR_LOCATION, String.join(VALUE_DELIMITER, generatorLocations));


Please serialize the list using XML or propose another serialization format that has no ambiguity.

For example, to reuse the CDT infrastructure to do this see org.eclipse.cdt.internal.core.cdtvariables.UserDefinedVariableSupplier.storeMacrosToStream(StorableCdtVariables)

Note that the CDT Variables GUI + backend is pretty complicated and it can be hard to see how the variable serialize connects to the preference tab. Here is a call hierarchy that shows how they are connected:

StorableCdtVariables.serialize(ICStorageElement)

UserDefinedVariableSupplier.storeMacrosToStream(StorableCdtVariables)

UserDefinedVariableSupplier.storeWorkspaceMacros(StorableCdtVariables, boolean)

UserDefinedVariableSupplier.storeWorkspaceVariables(boolean)

UserVarSupplier.storeWorkspaceVariables(boolean)

CPropertyVarsTab.performApply(ICResourceDescription, ICResourceDescription) line 648

jonahgraham · 2025-02-03T14:52:08Z

...eclipse.cdt.core/src/org/eclipse/cdt/internal/core/envvar/CMakeBuildEnvironmentSupplier.java

+		useCmakeToolLocation = getPreferences().getBoolean(ENABLE_USE_CMAKE_LOCATION, false);
+		cmakeLocation = getPreferences().get(CMAKE_LOCATION, ""); //$NON-NLS-1$
+		String genLocations = getPreferences().get(CMAKE_GENERATOR_LOCATION, ""); //$NON-NLS-1$
+		generatorLocations = genLocations.length() > 0 ? genLocations.split(" \\|\\| ") : new String[0]; //$NON-NLS-1$


Please see comment about serializing and then update the deserializing to match

DangMinhTam382 added 6 commits January 26, 2025 00:38

Merge branch 'eclipse-cdt:main' into IDE_82683_REQ_004_005

1952eed

Merge branch 'eclipse-cdt:main' into IDE_82683_REQ_004_005

ba20266

jonahgraham added this to the 12.0.0 M3 milestone Feb 2, 2025

jonahgraham requested changes Feb 3, 2025

View reviewed changes

Merge branch 'eclipse-cdt:main' into IDE_82683_REQ_004_005

b3f626a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI for specifying CMake tools and generators' locations #1067

Add UI for specifying CMake tools and generators' locations #1067

DangMinhTam382 commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

betamaxbandit commented Jan 29, 2025

betamaxbandit commented Jan 30, 2025

jonahgraham left a comment

jonahgraham Jan 29, 2025

jonahgraham Jan 29, 2025

jonahgraham Jan 30, 2025

jonahgraham Jan 30, 2025

jonahgraham Jan 30, 2025

jonahgraham Jan 30, 2025

jonahgraham Feb 3, 2025

jonahgraham Feb 3, 2025

	for (int i = 0; i < fStringListValue.length; i++) {
	ICStorageElement valEl = element.createChild(VALUE_ELEMENT_NAME);
	if (fStringListValue[i] != null)
	valEl.setAttribute(VALUE_ELEMENT_VALUE, fStringListValue[i]);
	}

		if (level instanceof ICBuildConfiguration \|\| level instanceof IBuildConfiguration buildConfiguration
		&& buildConfiguration.getAdapter(ICBuildConfiguration.class) != null) {

	if (level instanceof ICBuildConfiguration \|\| level instanceof IBuildConfiguration buildConfiguration
	&& buildConfiguration.getAdapter(ICBuildConfiguration.class) != null) {
	if (Adapters.adapt(level, ICBuildConfiguration.class) != null) {

Add UI for specifying CMake tools and generators' locations #1067

Are you sure you want to change the base?

Add UI for specifying CMake tools and generators' locations #1067

Conversation

DangMinhTam382 commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

Test Results

betamaxbandit commented Jan 29, 2025

betamaxbandit commented Jan 30, 2025

jonahgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment