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

IEP-1247 Run CMake initial build for newly created project(idf.py reconfigure) #1051

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
3 changes: 2 additions & 1 deletion bundles/com.espressif.idf.core/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,5 @@ Bundle-ClassPath: .,
lib/commons-compress-1.21.jar,
lib/xz-1.9.jar
Import-Package: org.eclipse.embedcdt.core,
org.eclipse.launchbar.ui.target
org.eclipse.launchbar.ui.target,
org.eclipse.ui.console
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,13 @@ public Process run(List<String> commands, IPath workingDirectory, Map<String, St
if (environment != null && !environment.isEmpty())
{
processBuilder.environment().putAll(environment);
// Removing Path, because we are using PATH
if (processBuilder.environment().containsKey("PATH") && processBuilder.environment().containsKey("Path")) //$NON-NLS-1$
{
processBuilder.environment().remove("Path"); //$NON-NLS-1$
}
}
// let's merge the error stream with the standard output

processBuilder.redirectErrorStream(true);
return processBuilder.start();
}
Expand All @@ -46,8 +51,7 @@ public IStatus runInBackground(List<String> commands, IPath workingDirectory, Ma
throws IOException
{
Process process = run(commands, workingDirectory, environment);
return processData(process.getInputStream(), process.getErrorStream(), process.getOutputStream(),
process);
return processData(process.getInputStream(), process.getErrorStream(), process.getOutputStream(), process);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*******************************************************************************
* Copyright 2024 Espressif Systems (Shanghai) PTE LTD. All rights reserved.
* Use is subject to license terms.
*******************************************************************************/

package com.espressif.idf.core.util;

import org.eclipse.ui.console.ConsolePlugin;
import org.eclipse.ui.console.IConsole;
import org.eclipse.ui.console.MessageConsole;

public class ConsoleManager
{

private ConsoleManager()
{
}

public static MessageConsole getConsole(String consoleName)
{
MessageConsole console = findConsole(consoleName);
if (console == null)
{
console = new MessageConsole(consoleName, null);
ConsolePlugin.getDefault().getConsoleManager().addConsoles(new IConsole[] { console });
}
ConsolePlugin.getDefault().getConsoleManager().showConsoleView(console);
return console;
}
Comment on lines +19 to +29
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add documentation and parameter validation.

The method logic is correct, but could benefit from these improvements:

  1. Add Javadoc with parameter and return descriptions
  2. Add null check for consoleName parameter
+/**
+ * Gets or creates a MessageConsole with the specified name.
+ * If a console with the given name already exists, it will be returned.
+ * Otherwise, a new console will be created and added to the console manager.
+ *
+ * @param consoleName the name of the console to get or create
+ * @return the MessageConsole instance
+ * @throws IllegalArgumentException if consoleName is null or empty
+ */
 public static MessageConsole getConsole(String consoleName)
 {
+    if (consoleName == null || consoleName.trim().isEmpty()) {
+        throw new IllegalArgumentException("Console name cannot be null or empty");
+    }
     MessageConsole console = findConsole(consoleName);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public static MessageConsole getConsole(String consoleName)
{
MessageConsole console = findConsole(consoleName);
if (console == null)
{
console = new MessageConsole(consoleName, null);
ConsolePlugin.getDefault().getConsoleManager().addConsoles(new IConsole[] { console });
}
ConsolePlugin.getDefault().getConsoleManager().showConsoleView(console);
return console;
}
/**
* Gets or creates a MessageConsole with the specified name.
* If a console with the given name already exists, it will be returned.
* Otherwise, a new console will be created and added to the console manager.
*
* @param consoleName the name of the console to get or create
* @return the MessageConsole instance
* @throws IllegalArgumentException if consoleName is null or empty
*/
public static MessageConsole getConsole(String consoleName)
{
if (consoleName == null || consoleName.trim().isEmpty()) {
throw new IllegalArgumentException("Console name cannot be null or empty");
}
MessageConsole console = findConsole(consoleName);
if (console == null)
{
console = new MessageConsole(consoleName, null);
ConsolePlugin.getDefault().getConsoleManager().addConsoles(new IConsole[] { console });
}
ConsolePlugin.getDefault().getConsoleManager().showConsoleView(console);
return console;
}


private static MessageConsole findConsole(String consoleName)
{
for (IConsole existing : ConsolePlugin.getDefault().getConsoleManager().getConsoles())
{
if (consoleName.equals(existing.getName()))
{
return (MessageConsole) existing;
}
}
return null;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont understand the need for this class, maybe I am not clear on this. Just a question from my side that why do we need a separate class to just execute this command when we can do that in the Job we are creating for the console?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @alirana01,

thanks for the review. I've created this class not only for this command but also for other idf commands that we have to execute in the console. My idea was that in the future we could refactor existing code and move other IDF commands and their preparation into this class to not scatter similar logic over the code base and to avoid duplicate code

Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*******************************************************************************
* Copyright 2021 Espressif Systems (Shanghai) PTE LTD. All rights reserved.
* Use is subject to license terms.
*******************************************************************************/

package com.espressif.idf.core.util;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import org.eclipse.core.resources.IProject;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.Status;
import org.eclipse.ui.console.MessageConsole;
import org.eclipse.ui.console.MessageConsoleStream;

import com.espressif.idf.core.IDFCorePlugin;
import com.espressif.idf.core.IDFEnvironmentVariables;
import com.espressif.idf.core.ProcessBuilderFactory;
import com.espressif.idf.core.logging.Logger;

public class IdfCommandExecutor
{

private final String target;
private final MessageConsole console;

public IdfCommandExecutor(String target, MessageConsole console)
{
this.target = target;
this.console = console;
}

public IStatus executeReconfigure(IProject project)
{
console.activate();
return runIdfReconfigureCommand(project);
}

private IStatus runIdfReconfigureCommand(IProject project)
{
ProcessBuilderFactory processRunner = new ProcessBuilderFactory();
List<String> arguments = prepareArguments();
Map<String, String> environment = new HashMap<>(new IDFEnvironmentVariables().getSystemEnvMap());

try (MessageConsoleStream messageConsoleStream = console.newMessageStream())
{
return runProcess(arguments, environment, processRunner, project, messageConsoleStream);
}
catch (IOException e1)
{
Logger.log(e1);
return IDFCorePlugin.errorStatus(e1.getMessage(), e1);
}
}

private List<String> prepareArguments()
{
List<String> arguments = new ArrayList<>();
arguments.add(IDFUtil.getIDFPythonEnvPath());
arguments.add(IDFUtil.getIDFPythonScriptFile().getAbsolutePath());
arguments.add("-DIDF_TARGET=" + target); //$NON-NLS-1$
arguments.add("reconfigure"); //$NON-NLS-1$
return arguments;
}

private IStatus runProcess(List<String> arguments, Map<String, String> environment,
ProcessBuilderFactory processRunner, IProject project, MessageConsoleStream messageConsoleStream)
{
StringBuilder output = new StringBuilder();
try (BufferedReader reader = new BufferedReader(new InputStreamReader(
processRunner.run(arguments, project.getLocation(), environment).getInputStream())))
{
String line;
while ((line = reader.readLine()) != null)
{
output.append(line).append(System.lineSeparator());
messageConsoleStream.println(line);
}
return new Status(IStatus.OK, IDFCorePlugin.PLUGIN_ID, output.toString());
}
catch (Exception e)
{
Logger.log(e);
return IDFCorePlugin.errorStatus(e.getMessage(), e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,10 @@ public ITerminalConnector createTerminalConnector(Map<String, Object> properties
}
envpList.add(envKey + "=" + envValue); //$NON-NLS-1$
}

//Removing path, since we are using PATH
if (envMap.containsKey("PATH") && envMap.containsKey("Path")) { //$NON-NLS-1$ //$NON-NLS-2$
envMap.remove("Path"); //$NON-NLS-1$
}
// Convert back into a string array
envp = envpList.toArray(new String[envpList.size()]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ public class Messages extends NLS
public static String TemplateGroupHeader;
public static String NewProjectTargetSelection_Tooltip;
public static String NewProjectTargetSelection_Label;

public static String RunIdfCommandButtonTxt;

static
{
// initialize resource bundle
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.eclipse.swt.events.ModifyListener;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Combo;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Label;
Expand All @@ -47,6 +48,7 @@ public class NewProjectCreationWizardPage extends AbstractTemplatesSelectionPage
private Combo targetCombo;
// initial value stores
private String initialProjectFieldValue;
private Button runIdfReconfigureCheckBoxButton;

/**
* Constructor
Expand Down Expand Up @@ -84,26 +86,25 @@ private void createProjectTargetSelection(Composite container)
targetCombo.select(0);
targetCombo.setToolTipText(Messages.NewProjectTargetSelection_Tooltip);
}

private void createProjectNameGroup(Composite container)
{
Composite mainComposite = new Composite(container, SWT.NONE);
mainComposite.setLayout(new GridLayout());
GridData gridData = new GridData(GridData.FILL_BOTH);
gridData.horizontalSpan = 20;
gridData.verticalSpan = 5;

mainComposite.setLayoutData(gridData);

Composite projectNameGroup = new Composite(mainComposite, SWT.NONE);
GridLayout layout = new GridLayout();
layout.numColumns = 2;
projectNameGroup.setLayout(layout);
projectNameGroup.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false));
Label projectNameLabel = new Label(projectNameGroup, SWT.NONE);
projectNameLabel.setText(IDEWorkbenchMessages.WizardNewProjectCreationPage_nameLabel);



projectNameField = new Text(projectNameGroup, SWT.BORDER);
projectNameField.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true));
projectNameField.addModifyListener(new ModifyListener()
Expand All @@ -119,20 +120,26 @@ public void modifyText(ModifyEvent e)
{
setPageComplete(false);
}

boolean valid = validatePage();
setPageComplete(valid);
}
});



locationArea = new ProjectContentsLocationArea(getErrorReporter(), mainComposite);
if (initialProjectFieldValue != null)
{
locationArea.updateProjectName(initialProjectFieldValue);
}

runIdfReconfigureCheckBoxButton = new Button(projectNameGroup, SWT.CHECK | SWT.RIGHT);
GridData buttonData = new GridData();
buttonData.horizontalSpan = 4;
runIdfReconfigureCheckBoxButton.setLayoutData(buttonData);
runIdfReconfigureCheckBoxButton.setSelection(true);
runIdfReconfigureCheckBoxButton.setText(Messages.RunIdfCommandButtonTxt);
}

/**
* Get an error reporter for the receiver.
*
Expand All @@ -157,7 +164,7 @@ private IErrorMessageReporter getErrorReporter()
setPageComplete(valid);
};
}

/**
* Returns the useDefaults.
*
Expand All @@ -167,17 +174,22 @@ public boolean useDefaults()
{
return locationArea.isDefault();
}


public boolean isRunIdfReconfigureEnabled()
{
return runIdfReconfigureCheckBoxButton.getSelection();
}
Comment on lines +178 to +181
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add null check for robustness

The getter method should handle cases where the button hasn't been initialized yet.

 public boolean isRunIdfReconfigureEnabled()
 {
-    return runIdfReconfigureCheckBoxButton.getSelection();
+    return runIdfReconfigureCheckBoxButton != null && runIdfReconfigureCheckBoxButton.getSelection();
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public boolean isRunIdfReconfigureEnabled()
{
return runIdfReconfigureCheckBoxButton.getSelection();
}
public boolean isRunIdfReconfigureEnabled()
{
return runIdfReconfigureCheckBoxButton != null && runIdfReconfigureCheckBoxButton.getSelection();
}


public IPath getLocationPath()
{
return new Path(locationArea.getProjectLocation());
}

public URI getLocationURI()
{
return locationArea.getProjectLocationURI();
}

/**
* Sets the initial project name that this page will use when created. The name is ignored if the
* createControl(Composite) method has already been called. Leading and trailing spaces in the name are ignored.
Expand All @@ -204,7 +216,7 @@ public void setInitialProjectName(String name)
}
}
}

/**
* Set the location to the default location if we are set to useDefaults.
*/
Expand All @@ -213,7 +225,6 @@ void setLocationForSelection()
locationArea.updateProjectName(getProjectNameFieldValue());
}


/**
* Returns the value of the project name field with leading and trailing spaces removed.
*
Expand All @@ -228,7 +239,7 @@ private String getProjectNameFieldValue()

return projectNameField.getText().trim();
}

@Override
protected boolean validatePage()
{
Expand All @@ -242,7 +253,8 @@ protected boolean validatePage()

if (!IDFUtil.checkIfIdfSupportsSpaces() && worspaceLocation.contains(" ")) //$NON-NLS-1$
{
setErrorMessage(com.espressif.idf.ui.wizard.Messages.WizardNewProjectCreationPage_WorkspaceLocCantIncludeSpaceErr);
setErrorMessage(
com.espressif.idf.ui.wizard.Messages.WizardNewProjectCreationPage_WorkspaceLocCantIncludeSpaceErr);
return false;
}

Expand Down Expand Up @@ -303,7 +315,7 @@ public IProject getProjectHandle()
{
return ResourcesPlugin.getWorkspace().getRoot().getProject(getProjectName());
}

/**
* Returns the current project name as entered by the user, or its anticipated initial value.
*
Expand All @@ -318,7 +330,7 @@ public String getProjectName()

return getProjectNameFieldValue();
}

@Override
protected void initializeViewer()
{
Expand Down Expand Up @@ -401,7 +413,7 @@ public void setInitialTemplateId(ITemplateNode templateNode)
@Override
public void setVisible(boolean visible)
{
if (visible && getfUseTemplate() != null )
if (visible && getfUseTemplate() != null)
{
if (getfUseTemplate().getSelection() == false)
templateViewer.getControl().setEnabled(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ NewProjectWizardPage_NoTemplateFoundMessage=No Templates were found on the syste
TemplateGroupHeader=Template Selection
NewProjectTargetSelection_Tooltip=Select a project target from the list. This setting is not final and can be changed later in the Launchbar target configuration, where you'll also configure the serial port.
NewProjectTargetSelection_Label=Select Project Target:
RunIdfCommandButtonTxt=Execute idf.py reconfigure with Project Creation
Loading
Loading