-
Notifications
You must be signed in to change notification settings - Fork 121
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-999: Tools Validation for required tools #802
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought of a better way for this while doing the self review. I think we can simply install these required tools ourselves as well instead of asking user to go and do it manually since we already have them listed in tools.json and we know these are required and are missing we can simply install them from their which is actually a good solution i think.
What do you think @kolipakakondal @sigmaaa
I have updated the added doc file to point this out I think it is sufficient for the current scenario so we are not forcing the user to install the IDF based version and rather asking them to either install or configure based on the paths in the system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
Hi @alirana01 That makes sense. We can install the missing tools. Internally, maintain some flags for these tools and choose them by default with the recommended one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @AndriiFilippov Please verify this |
@alirana01 hi! Tested on: MacOS: Similar behave on Windows 10: |
Hi @ali Can we merge this? |
WalkthroughThis pull request involves a significant refactoring of the codebase, primarily focusing on the Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 5
Files ignored due to filter (8)
- bundles/com.espressif.idf.core/lib/commons-compress-1.21-javadoc.jar
- bundles/com.espressif.idf.core/lib/commons-compress-1.21.jar
- bundles/com.espressif.idf.core/lib/xz-1.9-javadoc.jar
- bundles/com.espressif.idf.core/lib/xz-1.9-sources.jar
- bundles/com.espressif.idf.core/lib/xz-1.9.jar
- docs/images/ToolsValidation/EnvSettings.png
- docs/images/ToolsValidation/PathVarEdit.png
- docs/images/ToolsValidation/PathVarEdited.png
Files selected for processing (28)
- bundles/com.espressif.idf.core/.classpath (1 hunks)
- bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (2 hunks)
- bundles/com.espressif.idf.core/build.properties (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/DefaultSystemWrapper.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/SystemWrapper.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java (2 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java (6 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java (2 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (6 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java (3 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java (2 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java (2 hunks)
- bundles/com.espressif.idf.ui/.classpath (1 hunks)
- bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF (1 hunks)
- bundles/com.espressif.idf.ui/build.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageToolsInstallationHandler.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationHandler.java (16 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/ToolsManagerWizard.java (4 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/ToolsManagerWizardDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/InstallEspIdfPage.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/InstallPreRquisitePage.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/ManageToolsInstallationWizardPage.java (8 hunks)
- docs/MissingToolsManualPathUpdateGuide.md (1 hunks)
Files skipped from review due to trivial changes (15)
- bundles/com.espressif.idf.core/.classpath
- bundles/com.espressif.idf.core/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.core/build.properties
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/DefaultSystemWrapper.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsInstallationWizardConstants.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/JsonKey.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsPlatformMapping.java
- bundles/com.espressif.idf.ui/.classpath
- bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.ui/build.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ManageToolsInstallationHandler.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/InstallPreRquisitePage.java
- docs/MissingToolsManualPathUpdateGuide.md
Additional comments (Suppressed): 39
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsSystemWrapper.java (1)
- 15-36: The
ToolsSystemWrapper
class is well implemented. It correctly overrides thegetPathEnv()
andgetEnvExecutables()
methods from theSystemWrapper
interface. However, it's important to ensure that thepath
variable passed to the constructor is always a valid path string.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/IToolsJsonKeys.java (2)
1-4: The package name has been changed from
com.espressif.idf.ui.tools
tocom.espressif.idf.core.tools
. Ensure that all references to this interface in the codebase have been updated accordingly.22-29: A new key
VERSION_REGEX
has been added. Make sure it is used correctly throughout the codebase and the corresponding value in the JSON file follows a valid regex pattern.bundles/com.espressif.idf.core/src/com/espressif/idf/core/SystemWrapper.java (2)
9-10: The addition of
PATH
andPATHEXT
constants in the interface is a good practice as it avoids hardcoding these values in multiple places. However, ensure that these new constants are used wherever applicable in the codebase.12-13: No issues found with the existing method signatures. They remain unchanged which ensures backward compatibility.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionDetailsVO.java (3)
71-73:
75-86: The
equals()
method has been overridden to include new fields. Ensure that all usages of this method in the codebase are still valid with these changes.89-93: The
hashCode()
method has been overridden to include new fields. Ensure that all usages of this method in the codebase are still valid with these changes.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/ToolsVO.java (3)
5-18: The class has been moved from the
ui
project to thecore
project. Ensure that all references to this class in the codebase have been updated to reflect the new package.48-56: A new field
versionRegex
has been added. This field is used to store the regular expression for matching the version of a tool. The addition of this field should not cause any issues as long as it is properly initialized and used.217-259: New getter and setter methods for the
versionRegex
field have been added. Theequals()
andhashCode()
methods have been overridden to include the newversionRegex
field. These changes are correct and should not cause any issues.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/vo/VersionsVO.java (3)
74-76:
78-91: The
equals()
method has been correctly overridden to include the new fieldavailablePath
and the existing fields. This ensures that twoVersionsVO
objects are considered equal if all their fields are equal.93-97: The
hashCode()
method has been correctly overridden to include the new fieldavailablePath
and the existing fields. This ensures that two equalVersionsVO
objects will always produce the same hash code, which is a requirement for objects that overrideequals()
.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java (3)
54-58: Ensure that the file
IDFUtil.getIDFToolsJsonFileForInstallation()
exists and is accessible before trying to read from it. Add error handling for file not found or inaccessible scenarios.77-83: Check if the keys used in
toolsJsonObject.get()
calls exist in the JSON object before accessing them to avoid potentialNullPointerException
.89-93: This logic correctly filters out the required tools based on the
REQUIRED_TOOLS
list and adds them torequiredToolsList
. Good job!bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/InstallEspIdfPage.java (1)
- 33-44: The import statements have been rearranged, and no new imports have been added. Ensure that the code adheres to your project's import statement ordering conventions.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/ToolsManagerWizardDialog.java (1)
- 19-25: The import statement for
IToolsInstallationWizardConstants
has been added. Ensure that the constants from this class are used appropriately in the code.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java (4)
2-8: The package name has been changed from
com.espressif.idf.ui.tools
tocom.espressif.idf.core.tools.util
. Ensure that all references to this class have been updated accordingly.27-41: The import statements have been updated to include new classes and remove unused ones. Make sure these changes do not break any existing functionality.
227-232: No significant issues found in this hunk.
307-331: The new method
findAbsoluteToolPath()
has been added. It seems to be correctly implemented, but ensure that it is being used properly throughout the codebase and its return value is always checked for null before usage.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/ToolsManagerWizard.java (4)
19-23: The import statement for
com.espressif.idf.core.tools.ToolsJsonParser
has been added. Ensure that theToolsJsonParser
class is available in thecom.espressif.idf.core.tools
package.43-43: A new instance variable
toolsJsonParser
of typeToolsJsonParser
has been introduced. This seems to be a part of the refactoring effort mentioned in the PR summary, where some classes have been moved from theui
project to thecore
project.53-53: The
toolsJsonParser
instance variable is being initialized in the constructor. This is a good practice as it ensures that thetoolsJsonParser
is not null when it's used later in the code.67-67: The
ManageToolsInstallationWizardPage
constructor now takes an additional argumenttoolsJsonParser
. Make sure that the constructor ofManageToolsInstallationWizardPage
has been updated to accept this new parameter and uses it appropriately.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationHandler.java (7)
42-54: The import statements have been rearranged and some classes have been moved from the
ui
project to thecore
project. Ensure that these changes do not break any dependencies in other parts of the codebase.78-82: A new field
requireToolsVOs
has been added to store the required tools. Make sure this field is properly initialized before it's used anywhere else in the class.176-182: The method signature for
operationToPerform()
has been updated to include a new parameterrequiredToolsVOs
. Ensure all calls to this method throughout the codebase have been updated to match the new signature.559-565: This hunk looks good, no issues found.
577-657: A new method
validateToolsInstall()
has been added. This method validates the installation of required tools after they are installed. If any tool is missing, it adds a message to the log queue and displays a link to a document for manual path configuration on the wizard page. This is a good addition as it ensures that all required tools are properly installed.851-864: This hunk looks good, no issues found.
866-868: The condition for copying OpenOCD rules has been moved to a new line. This change doesn't affect the functionality of the code.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/ManageToolsInstallationWizardPage.java (5)
117-125: The constructor of
ManageToolsInstallationWizardPage
has been modified to accept an additional parametertoolsJsonParser
. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.276-279: A new Link widget
linkForDoc
is added to the UI. This link is expected to be used for launching a document in the default browser when clicked. However, it's not clear what text and URL this link will display. Make sure to set appropriate text and hyperlink for this widget before making it visible.854-859: The key used to retrieve the
VersionsVO
object from the item data has been removed. If this key is no longer needed, this change should be fine. However, if other parts of the code rely on this key, this could cause issues. Please verify this change.923-925: The method
operationToPerform()
oftoolsInstallationHandler
now accepts an additional parametertoolsJsonParser.getRequiredToolsList()
. Ensure that the method implementation has been updated to handle this new argument correctly.952-960: Getter and setter methods for
linkForDoc
are introduced. While these methods are syntactically correct, it's important to ensure they are used appropriately in the rest of the codebase.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolsJsonParser.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/util/ToolsUtility.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationHandler.java
Show resolved
Hide resolved
@AndriiFilippov can you please verify on mac os the dfu-util is missing in tools.json for macos that is to be installed by the user manually if its not installed but there was one issue with ninja and I have fixed that please take a look again for macos and windows as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/SystemExecutableFinder.java (3 hunks)
Additional comments (Suppressed): 2
bundles/com.espressif.idf.core/src/com/espressif/idf/core/SystemExecutableFinder.java (2)
8-16: The import for
java.io.IOException
has been added, which is necessary for the new exception handling in lines 86-93. The import forcom.espressif.idf.core.logging.Logger
has also been added to log any exceptions that occur.114-119: No significant changes have been made in this hunk. The code checks if the file exists and is not a directory, and if it has executable permissions.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/SystemExecutableFinder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- bundles/com.espressif.idf.core/pom.xml (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.core/pom.xml
@alirana01 hi ! MacOS - LGTM 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationHandler.java (16 hunks)
Additional comments (Suppressed): 11
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationHandler.java (11)
30-33: The import
org.eclipse.core.runtime.IPath
is added but not used in the new hunks. Please ensure it's necessary or remove it to avoid unnecessary imports.42-54: Several classes have been moved from the
ui
project to thecore
project, includingIToolsInstallationWizardConstants
,ToolsJsonParser
,ToolsPlatformMapping
,ToolsVO
,VersionDetailsVO
,VersionsVO
, andToolsUtility
. Ensure that all references to these classes throughout the codebase have been updated to match the new locations.78-82: A new field
requireToolsVOs
is introduced. Make sure it's properly initialized before use and consider making it final if it doesn't need to be changed after being set.173-182: The method signature of
operationToPerform
has been changed to include a new parameterrequiredToolsVOs
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.383-397: No significant issues found in this hunk.
413-419: No significant issues found in this hunk.
563-565: No significant issues found in this hunk.
577-660: This new method
validateToolsInstall()
checks for missing tools after installation and logs any missing ones. It also updates the UI with a link to documentation for manual configuration if any tools are missing. This is a good addition for better user experience and error handling.773-834: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [681-834]
The method
replacePathVariable
has been significantly refactored. The new implementation seems more robust as it handles different cases for Windows and other platforms separately. However, please verify that the new implementation works as expected across all supported platforms.
854-867: No significant issues found in this hunk.
869-871: No significant issues found in this hunk.
@AndriiFilippov I have pushed the fix for the windows please test it when done |
@alirana01 hi ! Windows 10 - LGTM 👍 |
Please make sure that cmake is installed on mac os and is configured as suggested in the readme file |
@alirana01 hi ! MacOS - LGTM 👍 |
Description
Some tools are required by the IDF and for MAC and Linux they are marked as not installed or not required in the tools.json. We have created a static list for the tools at the moment that are a prereq for the IDF and are trying to validate them after the tools installation. In case these tools are not found on the PATH the user is shown a message and a link to our document that can be used to configure the IDE path.
Also did some refactoring so some classes were moved out of the ui project to the core project
Fixes # (IEP-999)
Type of change
Please delete options that are not relevant.
How has this been tested?
On MacOS try to run install only recommended tools via the installation wizard. If the required tools are not found on the path after installation. The user can click on the link to go to a document on github and configure the path manually.
Test Configuration:
Checklist
Summary by CodeRabbit
com.espressif.idf.ui.tools
package tocom.espressif.idf.core.tools
package for better organization and separation of concerns.ToolsInstallationHandler
.commons-io
dependency from 2.10.0 to 2.13.0 for enhanced functionality and security.