IEP-990 Integration of esp-idf BoM #832
Conversation
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Files ignored due to filter (1)
- bundles/com.espressif.idf.ui/plugin.xml
Files selected for processing (5)
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/SbomCommandHandler.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1 hunks)
Files skipped from review due to trivial changes (2)
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java
Additional comments (Suppressed): 1
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/SbomCommandHandler.java (1)
- 1-23: The
SbomCommandHandlerclass is well implemented. It correctly extendsAbstractHandlerand overrides theexecutemethod. Theexecutemethod creates and opens a newSbomCommandDialogusing the active shell from the workbench. The code is simple, clear, and follows the best practices for creating command handlers in Eclipse.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1 hunks)
Additional comments (Suppressed): 16
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1)
- 46-57: The new strings added for the SBOM tool seem to be well defined and cover all the necessary user interactions. The error messages are clear and informative. The labels and button text are concise and self-explanatory.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (15)
1-4: The package and import statements are well organized and relevant to the code. No unused imports detected.
56-57: The class
SbomCommandDialogextendsTitleAreaDialogwhich is a standard way to create dialogs in Eclipse. Good use of inheritance.71-74: The constructor is correctly defined and calls the superclass constructor as expected.
76-84: The
createmethod is overridden to set the title and message for the dialog. It also callssetDefaultsto initialize the dialog with default values. Good use of method overriding.93-169: The
createDialogAreamethod is overridden to create the dialog's UI components. The layout and UI components are correctly set up. The listeners are correctly added to the UI components.171-193: The
runCommandmethod is used to run a command in the background. It usesProcessBuilderFactoryto run the command and logs the output or error. Good use of logging and error handling.195-217: The
okPressedmethod is overridden to execute when the OK button is pressed. It creates a new job to run the SBOM command in the background. The job is correctly scheduled and the dialog values are correctly saved.219-223: The
isResizablemethod is overridden to allow the dialog to be resizable. This is a good practice for user experience.225-250: The
setDefaultsmethod is used to set the default values for the dialog. It correctly gets the selected project and sets the default paths for the project description and output file.252-256: The
buildProjectDescriptionPathmethod is used to build the path for the project description file. It correctly concatenates the project path with the build directory and project description file name.258-351: The
runEspIdfSbomCommandmethod is used to run the SBOM command. It correctly sets up the command arguments and environment, runs the command, and logs the output. It also adds a hyperlink to the output file in the console.353-373: The
validateInputmethod is used to validate the input fields in the dialog. It correctly checks if the project description file exists and if the output file is writable.375-378: The
checkIfFileIsWritablemethod is used to check if a file is writable. It correctly checks if the file exists and if it is writable.380-393: The
installEspIdfSbommethod is used to install the SBOM tool. It correctly sets up the command arguments and environment, runs the command, and logs the output.395-406: The
getIfEspIfSbomInstalledStatusmethod is used to check if the SBOM tool is installed. It correctly sets up the command arguments and environment, runs the command, and checks the output for the SBOM tool name.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1 hunks)
Additional comments (Suppressed): 1
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1)
- 46-57: The new messages added are related to the Software Bill of Materials (SBOM) tool. They include labels for UI elements, error messages, and information messages. Ensure that these messages are correctly displayed in the appropriate contexts in the UI.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1 hunks)
Additional comments (Suppressed): 1
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1)
- 1-407: The
SbomCommandDialogclass is well-structured and follows the Eclipse SWT and JFace conventions for creating and managing dialog boxes. However, there are a few areas that could be improved for better maintainability and readability.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1 hunks)
Additional comments (Suppressed): 16
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (16)
1-4: The package and import statements are well organized and relevant to the code. No unused imports detected.
57-58: The class
SbomCommandDialogextendsTitleAreaDialogwhich is a standard way to create dialogs in Eclipse. Good use of inheritance.72-75: The constructor is correctly defined and calls the superclass constructor as expected.
77-86: The
create()method is correctly overridden and sets up the dialog shell, title, and message. It also callssetDefaults()to initialize the dialog with default values.88-164: The
createDialogArea()method is correctly overridden to create the dialog UI. It creates a composite container with a grid layout and adds various UI components (labels, text fields, buttons) to it. It also sets up listeners for the buttons and text fields. The layout and UI components are correctly configured.166-188: The
runCommand()method is used to run a command in the background usingProcessBuilderFactory. It handles the case where the status returned byrunInBackground()is null, logging an error message in this case. It also logs any IOExceptions that occur.190-212: The
okPressed()method is correctly overridden to handle the OK button press event. It creates a new job to run theesp-idf-sbomcommand in the background, schedules the job, and saves the user input from the dialog. It then calls the superclassokPressed()method.214-218: The
isResizable()method is correctly overridden to allow the dialog to be resizable.220-245: The
setDefaults()method sets the default values for the dialog. It gets the currently selected project from the workbench and sets the default paths for the project description file and output file. It also disables the OK button if the project description file does not exist.247-251: The
buildProjectDescriptionPath()method correctly builds the path to the project description file.253-275: The
runEspIdfSbomCommand()method runs theesp-idf-sbomcommand with the appropriate arguments and environment. It also handles the case where the command output is redirected to a file.277-350: The
getPatternMatchListener()method returns aIPatternMatchListenerthat adds a hyperlink to the console output when a match for the output file path is found. The hyperlink opens the output file in an editor when clicked.353-374: The
validateInput()method validates the user input from the dialog. It checks if the project description file exists and if the output file is writable (if the output is being saved to a file). It sets an error message and disables the OK button if the input is not valid.376-379: The
checkIfFileIsNotWritable()method checks if a file exists and is not writable. It is correctly implemented.381-394: The
installEspIdfSbom()method installs theesp-idf-sbomtool using pip. It correctly sets up the command arguments and environment and runs the command.396-407: The
getEspIdfSbomInstalledStatus()method checks if theesp-idf-sbomtool is installed by running thepip listcommand and checking the output. It is correctly implemented.
|
@sigmaaa hi ! Tested: Create project - command "ESP-IDF: SBOM Tool" doesn't work ERROR LOG: |
| package com.espressif.idf.ui.dialogs; | ||
|
|
||
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.nio.file.FileSystems; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Paths; | ||
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import org.eclipse.core.filesystem.EFS; | ||
| import org.eclipse.core.filesystem.IFileStore; | ||
| import org.eclipse.core.resources.IProject; | ||
| import org.eclipse.core.resources.IResource; | ||
| import org.eclipse.core.runtime.IProgressMonitor; | ||
| import org.eclipse.core.runtime.IStatus; | ||
| import org.eclipse.core.runtime.Path; | ||
| import org.eclipse.core.runtime.Status; | ||
| import org.eclipse.core.runtime.jobs.Job; | ||
| import org.eclipse.jface.dialogs.IDialogConstants; | ||
| import org.eclipse.jface.dialogs.TitleAreaDialog; | ||
| import org.eclipse.jface.text.BadLocationException; | ||
| import org.eclipse.jface.viewers.ISelection; | ||
| import org.eclipse.jface.viewers.IStructuredSelection; | ||
| import org.eclipse.swt.SWT; | ||
| import org.eclipse.swt.events.SelectionAdapter; | ||
| import org.eclipse.swt.events.SelectionEvent; | ||
| import org.eclipse.swt.layout.GridData; | ||
| import org.eclipse.swt.layout.GridLayout; | ||
| import org.eclipse.swt.widgets.Button; | ||
| import org.eclipse.swt.widgets.Composite; | ||
| import org.eclipse.swt.widgets.Control; | ||
| import org.eclipse.swt.widgets.FileDialog; | ||
| import org.eclipse.swt.widgets.Label; | ||
| import org.eclipse.swt.widgets.Shell; | ||
| import org.eclipse.swt.widgets.Text; | ||
| import org.eclipse.ui.IWorkbenchPage; | ||
| import org.eclipse.ui.PartInitException; | ||
| import org.eclipse.ui.PlatformUI; | ||
| import org.eclipse.ui.console.IHyperlink; | ||
| import org.eclipse.ui.console.IPatternMatchListener; | ||
| import org.eclipse.ui.console.MessageConsoleStream; | ||
| import org.eclipse.ui.console.PatternMatchEvent; | ||
| import org.eclipse.ui.console.TextConsole; | ||
| import org.eclipse.ui.ide.IDE; | ||
|
|
||
| import com.espressif.idf.core.IDFCorePlugin; | ||
| import com.espressif.idf.core.ProcessBuilderFactory; | ||
| import com.espressif.idf.core.logging.Logger; | ||
| import com.espressif.idf.core.util.IDFUtil; | ||
| import com.espressif.idf.core.util.StringUtil; | ||
| import com.espressif.idf.ui.IDFConsole; | ||
| import com.espressif.idf.ui.update.Messages; | ||
|
|
||
| public class SbomCommandDialog extends TitleAreaDialog | ||
| { | ||
|
|
||
| private static final String DEFAULT_OUTPUT_FILE_NAME = "sbom.txt"; //$NON-NLS-1$ | ||
| private static final String ESP_IDF_SBOM_COMMAND_NAME = "esp_idf_sbom"; //$NON-NLS-1$ | ||
| protected static final String[] EXTENSIONS = { ".json" }; //$NON-NLS-1$ | ||
| private MessageConsoleStream console; | ||
| private Button saveOutputToFileCheckBoxButton; | ||
| private Text outputFileText; | ||
| private IProject selectedProject; | ||
| private Text projectDescriptionPathText; | ||
| private String projectDescription; | ||
| private boolean saveOutputFileStatus; | ||
| private String outputFilePath; | ||
|
|
||
| public SbomCommandDialog(Shell parentShell) | ||
| { | ||
| super(parentShell); | ||
| } | ||
|
|
||
| @Override | ||
| public void create() | ||
| { | ||
| super.create(); | ||
| getShell().setText(Messages.SbomCommandDialog_SbomTitle); | ||
| setTitle(Messages.SbomCommandDialog_SbomTitle); | ||
| setMessage( | ||
| Messages.SbomCommandDialog_SbomInfoMsg); | ||
| setDefaults(); | ||
| } | ||
|
|
||
| @Override | ||
| protected Control createDialogArea(Composite parent) | ||
| { | ||
| Composite area = (Composite) super.createDialogArea(parent); | ||
|
|
||
| Composite container = new Composite(area, SWT.NONE); | ||
| container.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); | ||
| GridLayout layout = new GridLayout(3, false); | ||
| container.setLayout(layout); | ||
|
|
||
| saveOutputToFileCheckBoxButton = new Button(container, SWT.CHECK); | ||
| saveOutputToFileCheckBoxButton.setText(Messages.SbomCommandDialog_RedirectOutputCheckBoxLbl); | ||
| saveOutputToFileCheckBoxButton.setLayoutData(new GridData(SWT.FILL, SWT.FILL, false, false, 3, 1)); | ||
|
|
||
|
|
||
| Label projectDescriptionPathLbl = new Label(container, SWT.NONE); | ||
| projectDescriptionPathLbl.setText(Messages.SbomCommandDialog_ProjectDescriptionPathLbl); | ||
| projectDescriptionPathText = new Text(container, SWT.SINGLE | SWT.BORDER | SWT.H_SCROLL); | ||
| projectDescriptionPathText.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); | ||
| Button projectDescriptionBtn = new Button(container, SWT.PUSH); | ||
| projectDescriptionBtn.setLayoutData(new GridData(SWT.RIGHT, SWT.FILL, false, false, 1, 1)); | ||
| projectDescriptionBtn.setText(Messages.SbomCommandDialog_BrowseBtnTxt); | ||
| projectDescriptionBtn.addSelectionListener(new SelectionAdapter() | ||
| { | ||
| @Override | ||
| public void widgetSelected(SelectionEvent e) | ||
| { | ||
| FileDialog fileSelectionDialog = new FileDialog(getParentShell()); | ||
| fileSelectionDialog.setFilterExtensions(EXTENSIONS); | ||
| fileSelectionDialog.setFilterPath(buildProjectDescriptionPath()); | ||
| String selectedFilePath = fileSelectionDialog.open(); | ||
|
|
||
| if (selectedFilePath != null && !selectedFilePath.isEmpty()) | ||
| { | ||
| projectDescriptionPathText.setText(selectedFilePath); | ||
| } | ||
| super.widgetSelected(e); | ||
| } | ||
| }); | ||
|
|
||
|
|
||
| Label outputFileLbl = new Label(container, SWT.NONE); | ||
| outputFileLbl.setText(Messages.SbomCommandDialog_OutputFilePathLbl); | ||
| outputFileText = new Text(container, SWT.NONE); | ||
| outputFileText.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); | ||
| Button outputFileBrowseBtn = new Button(container, SWT.PUSH); | ||
| outputFileBrowseBtn.setText(Messages.SbomCommandDialog_BrowseBtnTxt); | ||
| outputFileBrowseBtn.addSelectionListener(new SelectionAdapter() | ||
| { | ||
| @Override | ||
| public void widgetSelected(SelectionEvent e) | ||
| { | ||
| FileDialog fileSelectionDialog = new FileDialog(getParentShell()); | ||
| fileSelectionDialog.setFilterPath(buildProjectDescriptionPath()); | ||
| String selectedFilePath = fileSelectionDialog.open(); | ||
|
|
||
| if (selectedFilePath != null && !selectedFilePath.isEmpty()) | ||
| { | ||
| outputFileText.setText(selectedFilePath); | ||
| } | ||
| super.widgetSelected(e); | ||
| } | ||
| }); | ||
| saveOutputToFileCheckBoxButton.addListener(SWT.Selection, e -> { | ||
| outputFileLbl.setVisible(saveOutputToFileCheckBoxButton.getSelection()); | ||
| outputFileText.setVisible(saveOutputToFileCheckBoxButton.getSelection()); | ||
| outputFileBrowseBtn.setVisible(saveOutputToFileCheckBoxButton.getSelection()); | ||
| container.requestLayout(); | ||
| }); | ||
| saveOutputToFileCheckBoxButton.setSelection(false); | ||
| saveOutputToFileCheckBoxButton.notifyListeners(SWT.Selection, null); | ||
|
|
||
| outputFileText.addListener(SWT.Modify, e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInput())); | ||
| projectDescriptionPathText.addListener(SWT.Modify, | ||
| e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInput())); | ||
| return super.createDialogArea(parent); | ||
| } | ||
|
|
||
| protected String runCommand(List<String> arguments, Path workDir, Map<String, String> env) | ||
| { | ||
| String exportCmdOp = StringUtil.EMPTY; | ||
| ProcessBuilderFactory processRunner = new ProcessBuilderFactory(); | ||
| try | ||
| { | ||
| IStatus status = processRunner.runInBackground(arguments, workDir, env); | ||
| if (status == null) | ||
| { | ||
| IStatus errorStatus = IDFCorePlugin.errorStatus(Messages.SbomCommandDialog_StatusCantBeNullErrorMsg, null); | ||
| Logger.log(IDFCorePlugin.getPlugin(), errorStatus); | ||
| return errorStatus.getMessage(); | ||
| } | ||
|
|
||
| exportCmdOp = status.getMessage(); | ||
| Logger.log(exportCmdOp); | ||
| } | ||
| catch (IOException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return exportCmdOp; | ||
| } | ||
|
|
||
| @Override | ||
| protected void okPressed() | ||
| { | ||
| console = new IDFConsole().getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, false); | ||
| Job espIdfSbomJob = new Job(Messages.SbomCommandDialog_EspIdfSbomJobName) | ||
| { | ||
|
|
||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| if (!getEspIdfSbomInstalledStatus()) | ||
| { | ||
| installEspIdfSbom(); | ||
| } | ||
| runEspIdfSbomCommand(); | ||
| return Status.OK_STATUS; | ||
| } | ||
| }; | ||
| espIdfSbomJob.schedule(); | ||
| projectDescription = projectDescriptionPathText.getText(); | ||
| saveOutputFileStatus = saveOutputToFileCheckBoxButton.getSelection(); | ||
| outputFilePath = outputFileText.getText(); | ||
| super.okPressed(); | ||
| } | ||
|
|
||
| @Override | ||
| protected boolean isResizable() | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| private void setDefaults() | ||
| { | ||
| ISelection selection = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getSelectionService() | ||
| .getSelection(); | ||
| if (selection instanceof IStructuredSelection) | ||
| { | ||
| Object element = ((IStructuredSelection) selection).getFirstElement(); | ||
|
|
||
| if (element instanceof IResource) | ||
| { | ||
| selectedProject = ((IResource) element).getProject(); | ||
| } | ||
| } | ||
|
|
||
| projectDescriptionPathText | ||
| .setText(buildProjectDescriptionPath()); | ||
| if (!Files.exists(Paths.get(projectDescriptionPathText.getText()))) | ||
| { | ||
| setMessage(Messages.SbomCommandDialog_ProjectDescDoesntExistDefaultErrorMsg); | ||
| getButton(IDialogConstants.OK_ID).setEnabled(false); | ||
| } | ||
|
|
||
| outputFileText.setText( | ||
| selectedProject.getLocationURI().getPath().concat(File.separator).concat(DEFAULT_OUTPUT_FILE_NAME)); | ||
|
|
||
| } | ||
|
|
||
| private String buildProjectDescriptionPath() | ||
| { | ||
| return String.join(FileSystems.getDefault().getSeparator(), | ||
| Paths.get(selectedProject.getLocationURI()).toString(), "build", //$NON-NLS-1$ | ||
| "project_description.json"); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| private void runEspIdfSbomCommand() | ||
| { | ||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| List<String> arguments = new ArrayList<>(); | ||
| final String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(); | ||
| arguments.add(pythonEnvPath); | ||
| arguments.add("-m"); //$NON-NLS-1$ | ||
| arguments.add(ESP_IDF_SBOM_COMMAND_NAME); | ||
| arguments.add("create"); //$NON-NLS-1$ | ||
| arguments.add(projectDescription); | ||
| if (saveOutputFileStatus) | ||
| { | ||
| arguments.add("--output-file"); //$NON-NLS-1$ | ||
| arguments.add(outputFilePath); | ||
| } | ||
| String cmdOutput = runCommand(arguments, null, environment); | ||
| cmdOutput = cmdOutput.isEmpty() && saveOutputFileStatus | ||
| ? String.format(Messages.SbomCommandDialog_ConsoleRedirectedOutputFormatString, outputFilePath) | ||
| : cmdOutput; | ||
| console.getConsole().addPatternMatchListener(getPatternMatchListener()); | ||
| console.println(cmdOutput); | ||
|
|
||
| } | ||
|
|
||
| private IPatternMatchListener getPatternMatchListener() | ||
| { | ||
| return new IPatternMatchListener() | ||
| { | ||
|
|
||
| public void matchFound(PatternMatchEvent event) | ||
| { | ||
|
|
||
| try | ||
| { | ||
| IHyperlink hepHyperlink = createHyperlinkWhichOpensFileInEditor(); | ||
| console.getConsole().addHyperlink(hepHyperlink, event.getOffset(), event.getLength()); | ||
| } | ||
| catch (BadLocationException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } | ||
|
|
||
| private IHyperlink createHyperlinkWhichOpensFileInEditor() | ||
| { | ||
| return new IHyperlink() | ||
| { | ||
| public void linkActivated() | ||
| { | ||
| IFileStore fileStore = EFS.getLocalFileSystem().getStore(new Path(outputFilePath)); | ||
| if (!fileStore.fetchInfo().isDirectory() && fileStore.fetchInfo().exists()) | ||
| { | ||
| IWorkbenchPage page = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage(); | ||
| try | ||
| { | ||
| IDE.openEditorOnFileStore(page, fileStore); | ||
| } | ||
| catch (PartInitException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| public void linkEntered() | ||
| { | ||
| } | ||
|
|
||
| public void linkExited() | ||
| { | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| public void disconnect() | ||
| { | ||
| } | ||
|
|
||
| public void connect(TextConsole console) | ||
| { | ||
| } | ||
|
|
||
| public String getPattern() | ||
| { | ||
| return outputFilePath; | ||
| } | ||
|
|
||
| public String getLineQualifier() | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| public int getCompilerFlags() | ||
| { | ||
| return 0; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private boolean validateInput() | ||
| { | ||
| boolean validateStatus = true; | ||
| if (!Files.exists(Paths.get(projectDescriptionPathText.getText()))) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_ProjectDescDoesntExistsErrorMsg); | ||
| } | ||
|
|
||
| if (saveOutputToFileCheckBoxButton.getSelection() | ||
| && checkIfFileIsNotWritable(Paths.get(outputFileText.getText()))) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_OutputFileNotWritabbleErrorMsg); | ||
| } | ||
|
|
||
| if (validateStatus) | ||
| { | ||
| setErrorMessage(null); | ||
| } | ||
| return validateStatus; | ||
| } | ||
|
|
||
| private boolean checkIfFileIsNotWritable(java.nio.file.Path pathToFile) | ||
| { | ||
| return Files.exists(pathToFile) && !Files.isWritable(pathToFile); | ||
| } | ||
|
|
||
| private void installEspIdfSbom() | ||
| { | ||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| List<String> arguments = new ArrayList<>(); | ||
| final String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(); | ||
| arguments.add(pythonEnvPath); | ||
| arguments.add("-m"); //$NON-NLS-1$ | ||
| arguments.add("pip"); //$NON-NLS-1$ | ||
| arguments.add("install"); //$NON-NLS-1$ | ||
| arguments.add(ESP_IDF_SBOM_COMMAND_NAME); | ||
| String cmdOutput = runCommand(arguments, null, environment); | ||
| console.println(cmdOutput); | ||
|
|
||
| } | ||
|
|
||
| private boolean getEspIdfSbomInstalledStatus() | ||
| { | ||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| List<String> arguments = new ArrayList<>(); | ||
| final String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(); | ||
| arguments.add(pythonEnvPath); | ||
| arguments.add("-m"); //$NON-NLS-1$ | ||
| arguments.add("pip"); //$NON-NLS-1$ | ||
| arguments.add("list"); //$NON-NLS-1$ | ||
| String cmdOutput = runCommand(arguments, null, environment); | ||
| return cmdOutput.contains("esp-idf-sbom"); //$NON-NLS-1$ | ||
| } | ||
| } |
There was a problem hiding this comment.
The code looks well-structured and follows the Eclipse plugin development guidelines. However, there are a few areas that could be improved for better maintainability and error handling.
-
The
runCommandmethod (lines 167-189) could be improved by adding more detailed error handling. Currently, it only logs the error and returns the error message. It would be better to throw an exception and let the caller handle it. This would make the error handling more explicit and easier to trace. -
The
runEspIdfSbomCommandmethod (lines 255-277) could be improved by adding error handling for the case where the command execution fails. Currently, it just logs the output of the command, but it doesn't check if the command was successful or not. -
The
installEspIdfSbommethod (lines 383-396) andgetEspIdfSbomInstalledStatusmethod (lines 398-409) could be moved to a separate utility class. This would make the code more modular and easier to test. -
The
validateInputmethod (lines 355-376) could be improved by splitting it into separate methods for each validation rule. This would make the code more readable and easier to maintain. -
The
getPatternMatchListenermethod (lines 279-352) could be improved by moving the anonymous class to a separate top-level class. This would make the code more readable and easier to test. -
The
createDialogAreamethod (lines 90-165) is quite long and does a lot of things. It could be improved by splitting it into smaller methods, each doing one specific thing. -
The
okPressedmethod (lines 192-213) could be improved by adding error handling for the case where the job execution fails. Currently, it just schedules the job, but it doesn't check if the job was successful or not. -
The
setDefaultsmethod (lines 221-246) could be improved by adding error handling for the case where the project description file doesn't exist. Currently, it just sets the error message, but it doesn't prevent the dialog from being used.
|
@sigmaaa hi ! Tested under: can't install BoM tool, incorrect Output File Path - getting error: |
|
@sigmaaa hi ! Tested under: Steps:
Also, there is no input symbols validation. |
| outputFileText.addListener(SWT.Modify, e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInput())); | ||
| projectDescriptionPathText.addListener(SWT.Modify, | ||
| e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInput())); |
There was a problem hiding this comment.
The validation of the input fields is only done when the text is modified. This might lead to a situation where the "OK" button is enabled even though the input is invalid (e.g., if the file at the path specified in the outputFileText or projectDescriptionPathText is deleted after the dialog is opened). Consider adding a validation check when the "OK" button is pressed.
| protected String runCommand(List<String> arguments, Path workDir, Map<String, String> env) | ||
| { | ||
| String exportCmdOp = StringUtil.EMPTY; | ||
| ProcessBuilderFactory processRunner = new ProcessBuilderFactory(); | ||
| try | ||
| { | ||
| IStatus status = processRunner.runInBackground(arguments, workDir, env); | ||
| if (status == null) | ||
| { | ||
| IStatus errorStatus = IDFCorePlugin.errorStatus(Messages.SbomCommandDialog_StatusCantBeNullErrorMsg, null); | ||
| Logger.log(IDFCorePlugin.getPlugin(), errorStatus); | ||
| return errorStatus.getMessage(); | ||
| } | ||
|
|
||
| exportCmdOp = status.getMessage(); | ||
| Logger.log(exportCmdOp); | ||
| } | ||
| catch (IOException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return exportCmdOp; | ||
| } |
There was a problem hiding this comment.
The runCommand method does not handle the case where the command execution fails (i.e., when status.getSeverity() == IStatus.ERROR). This could lead to misleading error messages being displayed to the user. Consider checking the severity of the status and handling errors appropriately.
| if (!getEspIdfSbomInstalledStatus()) | ||
| { | ||
| installEspIdfSbom(); | ||
| } | ||
| runEspIdfSbomCommand(); | ||
| return Status.OK_STATUS; |
There was a problem hiding this comment.
The run method of the espIdfSbomJob does not handle exceptions that might be thrown by the installEspIdfSbom and runEspIdfSbomCommand methods. This could lead to unhandled exceptions. Consider adding a try-catch block to handle potential exceptions.
| private boolean validateInput() | ||
| { | ||
| boolean validateStatus = true; | ||
| if (!Files.exists(Paths.get(projectDescriptionPathText.getText()))) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_ProjectDescDoesntExistsErrorMsg); | ||
| } | ||
|
|
||
| if (saveOutputToFileCheckBoxButton.getSelection() | ||
| && checkIfFileIsNotWritable(Paths.get(outputFileText.getText()))) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_OutputFileNotWritabbleErrorMsg); | ||
| } | ||
|
|
||
| if (validateStatus) | ||
| { | ||
| setErrorMessage(null); | ||
| } | ||
| return validateStatus; | ||
| } |
There was a problem hiding this comment.
The validateInput method checks if the file at the path specified in projectDescriptionPathText exists and if the file at the path specified in outputFileText is writable. However, it does not check if the paths themselves are valid. Consider adding a check to ensure that the paths are valid.
|
@sigmaaa hi ! Tested under: OS - Windows 10 Not able to create Output File: Steps:
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Files selected for processing (3)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java
Additional comments (Suppressed): 1
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1)
- 1-431: The new hunk introduces a dialog for the SBOM tool command execution. It includes input validation, command execution, and output handling. The dialog allows users to select a project description file, specify an output file path, and run the SBOM command. The command is executed in the background using
ProcessBuilderFactory, with the output either printed to the console or redirected to a specified file. The dialog also provides a hyperlink to open the output file in an editor. The changes are non-breaking and require a documentation update.
| SbomCommandDialog_BrowseBtnTxt=Browse | ||
| SbomCommandDialog_ConsoleRedirectedOutputFormatString=The output was redirected to the file: %s | ||
| SbomCommandDialog_EspIdfSbomJobName=Running esp-idf-sbom command... | ||
| SbomCommandDialog_InvalidProjectDescPathErrorMsg=The path to project_description.json is invalid |
There was a problem hiding this comment.
Typographical error in the error message string. Correct the spelling of "Invalid".
- SbomCommandDialog_IvalidOutputFilePathErrorMsg=The path to the output file is invalid
+ SbomCommandDialog_InvalidOutputFilePathErrorMsg=The path to the output file is invalid| SbomCommandDialog_ProjectDescDoesntExistDefaultErrorMsg=Provided project_description.json doesn't exist. Build the project first. | ||
| SbomCommandDialog_ProjectDescDoesntExistsErrorMsg=Provided project_description.json doesn't exist. |
There was a problem hiding this comment.
These two error messages are very similar and could potentially be confusing for the user. Consider consolidating them into a single, clear error message.
- SbomCommandDialog_ProjectDescDoesntExistDefaultErrorMsg=Provided project_description.json doesn't exist. Build the project first.
- SbomCommandDialog_ProjectDescDoesntExistsErrorMsg=Provided project_description.json doesn't exist.
+ SbomCommandDialog_ProjectDescDoesntExistErrorMsg=Provided project_description.json doesn't exist. Build the project first.| SbomCommandDialog_RedirectOutputCheckBoxLbl=Redirect output to the file | ||
| SbomCommandDialog_SbomInfoMsg=Provide path to the project description file and click OK. The result of the command will be displayed on the console | ||
| SbomCommandDialog_SbomTitle=Software Bill of Materials Tool | ||
| SbomCommandDialog_StatusCantBeNullErrorMsg=Status can't be null |
There was a problem hiding this comment.
The error message "Status can't be null" might be unclear to the user. Consider providing more context or a more descriptive error message.
- SbomCommandDialog_StatusCantBeNullErrorMsg=Status can't be null
+ SbomCommandDialog_StatusCantBeNullErrorMsg=Operation status cannot be null. Please ensure the operation was executed correctly.| @Override | ||
| protected void okPressed() | ||
| { | ||
| console = new IDFConsole().getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, false); | ||
| Job espIdfSbomJob = new Job(Messages.SbomCommandDialog_EspIdfSbomJobName) | ||
| { | ||
|
|
||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| if (!getEspIdfSbomInstalledStatus()) | ||
| { | ||
| installEspIdfSbom(); | ||
| } | ||
| runEspIdfSbomCommand(); | ||
| return Status.OK_STATUS; | ||
| } | ||
| }; | ||
| espIdfSbomJob.schedule(); | ||
| projectDescription = projectDescriptionPathText.getText(); | ||
| saveOutputFileStatus = saveOutputToFileCheckBoxButton.getSelection(); | ||
| outputFilePath = outputFileText.getText(); | ||
| super.okPressed(); | ||
| } |
There was a problem hiding this comment.
The okPressed method is used to execute the SBOM command when the OK button is pressed. It creates a new job to run the command and schedules it for execution. However, it doesn't handle the case where the job execution fails. It's recommended to add error handling to manage such scenarios.
protected IStatus run(IProgressMonitor monitor)
{
if (!getEspIdfSbomInstalledStatus())
{
installEspIdfSbom();
}
runEspIdfSbomCommand();
- return Status.OK_STATUS;
+ try {
+ runEspIdfSbomCommand();
+ return Status.OK_STATUS;
+ } catch (Exception e) {
+ return new Status(IStatus.ERROR, IDFCorePlugin.PLUGIN_ID, "SBOM command execution failed", e);
+ }
}
};| boolean validateStatus = true; | ||
| java.nio.file.Path projectDescriptionPath = null; | ||
| try | ||
| { | ||
| projectDescriptionPath = Paths.get(projectDescriptionPathText.getText()); | ||
| } | ||
| catch (InvalidPathException e) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_InvalidProjectDescPathErrorMsg); | ||
| } | ||
| if (projectDescriptionPath != null && !Files.exists(projectDescriptionPath)) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_ProjectDescDoesntExistsErrorMsg); | ||
| } | ||
|
|
||
| java.nio.file.Path outputFilePath = null; | ||
| try | ||
| { | ||
| outputFilePath = Paths.get(outputFileText.getText()); | ||
| } | ||
| catch (InvalidPathException e) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_IvalidOutputFilePathErrorMsg); | ||
| } | ||
| if (outputFilePath != null && saveOutputToFileCheckBoxButton.getSelection() | ||
| && checkIfFileIsNotWritable(outputFilePath)) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_OutputFileNotWritabbleErrorMsg); | ||
| } | ||
|
|
||
| if (validateStatus) | ||
| { | ||
| setErrorMessage(null); | ||
| } | ||
| return validateStatus; | ||
| } |
There was a problem hiding this comment.
The validateInput method is used to validate the input fields in the dialog. It checks if the project description path and output file path are valid and writable. However, it doesn't handle the case where the paths are null. It's recommended to add null checks to prevent NullPointerExceptions.
boolean validateStatus = true;
java.nio.file.Path projectDescriptionPath = null;
try
{
projectDescriptionPath = Paths.get(projectDescriptionPathText.getText());
}
catch (InvalidPathException e)
{
validateStatus = false;
setErrorMessage(Messages.SbomCommandDialog_InvalidProjectDescPathErrorMsg);
}
if (projectDescriptionPath != null && !Files.exists(projectDescriptionPath))
{
validateStatus = false;
setErrorMessage(Messages.SbomCommandDialog_ProjectDescDoesntExistsErrorMsg);
}
java.nio.file.Path outputFilePath = null;
try
{
outputFilePath = Paths.get(outputFileText.getText());
}
catch (InvalidPathException e)
{
validateStatus = false;
setErrorMessage(Messages.SbomCommandDialog_IvalidOutputFilePathErrorMsg);
}
if (outputFilePath != null && saveOutputToFileCheckBoxButton.getSelection()
&& checkIfFileIsNotWritable(outputFilePath))
{
validateStatus = false;
setErrorMessage(Messages.SbomCommandDialog_OutputFileNotWritabbleErrorMsg);
}
if (validateStatus)
{
setErrorMessage(null);
}
return validateStatus;
|
Hi @AndriiFilippov, thanks for reviewing. Mentioned errors should be fixed after latest commit |
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
Self reviewed. Some code can be extracted to increase readability, for example, the part with PatternMatchListener. However, I'm not sure if it's necessary because most probably we will not have additional benefits from such abstraction.
|
@sigmaaa hi ! OS - Windows 10
Steps: ESP-IDF: SBOM Tool -> click OK -> ESP-IDF: SBOM Tool -> select "Redirect output to the file" -> click OK -> result:
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1 hunks)
Additional comments (Suppressed): 1
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1)
- 1-428: The new hunk introduces a dialog for the Software Bill of Materials (SBOM) tool in the ESP-IDF project. The dialog allows users to select a project description file, specify an output file path, and execute the SBOM command. The command is run in the background, and the output is displayed in a console. The dialog also handles the installation of the SBOM tool if it's not installed. The code seems to follow good practices for exception handling, user input validation, and UI responsiveness. However, there are a few areas that could be improved for better maintainability and modularity.
| protected void okPressed() | ||
| { | ||
| console = new IDFConsole().getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, false); | ||
| Job espIdfSbomJob = new Job(Messages.SbomCommandDialog_EspIdfSbomJobName) | ||
| { | ||
|
|
||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| if (!getEspIdfSbomInstalledStatus()) | ||
| { | ||
| installEspIdfSbom(); | ||
| } | ||
| runEspIdfSbomCommand(); | ||
| return Status.OK_STATUS; | ||
| } | ||
| }; | ||
| espIdfSbomJob.schedule(); | ||
| projectDescription = projectDescriptionPathText.getText(); | ||
| saveOutputFileStatus = saveOutputToFileCheckBoxButton.getSelection(); | ||
| outputFilePath = outputFileText.getText(); | ||
| super.okPressed(); | ||
| } |
There was a problem hiding this comment.
The okPressed method is doing a lot of work, including creating a console, scheduling a job, and setting several instance variables. Consider breaking this method down into smaller, more manageable methods.
| private String buildProjectDescriptionPath() | ||
| { | ||
| return String.join(FileSystems.getDefault().getSeparator(), | ||
| Paths.get(selectedProject.getLocationURI()).toString(), "build", //$NON-NLS-1$ | ||
| "project_description.json"); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
The buildProjectDescriptionPath method is hardcoding the path to the project description file. This could lead to issues if the file structure changes in the future. Consider making this more flexible, for example by using a configuration file or a setting.
| package com.espressif.idf.ui.dialogs; | ||
|
|
||
| import java.io.IOException; | ||
| import java.nio.file.FileSystems; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.InvalidPathException; | ||
| import java.nio.file.Paths; | ||
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import org.eclipse.core.filesystem.EFS; | ||
| import org.eclipse.core.filesystem.IFileStore; | ||
| import org.eclipse.core.resources.IProject; | ||
| import org.eclipse.core.resources.IResource; | ||
| import org.eclipse.core.runtime.IProgressMonitor; | ||
| import org.eclipse.core.runtime.IStatus; | ||
| import org.eclipse.core.runtime.Path; | ||
| import org.eclipse.core.runtime.Status; | ||
| import org.eclipse.core.runtime.jobs.Job; | ||
| import org.eclipse.jface.dialogs.IDialogConstants; | ||
| import org.eclipse.jface.dialogs.TitleAreaDialog; | ||
| import org.eclipse.jface.text.BadLocationException; | ||
| import org.eclipse.jface.viewers.ISelection; | ||
| import org.eclipse.jface.viewers.IStructuredSelection; | ||
| import org.eclipse.swt.SWT; | ||
| import org.eclipse.swt.events.SelectionAdapter; | ||
| import org.eclipse.swt.events.SelectionEvent; | ||
| import org.eclipse.swt.layout.GridData; | ||
| import org.eclipse.swt.layout.GridLayout; | ||
| import org.eclipse.swt.widgets.Button; | ||
| import org.eclipse.swt.widgets.Composite; | ||
| import org.eclipse.swt.widgets.Control; | ||
| import org.eclipse.swt.widgets.FileDialog; | ||
| import org.eclipse.swt.widgets.Label; | ||
| import org.eclipse.swt.widgets.Shell; | ||
| import org.eclipse.swt.widgets.Text; | ||
| import org.eclipse.ui.IWorkbenchPage; | ||
| import org.eclipse.ui.PartInitException; | ||
| import org.eclipse.ui.PlatformUI; | ||
| import org.eclipse.ui.console.IHyperlink; | ||
| import org.eclipse.ui.console.IPatternMatchListener; | ||
| import org.eclipse.ui.console.MessageConsoleStream; | ||
| import org.eclipse.ui.console.PatternMatchEvent; | ||
| import org.eclipse.ui.console.TextConsole; | ||
| import org.eclipse.ui.ide.IDE; | ||
|
|
||
| import com.espressif.idf.core.IDFCorePlugin; | ||
| import com.espressif.idf.core.ProcessBuilderFactory; | ||
| import com.espressif.idf.core.logging.Logger; | ||
| import com.espressif.idf.core.util.IDFUtil; | ||
| import com.espressif.idf.core.util.StringUtil; | ||
| import com.espressif.idf.ui.IDFConsole; | ||
| import com.espressif.idf.ui.update.Messages; | ||
|
|
||
| public class SbomCommandDialog extends TitleAreaDialog | ||
| { | ||
|
|
||
| private static final String PATH_REGEX = "(\\S+)"; //$NON-NLS-1$ | ||
| private static final String DEFAULT_OUTPUT_FILE_NAME = "sbom.txt"; //$NON-NLS-1$ | ||
| private static final String ESP_IDF_SBOM_COMMAND_NAME = "esp_idf_sbom"; //$NON-NLS-1$ | ||
| protected static final String[] EXTENSIONS = { "*.json" }; //$NON-NLS-1$ | ||
| private MessageConsoleStream console; | ||
| private Button saveOutputToFileCheckBoxButton; | ||
| private Text outputFileText; | ||
| private IProject selectedProject; | ||
| private Text projectDescriptionPathText; | ||
| private String projectDescription; | ||
| private boolean saveOutputFileStatus; | ||
| private String outputFilePath; | ||
|
|
||
| public SbomCommandDialog(Shell parentShell) | ||
| { | ||
| super(parentShell); | ||
| } | ||
|
|
||
| @Override | ||
| public void create() | ||
| { | ||
| super.create(); | ||
| getShell().setText(Messages.SbomCommandDialog_SbomTitle); | ||
| setTitle(Messages.SbomCommandDialog_SbomTitle); | ||
| setMessage(Messages.SbomCommandDialog_SbomInfoMsg); | ||
| setDefaults(); | ||
| } | ||
|
|
||
| @Override | ||
| protected Control createDialogArea(Composite parent) | ||
| { | ||
| Composite area = (Composite) super.createDialogArea(parent); | ||
|
|
||
| Composite container = new Composite(area, SWT.NONE); | ||
| container.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); | ||
| GridLayout layout = new GridLayout(3, false); | ||
| container.setLayout(layout); | ||
|
|
||
| saveOutputToFileCheckBoxButton = new Button(container, SWT.CHECK); | ||
| saveOutputToFileCheckBoxButton.setText(Messages.SbomCommandDialog_RedirectOutputCheckBoxLbl); | ||
| saveOutputToFileCheckBoxButton.setLayoutData(new GridData(SWT.FILL, SWT.FILL, false, false, 3, 1)); | ||
|
|
||
| Label projectDescriptionPathLbl = new Label(container, SWT.NONE); | ||
| projectDescriptionPathLbl.setText(Messages.SbomCommandDialog_ProjectDescriptionPathLbl); | ||
| projectDescriptionPathText = new Text(container, SWT.SINGLE | SWT.BORDER | SWT.H_SCROLL); | ||
| projectDescriptionPathText.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); | ||
| Button projectDescriptionBtn = new Button(container, SWT.PUSH); | ||
| projectDescriptionBtn.setLayoutData(new GridData(SWT.RIGHT, SWT.FILL, false, false, 1, 1)); | ||
| projectDescriptionBtn.setText(Messages.SbomCommandDialog_BrowseBtnTxt); | ||
| projectDescriptionBtn.addSelectionListener(new SelectionAdapter() | ||
| { | ||
| @Override | ||
| public void widgetSelected(SelectionEvent e) | ||
| { | ||
| FileDialog fileSelectionDialog = new FileDialog(getParentShell()); | ||
| fileSelectionDialog.setFilterExtensions(EXTENSIONS); | ||
| fileSelectionDialog.setFilterPath(buildProjectDescriptionPath()); | ||
| String selectedFilePath = fileSelectionDialog.open(); | ||
|
|
||
| if (selectedFilePath != null && !selectedFilePath.isEmpty()) | ||
| { | ||
| projectDescriptionPathText.setText(selectedFilePath); | ||
| } | ||
| super.widgetSelected(e); | ||
| } | ||
| }); | ||
|
|
||
| Label outputFileLbl = new Label(container, SWT.NONE); | ||
| outputFileLbl.setText(Messages.SbomCommandDialog_OutputFilePathLbl); | ||
| outputFileText = new Text(container, SWT.NONE); | ||
| outputFileText.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); | ||
| Button outputFileBrowseBtn = new Button(container, SWT.PUSH); | ||
| outputFileBrowseBtn.setText(Messages.SbomCommandDialog_BrowseBtnTxt); | ||
| outputFileBrowseBtn.addSelectionListener(new SelectionAdapter() | ||
| { | ||
| @Override | ||
| public void widgetSelected(SelectionEvent e) | ||
| { | ||
| FileDialog fileSelectionDialog = new FileDialog(getParentShell()); | ||
| fileSelectionDialog.setFilterPath(buildProjectDescriptionPath()); | ||
| String selectedFilePath = fileSelectionDialog.open(); | ||
|
|
||
| if (selectedFilePath != null && !selectedFilePath.isEmpty()) | ||
| { | ||
| outputFileText.setText(selectedFilePath); | ||
| } | ||
| super.widgetSelected(e); | ||
| } | ||
| }); | ||
| saveOutputToFileCheckBoxButton.addListener(SWT.Selection, e -> { | ||
| outputFileLbl.setVisible(saveOutputToFileCheckBoxButton.getSelection()); | ||
| outputFileText.setVisible(saveOutputToFileCheckBoxButton.getSelection()); | ||
| outputFileBrowseBtn.setVisible(saveOutputToFileCheckBoxButton.getSelection()); | ||
| container.requestLayout(); | ||
| }); | ||
| saveOutputToFileCheckBoxButton.setSelection(false); | ||
| saveOutputToFileCheckBoxButton.notifyListeners(SWT.Selection, null); | ||
|
|
||
| outputFileText.addListener(SWT.Modify, e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInput())); | ||
| projectDescriptionPathText.addListener(SWT.Modify, | ||
| e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInput())); | ||
| return super.createDialogArea(parent); | ||
| } | ||
|
|
||
| protected String runCommand(List<String> arguments, Path workDir, Map<String, String> env) | ||
| { | ||
| String exportCmdOp = StringUtil.EMPTY; | ||
| ProcessBuilderFactory processRunner = new ProcessBuilderFactory(); | ||
| try | ||
| { | ||
| IStatus status = processRunner.runInBackground(arguments, workDir, env); | ||
| if (status == null) | ||
| { | ||
| IStatus errorStatus = IDFCorePlugin.errorStatus(Messages.SbomCommandDialog_StatusCantBeNullErrorMsg, | ||
| null); | ||
| Logger.log(IDFCorePlugin.getPlugin(), errorStatus); | ||
| return errorStatus.getMessage(); | ||
| } | ||
|
|
||
| exportCmdOp = status.getMessage(); | ||
| Logger.log(exportCmdOp); | ||
| } | ||
| catch (IOException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return exportCmdOp; | ||
| } | ||
|
|
||
| @Override | ||
| protected void okPressed() | ||
| { | ||
| console = new IDFConsole().getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, false); | ||
| Job espIdfSbomJob = new Job(Messages.SbomCommandDialog_EspIdfSbomJobName) | ||
| { | ||
|
|
||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| if (!getEspIdfSbomInstalledStatus()) | ||
| { | ||
| installEspIdfSbom(); | ||
| } | ||
| runEspIdfSbomCommand(); | ||
| return Status.OK_STATUS; | ||
| } | ||
| }; | ||
| espIdfSbomJob.schedule(); | ||
| projectDescription = projectDescriptionPathText.getText(); | ||
| saveOutputFileStatus = saveOutputToFileCheckBoxButton.getSelection(); | ||
| outputFilePath = outputFileText.getText(); | ||
| super.okPressed(); | ||
| } | ||
|
|
||
| @Override | ||
| protected boolean isResizable() | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| private void setDefaults() | ||
| { | ||
| ISelection selection = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getSelectionService() | ||
| .getSelection(); | ||
| if (selection instanceof IStructuredSelection) | ||
| { | ||
| Object element = ((IStructuredSelection) selection).getFirstElement(); | ||
|
|
||
| if (element instanceof IResource) | ||
| { | ||
| selectedProject = ((IResource) element).getProject(); | ||
| } | ||
| } | ||
|
|
||
| projectDescriptionPathText.setText(buildProjectDescriptionPath()); | ||
| if (!Files.exists(Paths.get(projectDescriptionPathText.getText()))) | ||
| { | ||
| setMessage(Messages.SbomCommandDialog_ProjectDescDoesntExistDefaultErrorMsg); | ||
| getButton(IDialogConstants.OK_ID).setEnabled(false); | ||
| } | ||
| outputFileText.setText(String.join(FileSystems.getDefault().getSeparator(), | ||
| Paths.get(selectedProject.getLocationURI()).toString(), DEFAULT_OUTPUT_FILE_NAME)); | ||
|
|
||
| } | ||
|
|
||
| private String buildProjectDescriptionPath() | ||
| { | ||
| return String.join(FileSystems.getDefault().getSeparator(), | ||
| Paths.get(selectedProject.getLocationURI()).toString(), "build", //$NON-NLS-1$ | ||
| "project_description.json"); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| private void runEspIdfSbomCommand() | ||
| { | ||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| List<String> arguments = new ArrayList<>(); | ||
| final String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(); | ||
| arguments.add(pythonEnvPath); | ||
| arguments.add("-m"); //$NON-NLS-1$ | ||
| arguments.add(ESP_IDF_SBOM_COMMAND_NAME); | ||
| arguments.add("create"); //$NON-NLS-1$ | ||
| arguments.add(projectDescription); | ||
| if (saveOutputFileStatus) | ||
| { | ||
| arguments.add("--output-file"); //$NON-NLS-1$ | ||
| arguments.add(outputFilePath); | ||
| } | ||
| String cmdOutput = runCommand(arguments, null, environment); | ||
| cmdOutput = cmdOutput.isEmpty() && saveOutputFileStatus | ||
| ? String.format(Messages.SbomCommandDialog_ConsoleRedirectedOutputFormatString, outputFilePath) | ||
| : cmdOutput; | ||
| console.getConsole().addPatternMatchListener(getPatternMatchListener()); | ||
| console.println(cmdOutput); | ||
|
|
||
| } | ||
|
|
||
| private IPatternMatchListener getPatternMatchListener() | ||
| { | ||
| return new IPatternMatchListener() | ||
| { | ||
|
|
||
| public void matchFound(PatternMatchEvent event) | ||
| { | ||
|
|
||
| try | ||
| { | ||
| IHyperlink hepHyperlink = createHyperlinkWhichOpensFileInEditor(); | ||
| // calculating the right offset and length to highlight only the output path | ||
| console.getConsole().addHyperlink(hepHyperlink, getPattern().length() - PATH_REGEX.length(), | ||
| event.getLength() - getPattern().length() + PATH_REGEX.length()); | ||
| } | ||
| catch (BadLocationException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } | ||
|
|
||
| private IHyperlink createHyperlinkWhichOpensFileInEditor() | ||
| { | ||
| return new IHyperlink() | ||
| { | ||
| public void linkActivated() | ||
| { | ||
| IFileStore fileStore = EFS.getLocalFileSystem().getStore(new Path(outputFilePath)); | ||
| if (!fileStore.fetchInfo().isDirectory() && fileStore.fetchInfo().exists()) | ||
| { | ||
| IWorkbenchPage page = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage(); | ||
| try | ||
| { | ||
| IDE.openEditorOnFileStore(page, fileStore); | ||
| } | ||
| catch (PartInitException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| public void linkEntered() | ||
| { | ||
| } | ||
|
|
||
| public void linkExited() | ||
| { | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| public void disconnect() | ||
| { | ||
| } | ||
|
|
||
| public void connect(TextConsole console) | ||
| { | ||
| } | ||
|
|
||
| public String getPattern() | ||
| { | ||
| return String.format(Messages.SbomCommandDialog_ConsoleRedirectedOutputFormatString, PATH_REGEX); | ||
| } | ||
|
|
||
| public String getLineQualifier() | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| public int getCompilerFlags() | ||
| { | ||
| return 0; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private boolean validateInput() | ||
| { | ||
| boolean validateStatus = true; | ||
| java.nio.file.Path projectDescriptionPath = null; | ||
| try | ||
| { | ||
| projectDescriptionPath = Paths.get(projectDescriptionPathText.getText()); | ||
| } | ||
| catch (InvalidPathException e) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_InvalidProjectDescPathErrorMsg); | ||
| } | ||
| if (projectDescriptionPath != null && !Files.exists(projectDescriptionPath)) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_ProjectDescDoesntExistsErrorMsg); | ||
| } | ||
|
|
||
| java.nio.file.Path outputFilePath = null; | ||
| try | ||
| { | ||
| outputFilePath = Paths.get(outputFileText.getText()); | ||
| } | ||
| catch (InvalidPathException e) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_IvalidOutputFilePathErrorMsg); | ||
| } | ||
| if (outputFilePath != null && saveOutputToFileCheckBoxButton.getSelection() | ||
| && checkIfFileIsNotWritable(outputFilePath)) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_OutputFileNotWritabbleErrorMsg); | ||
| } | ||
|
|
||
| if (validateStatus) | ||
| { | ||
| setErrorMessage(null); | ||
| } | ||
| return validateStatus; | ||
| } | ||
|
|
||
| private boolean checkIfFileIsNotWritable(java.nio.file.Path pathToFile) | ||
| { | ||
| return Files.exists(pathToFile) && !Files.isWritable(pathToFile); | ||
| } | ||
|
|
||
| private void installEspIdfSbom() | ||
| { | ||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| List<String> arguments = new ArrayList<>(); | ||
| final String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(); | ||
| arguments.add(pythonEnvPath); | ||
| arguments.add("-m"); //$NON-NLS-1$ | ||
| arguments.add("pip"); //$NON-NLS-1$ | ||
| arguments.add("install"); //$NON-NLS-1$ | ||
| arguments.add(ESP_IDF_SBOM_COMMAND_NAME); | ||
| String cmdOutput = runCommand(arguments, null, environment); | ||
| console.println(cmdOutput); | ||
|
|
||
| } | ||
|
|
||
| private boolean getEspIdfSbomInstalledStatus() | ||
| { | ||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| List<String> arguments = new ArrayList<>(); | ||
| final String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(); | ||
| arguments.add(pythonEnvPath); | ||
| arguments.add("-m"); //$NON-NLS-1$ | ||
| arguments.add("pip"); //$NON-NLS-1$ | ||
| arguments.add("list"); //$NON-NLS-1$ | ||
| String cmdOutput = runCommand(arguments, null, environment); | ||
| return cmdOutput.contains("esp-idf-sbom"); //$NON-NLS-1$ | ||
| } | ||
| } |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [353-351]
The getPatternMatchListener method is creating a new anonymous class every time it's called. This could lead to unnecessary memory usage if the method is called frequently. Consider creating a separate class for the pattern match listener.
| private boolean checkIfFileIsNotWritable(java.nio.file.Path pathToFile) | ||
| { | ||
| return Files.exists(pathToFile) && !Files.isWritable(pathToFile); | ||
| } |
There was a problem hiding this comment.
The checkIfFileIsNotWritable method is checking if a file exists and is not writable. However, it's not handling the case where the file does not exist. Consider adding error handling for this case.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (3)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java
Additional comments (Suppressed): 1
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1)
- 46-60: The new messages added for the Software Bill of Materials (SBOM) tool seem to cover all the necessary user interactions and error scenarios. The messages are clear and concise, providing the user with the necessary information. However, there seems to be a typo in line 50, where "Ivalid" should be "Invalid".
- SbomCommandDialog_IvalidOutputFilePathErrorMsg=The path to the output file is invalid + SbomCommandDialog_InvalidOutputFilePathErrorMsg=The path to the output file is invalid
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java
Show resolved
Hide resolved
| protected Control createDialogArea(Composite parent) | ||
| { | ||
| Composite area = (Composite) super.createDialogArea(parent); | ||
|
|
||
| Composite container = new Composite(area, SWT.NONE); | ||
| container.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); | ||
| GridLayout layout = new GridLayout(3, false); | ||
| container.setLayout(layout); | ||
|
|
||
| saveOutputToFileCheckBoxButton = new Button(container, SWT.CHECK); | ||
| saveOutputToFileCheckBoxButton.setText(Messages.SbomCommandDialog_RedirectOutputCheckBoxLbl); | ||
| saveOutputToFileCheckBoxButton.setLayoutData(new GridData(SWT.FILL, SWT.FILL, false, false, 3, 1)); | ||
|
|
||
| Label projectDescriptionPathLbl = new Label(container, SWT.NONE); | ||
| projectDescriptionPathLbl.setText(Messages.SbomCommandDialog_ProjectDescriptionPathLbl); | ||
| projectDescriptionPathText = new Text(container, SWT.SINGLE | SWT.BORDER | SWT.H_SCROLL); | ||
| projectDescriptionPathText.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); | ||
| Button projectDescriptionBtn = new Button(container, SWT.PUSH); | ||
| projectDescriptionBtn.setLayoutData(new GridData(SWT.RIGHT, SWT.FILL, false, false, 1, 1)); | ||
| projectDescriptionBtn.setText(Messages.SbomCommandDialog_BrowseBtnTxt); | ||
| projectDescriptionBtn.addSelectionListener(new SelectionAdapter() | ||
| { | ||
| @Override | ||
| public void widgetSelected(SelectionEvent e) | ||
| { | ||
| FileDialog fileSelectionDialog = new FileDialog(getParentShell()); | ||
| fileSelectionDialog.setFilterExtensions(EXTENSIONS); | ||
| fileSelectionDialog.setFilterPath(buildProjectDescriptionPath()); | ||
| String selectedFilePath = fileSelectionDialog.open(); | ||
|
|
||
| if (selectedFilePath != null && !selectedFilePath.isEmpty()) | ||
| { | ||
| projectDescriptionPathText.setText(selectedFilePath); | ||
| } | ||
| super.widgetSelected(e); | ||
| } | ||
| }); | ||
|
|
||
| Label outputFileLbl = new Label(container, SWT.NONE); | ||
| outputFileLbl.setText(Messages.SbomCommandDialog_OutputFilePathLbl); | ||
| outputFileText = new Text(container, SWT.NONE); | ||
| outputFileText.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); | ||
| Button outputFileBrowseBtn = new Button(container, SWT.PUSH); | ||
| outputFileBrowseBtn.setText(Messages.SbomCommandDialog_BrowseBtnTxt); | ||
| outputFileBrowseBtn.addSelectionListener(new SelectionAdapter() | ||
| { | ||
| @Override | ||
| public void widgetSelected(SelectionEvent e) | ||
| { | ||
| FileDialog fileSelectionDialog = new FileDialog(getParentShell()); | ||
| fileSelectionDialog.setFilterPath(buildProjectDescriptionPath()); | ||
| String selectedFilePath = fileSelectionDialog.open(); | ||
|
|
||
| if (selectedFilePath != null && !selectedFilePath.isEmpty()) | ||
| { | ||
| outputFileText.setText(selectedFilePath); | ||
| } | ||
| super.widgetSelected(e); | ||
| } | ||
| }); | ||
| saveOutputToFileCheckBoxButton.addListener(SWT.Selection, e -> { | ||
| outputFileLbl.setVisible(saveOutputToFileCheckBoxButton.getSelection()); | ||
| outputFileText.setVisible(saveOutputToFileCheckBoxButton.getSelection()); | ||
| outputFileBrowseBtn.setVisible(saveOutputToFileCheckBoxButton.getSelection()); | ||
| container.requestLayout(); | ||
| }); | ||
|
|
||
| return super.createDialogArea(parent); | ||
| } |
There was a problem hiding this comment.
The UI layout is well-structured and the use of SWT for creating the dialog is appropriate. However, the createDialogArea method is quite long and does a lot of things. Consider breaking it down into smaller methods for better readability and maintainability. For example, you could have separate methods for creating the saveOutputToFileCheckBoxButton, projectDescriptionPathText, and outputFileText components.
| protected void okPressed() | ||
| { | ||
| console = new IDFConsole().getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, false); | ||
| Job refreshJob = new Job(Messages.SbomCommandDialog_RefreshProjectJob) | ||
| { | ||
|
|
||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| try | ||
| { | ||
| selectedProject.refreshLocal(IResource.DEPTH_INFINITE, null); | ||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return Status.OK_STATUS; | ||
| } | ||
| }; | ||
| Job espIdfSbomJob = new Job(Messages.SbomCommandDialog_EspIdfSbomJobName) | ||
| { | ||
|
|
||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| if (!getEspIdfSbomInstalledStatus()) | ||
| { | ||
| installEspIdfSbom(); | ||
| } | ||
| runEspIdfSbomCommand(); | ||
| try | ||
| { | ||
| refreshJob.join(); | ||
| } | ||
| catch (InterruptedException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return Status.OK_STATUS; | ||
| } | ||
| }; | ||
|
|
||
| projectDescription = projectDescriptionPathText.getText(); | ||
| saveOutputFileStatus = saveOutputToFileCheckBoxButton.getSelection(); | ||
| outputFilePath = outputFileText.getText(); | ||
| espIdfSbomJob.schedule(); | ||
|
|
||
| super.okPressed(); | ||
| } |
There was a problem hiding this comment.
The okPressed method is quite long and does a lot of things. Consider breaking it down into smaller methods for better readability and maintainability. For example, you could have separate methods for creating and scheduling the refreshJob and espIdfSbomJob.
| private void setDefaults() | ||
| { | ||
| ISelection selection = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getSelectionService() | ||
| .getSelection(); | ||
| if (selection instanceof IStructuredSelection) | ||
| { | ||
| Object element = ((IStructuredSelection) selection).getFirstElement(); | ||
|
|
||
| if (element instanceof IResource) | ||
| { | ||
| selectedProject = ((IResource) element).getProject(); | ||
| } | ||
| } | ||
| projectDescriptionPathText.setText(buildProjectDescriptionPath()); | ||
| if (!Files.isRegularFile(Paths.get(projectDescriptionPathText.getText()))) | ||
| { | ||
| setMessage(Messages.SbomCommandDialog_ProjectDescDoesntExistDefaultErrorMsg); | ||
| getButton(IDialogConstants.OK_ID).setEnabled(false); | ||
| } | ||
| outputFileText.setText(String.join(FileSystems.getDefault().getSeparator(), | ||
| Paths.get(selectedProject.getLocationURI()).toString(), DEFAULT_OUTPUT_FILE_NAME)); | ||
|
|
||
| saveOutputToFileCheckBoxButton.setSelection(false); | ||
| saveOutputToFileCheckBoxButton.notifyListeners(SWT.Selection, null); | ||
|
|
||
| outputFileText.addListener(SWT.Modify, e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInput())); | ||
| projectDescriptionPathText.addListener(SWT.Modify, | ||
| e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInput())); | ||
| } |
There was a problem hiding this comment.
The setDefaults method is well-implemented and sets the default values for the dialog components appropriately. However, it's not clear what happens when the project description file does not exist. The error message Messages.SbomCommandDialog_ProjectDescDoesntExistDefaultErrorMsg is set, but the method continues to execute. If the project description file not existing is an error condition, consider throwing an exception or returning immediately after setting the error message.
| private void runEspIdfSbomCommand() | ||
| { | ||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| List<String> arguments = new ArrayList<>(); | ||
| final String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(); | ||
| arguments.add(pythonEnvPath); | ||
| arguments.add("-m"); //$NON-NLS-1$ | ||
| arguments.add(ESP_IDF_SBOM_COMMAND_NAME); | ||
| arguments.add("create"); //$NON-NLS-1$ | ||
| arguments.add(projectDescription); | ||
| if (saveOutputFileStatus) | ||
| { | ||
| arguments.add("--output-file"); //$NON-NLS-1$ | ||
| arguments.add(outputFilePath); | ||
| } | ||
| String cmdOutput = runCommand(arguments, null, environment); | ||
| cmdOutput = cmdOutput.isEmpty() && saveOutputFileStatus | ||
| ? String.format(Messages.SbomCommandDialog_ConsoleRedirectedOutputFormatString, outputFilePath) | ||
| : cmdOutput; | ||
| console.getConsole().addPatternMatchListener(getPatternMatchListener()); | ||
| console.println(cmdOutput); | ||
|
|
||
| } |
There was a problem hiding this comment.
The runEspIdfSbomCommand method is well-implemented and handles the command execution properly. However, it's not clear what happens when the command output is empty and saveOutputFileStatus is true. The message String.format(Messages.SbomCommandDialog_ConsoleRedirectedOutputFormatString, outputFilePath) is printed to the console, but the method continues to execute. If the command output being empty is an error condition, consider throwing an exception or returning immediately after printing the message.
| boolean validateStatus = true; | ||
| java.nio.file.Path projectDescriptionPath = null; | ||
| try | ||
| { | ||
| projectDescriptionPath = Paths.get(projectDescriptionPathText.getText()); | ||
| } | ||
| catch (InvalidPathException e) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_InvalidProjectDescPathErrorMsg); | ||
| } | ||
| if (projectDescriptionPath != null && !Files.isRegularFile(projectDescriptionPath)) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_ProjectDescDoesntExistsErrorMsg); | ||
| } | ||
|
|
||
| java.nio.file.Path outputFilePath = null; | ||
| try | ||
| { | ||
| outputFilePath = Paths.get(outputFileText.getText()); | ||
| } | ||
| catch (InvalidPathException e) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_IvalidOutputFilePathErrorMsg); | ||
| } | ||
| if (outputFilePath != null && saveOutputToFileCheckBoxButton.getSelection() | ||
| && checkIfFileIsNotWritable(outputFilePath)) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.SbomCommandDialog_OutputFileNotWritabbleErrorMsg); | ||
| } | ||
|
|
||
| if (validateStatus) | ||
| { | ||
| setErrorMessage(null); | ||
| } | ||
| return validateStatus; |
There was a problem hiding this comment.
The validateInput method is well-implemented and validates the input properly. However, it's not clear what happens when the project description path or output file path is invalid or when the output file is not writable. The appropriate error message is set, but the method continues to execute and returns false. If any of these conditions are error conditions, consider throwing an exception or returning immediately after setting the error message.
kolipakakondal
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (3)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SbomCommandDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java
Additional comments (Suppressed): 1
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1)
- 43-60: The new messages added for the SBOM Tool feature seem to cover all necessary user interactions and potential error scenarios. The messages are clear and informative. Ensure that these new messages are correctly referenced in the code where they are needed.
| protected String runCommand(List<String> arguments, Path workDir, Map<String, String> env) | ||
| { | ||
| String exportCmdOp = StringUtil.EMPTY; | ||
| ProcessBuilderFactory processRunner = new ProcessBuilderFactory(); | ||
| try | ||
| { | ||
| IStatus status = processRunner.runInBackground(arguments, workDir, env); | ||
| if (status == null) | ||
| { | ||
| IStatus errorStatus = IDFCorePlugin.errorStatus(Messages.SbomCommandDialog_StatusCantBeNullErrorMsg, | ||
| null); | ||
| Logger.log(IDFCorePlugin.getPlugin(), errorStatus); | ||
| return errorStatus.getMessage(); | ||
| } | ||
|
|
||
| exportCmdOp = status.getMessage(); | ||
| Logger.log(exportCmdOp); | ||
| } | ||
| catch (IOException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return exportCmdOp; | ||
| } |
There was a problem hiding this comment.
The runCommand method does not handle the case where the command execution fails. It only logs the error and returns the error message. It would be better to throw an exception to allow the caller to handle the error.
- String exportCmdOp = StringUtil.EMPTY;
- ProcessBuilderFactory processRunner = new ProcessBuilderFactory();
- try
- {
- IStatus status = processRunner.runInBackground(arguments, workDir, env);
- if (status == null)
- {
- IStatus errorStatus = IDFCorePlugin.errorStatus(Messages.SbomCommandDialog_StatusCantBeNullErrorMsg,
- null);
- Logger.log(IDFCorePlugin.getPlugin(), errorStatus);
- return errorStatus.getMessage();
- }
-
- exportCmdOp = status.getMessage();
- Logger.log(exportCmdOp);
- }
- catch (IOException e)
- {
- Logger.log(e);
- }
- return exportCmdOp;
+ try {
+ ProcessBuilderFactory processRunner = new ProcessBuilderFactory();
+ IStatus status = processRunner.runInBackground(arguments, workDir, env);
+ if (status == null) {
+ throw new IOException(Messages.SbomCommandDialog_StatusCantBeNullErrorMsg);
+ }
+ String exportCmdOp = status.getMessage();
+ Logger.log(exportCmdOp);
+ return exportCmdOp;
+ } catch (IOException e) {
+ Logger.log(e);
+ throw e;
+ }| @Override | ||
| protected void okPressed() | ||
| { | ||
| console = new IDFConsole().getConsoleStream(Messages.IDFToolsHandler_ToolsManagerConsole, null, false); | ||
| Job refreshJob = new Job(Messages.SbomCommandDialog_RefreshProjectJob) | ||
| { | ||
|
|
||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| try | ||
| { | ||
| selectedProject.refreshLocal(IResource.DEPTH_INFINITE, null); | ||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return Status.OK_STATUS; | ||
| } | ||
| }; | ||
| Job espIdfSbomJob = new Job(Messages.SbomCommandDialog_EspIdfSbomJobName) | ||
| { | ||
|
|
||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| if (!getEspIdfSbomInstalledStatus()) | ||
| { | ||
| installEspIdfSbom(); | ||
| } | ||
| runEspIdfSbomCommand(); | ||
| try | ||
| { | ||
| refreshJob.join(); | ||
| } | ||
| catch (InterruptedException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return Status.OK_STATUS; | ||
| } | ||
| }; | ||
|
|
||
| projectDescription = projectDescriptionPathText.getText(); | ||
| saveOutputFileStatus = saveOutputToFileCheckBoxButton.getSelection(); | ||
| outputFilePath = outputFileText.getText(); | ||
| espIdfSbomJob.schedule(); | ||
|
|
||
| super.okPressed(); | ||
| } |
There was a problem hiding this comment.
The okPressed method is doing too much. It's creating jobs, scheduling them, and setting up the command to run. This could be broken down into smaller methods for better readability and maintainability.
| } | ||
|
|
||
| private boolean checkIfFileIsNotWritable(java.nio.file.Path pathToFile) | ||
| { | ||
| return Files.exists(pathToFile) && !Files.isWritable(pathToFile); | ||
| } |
There was a problem hiding this comment.
The method checkIfFileIsNotWritable returns true if the file exists and is not writable. This could be misleading as the method name suggests it only checks if the file is not writable. Consider renaming the method to fileExistsAndIsNotWritable or change the implementation to only check if the file is not writable.
| private boolean getEspIdfSbomInstalledStatus() | ||
| { | ||
| Map<String, String> environment = new HashMap<>(System.getenv()); | ||
| List<String> arguments = new ArrayList<>(); | ||
| final String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(); | ||
| arguments.add(pythonEnvPath); | ||
| arguments.add("-m"); //$NON-NLS-1$ | ||
| arguments.add("pip"); //$NON-NLS-1$ | ||
| arguments.add("list"); //$NON-NLS-1$ | ||
| String cmdOutput = runCommand(arguments, null, environment); | ||
| return cmdOutput.contains("esp-idf-sbom"); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
The getEspIdfSbomInstalledStatus method checks if the esp-idf-sbom is installed by running the pip list command and checking if the output contains esp-idf-sbom. This could lead to false positives if there is another package with esp-idf-sbom in its name. Consider using pip show esp-idf-sbom instead, which will only return information about the esp-idf-sbom package if it's installed.
|
@sigmaaa hi ! Tested: LGTM 👍 |
|
added new icon |
Description
Added command, that represents esp-idf-sbom command . To run the command right click on the project -> ESP-IDF: SBOM Tool
Fixes # (IEP-990)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test 1:
Running esp-idf-sbom command...command is activated -> output is printed to the consoleTest 2:
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
esp_idf_sbomcommand. Users can now select a project description file, specify an output file path, and execute the command all within the dialog box.