IEP-1301: Null Build error if you're building a project with out any esp-idf version #1062
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1)
27-27: Improve grammar and clarity of the error message.The new error message is a good addition, but it can be improved for better clarity and grammar.
Consider applying this change:
-IDFToolChainsMissingErrorMsg=Toolchains are missing please verify that idf tools are installed +IDFToolChainsMissingErrorMsg=Toolchains are missing. Please verify that ESP-IDF tools are installed correctly.This revision:
- Adds a period after "missing" to separate the two clauses.
- Capitalizes "ESP-IDF" for consistency with official naming.
- Adds "correctly" to emphasize proper installation.
These small changes will make the message more professional and potentially more helpful to users encountering this error.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)
283-288: LGTM: Improved error handling in getToolChainFile() method.The addition of the null check for
toolChainand the throwing of aCoreExceptionwith a specific error message greatly improves the error handling in this method. This change will help preventNullPointerExceptions and provide more meaningful error messages.One minor suggestion:
Consider extracting the error message to a constant in the
Messagesclass for easier maintenance and reuse. For example:public static final String TOOLCHAIN_MISSING_ERROR = "Toolchains are missing. Please check your project configuration.";Then use it in the
getToolChainFile()method:throw new CoreException(new Status(IStatus.ERROR, IDFCorePlugin.PLUGIN_ID, Messages.TOOLCHAIN_MISSING_ERROR));This approach would make it easier to update the error message in the future if needed.
398-399: Remove unnecessary empty lines.There are two empty lines at the end of the
buildPrechecks()method that don't serve any purpose. To maintain code cleanliness and consistency, it's recommended to remove these lines.Consider removing the empty lines at the end of the
buildPrechecks()method:- - return true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (3 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (2)
Line range hint
1-53: Summary: New error message variable added successfully.The addition of the
IDFToolChainsMissingErrorMsgvariable is consistent with the existing code structure and supports the PR's objective of addressing the error message for missing toolchains. The change is minimal and doesn't introduce any apparent issues.To further improve the PR:
- Ensure that the corresponding message text is added to the
messages.propertiesfile (as verified in the previous comment).- Consider adding a brief comment above the new variable to describe its purpose, maintaining consistency with any existing documentation practices in the file.
35-35: LGTM: New message variable added correctly.The new variable
IDFToolChainsMissingErrorMsghas been added correctly, following the existing pattern for message declarations in this class. This addition aligns with the PR objective to address the error message for missing toolchains.However, to ensure completeness:
Let's verify if the corresponding message has been added to the resource bundle:
This will help confirm that the message text has been properly defined in the resource bundle.
✅ Verification successful
Verification Successful: Message Key Present in Resource Bundle
The
IDFToolChainsMissingErrorMsgkey has been successfully added to themessages.propertiesfile with the appropriate message. This ensures that the new message variable inMessages.javais properly linked to its corresponding message in the resource bundle.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new message key exists in the properties file # Test: Search for the new message key in the properties file rg --type properties "IDFToolChainsMissingErrorMsg" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.propertiesLength of output: 170
Script:
#!/bin/bash # Description: Check if the new message key exists in the properties file # Test: Search for the new message key in the properties file rg "IDFToolChainsMissingErrorMsg" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.propertiesLength of output: 212
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1)
27-27: LGTM: Change aligns well with PR objectives.This addition of the error message for missing toolchains directly addresses the PR's objective of handling the error when toolchains are missing during the build process. The change is minimal, non-breaking, and consistent with the existing file structure, making it a suitable solution for the described issue.
To ensure this message key is being used correctly in the codebase, let's verify its usage:
This will help confirm that the new message is properly integrated into the error handling logic of the build system.
✅ Verification successful
Verification Successful: The error message key
IDFToolChainsMissingErrorMsgis correctly integrated into the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new error message key in the codebase. # Test: Search for the usage of 'IDFToolChainsMissingErrorMsg'. Expect: At least one occurrence in a Java file. rg --type java 'IDFToolChainsMissingErrorMsg'Length of output: 564
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1)
69-72: LGTM: Import statements added correctly.The new import statements for
IStatusandStatusare necessary for the changes in thegetToolChainFile()method. These additions are correct and don't introduce any issues.
|
@alirana01 hi ! The "Toolchains are missing please verify that idf tools are installed" message is duplicated. And I think the message offered by Kondal could be added to give user hint: |
|
@AndriiFilippov the error message that is duplicated is because of how the eclipse is propagating the exception messages back not something we can do. Further the error message suggestion was updated |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1)
398-399: Remove unnecessary empty lines.These extra empty lines create inconsistent spacing in the method.
- - return true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(3 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties
🔇 Additional comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)
69-72: LGTM: Required imports added for error handling.
The new imports are necessary for the enhanced error handling implementation.
283-288: LGTM: Proper error handling for null toolchain.
The implementation correctly handles the case when toolchain is null, throwing a CoreException with a descriptive error message. This addresses the core issue mentioned in the PR objectives.
Let's verify that the error message is properly defined:
✅ Verification successful
Error message is properly defined and externalized
The error message constant IDFToolChainsMissingErrorMsg is correctly:
- Defined in
Messages.javaas a public static field - Externalized in
messages.propertieswith a clear user-friendly message - Used appropriately in the error handling code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the error message constant is properly defined in Messages class
rg -A 1 "IDFToolChainsMissingErrorMsg" --type java
Length of output: 750
|
@alirana01 hi ! Tested under: LGTM, do see error message: Question: I like the error message of "ESP-IDF" menu if the toolchains are missing. Do you think we could have same for "build" button ? or at least similar error message ?
|
|
also this error message BUT if I go to "Preferences - > env" and delete toolchains from here: and then try to build project - you get this message: |
I would suggest this to be separate ticket and i dont see any issue with this error message. We cannot try to override all the internal messages and how they are displayed I think this can be part of the patch release |
|
@sigmaaa @kolipakakondal please, review |
Description
Fixed the error message in case the toolchains are missing
Fixes # (IEP-1301)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please follow steps in Jira ticket
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation