Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom

IEP-1041: Parse Compile Commands File exception when using multiple esp-idf versions #835

Merged
merged 1 commit into from
Nov 14, 2023
Merged

IEP-1041: Parse Compile Commands File exception when using multiple esp-idf versions #835

merged 1 commit into from
Nov 14, 2023

Conversation

sigmaaa
Copy link
Collaborator

@sigmaaa sigmaaa commented Oct 17, 2023 •
edited
Loading

Description

added one additional check which ignores the issue when switching esp-idf versions

Fixes # (IEP-1041)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested?

Test 1:

  • downgrade esp-idf version from the IDE -> try rebuilding project without cleaning build folder -> expected no parse compile commands error

Test Configuration:

  • ESP-IDF Version:
  • OS (Windows,Linux and macOS):

Dependent components impacted by this PR:

  • Build

Checklist

  • PR Self Reviewed
  • Applied Code formatting
  • Added Documentation
  • Added Unit Test
  • Verified on all platforms - Windows,Linux and macOS

Summary by CodeRabbit

  • Bug Fix: Improved the reliability of the build process in the IDF (IoT Development Framework). The software now checks if a required source file is present before attempting to build. If the file is not found, the build process is halted gracefully, preventing potential errors or crashes. This change enhances the stability and predictability of the build process.

@coderabbitai
Copy link

coderabbitai bot commented Oct 17, 2023 •
edited
Loading

Walkthrough

The introduced change adds a condition in the IDFBuildConfiguration.java file to handle the scenario where a source file is not found. This adds a new exit point to the function, returning null when the source file is not found.

Changes

File Summary
.../idf/core/build/IDFBuildConfiguration.java Introduced a condition to check if the startIndex is -1, indicating that the source file was not found. If not found, the function returns null, adding a new exit point.

🐇

In the land of code, where logic intertwines,

A rabbit hops, leaving bugs behind.

With each leap, a problem's solved,

In the world of code, continuously evolved. 🎉

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
  • Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
    resume reviews, enter @coderabbitai resume as any issue or review comment.

@sigmaaa sigmaaa changed the title WIP: Parse Compile Commands File exception when using multiple esp-idf versions WIP: IEP-1041: Parse Compile Commands File exception when using multiple esp-idf versions Oct 17, 2023
coderabbitai[bot]
coderabbitai bot reviewed Oct 17, 2023
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between 0267c08 and 10356a1.
Files selected for processing (1)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1 hunks)
Additional comments (Suppressed): 2
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)

  • 1248-1251: The new condition checks if startIndex is -1 and returns null if true. This introduces a new exit point in the function. Ensure that all calls to this function can handle a null return value and that this does not introduce null pointer exceptions elsewhere in the code.

  • 1252-1252: In the old hunk, if startIndex was -1, substring would throw an IndexOutOfBoundsException. The new hunk fixes this issue by adding a check for startIndex == -1 before calling substring. This is a good fix for preventing potential exceptions.

@sigmaaa sigmaaa changed the title WIP: IEP-1041: Parse Compile Commands File exception when using multiple esp-idf versions IEP-1041: Parse Compile Commands File exception when using multiple esp-idf versions Nov 7, 2023
@sigmaaa sigmaaa self-assigned this Nov 7, 2023
kolipakakondal
kolipakakondal reviewed Nov 7, 2023
View reviewed changes
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java
if (startIndex == -1) // source file still not found means it was part of another esp-idf
{
return null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

That meanspathtolookfor = getIdfToolsPath(); was not returning the correct IDF_PATH, I wonder why IDF_PATH is not configured correctly and what are the other issues it might cause!

If user switched to a different esp-idf version then IDF_PATH would have modified accordingly.

Copy link
Collaborator Author

@sigmaaa sigmaaa Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @kolipakakondal, the IDF_PATH from getIdfToolsPath() is correct, the issue happens because sourceFile is pointing to the different esp-idf. I think the sourceFile is provided based on the files from the build folder. This issue happens when we build the project with the different esp-idf version, then switch esp-idf and trying to build project again without cleaning build folder

kolipakakondal
kolipakakondal approved these changes Nov 9, 2023
View reviewed changes
Copy link
Collaborator

@kolipakakondal kolipakakondal left a comment

Choose a reason for hiding this comment

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

LGTM

@kolipakakondal kolipakakondal added the bug Something isn't working label Nov 9, 2023
@kolipakakondal kolipakakondal added this to the v2.12.0 milestone Nov 9, 2023
@kolipakakondal
Copy link
Collaborator

kolipakakondal commented Nov 9, 2023 •
edited
Loading

Hi @AndriiFilippov Please check this

Dependent areas need to be tested:

  • Build
  • Indexer
  • Switch b/w different esp-idf versions
  • Custom build folder with in the project and outside the project

@AndriiFilippov
Copy link
Collaborator

@sigmaaa hi !

Tested:
OS - Windows 10 / MacOS
ESP-IDF: v5.1.1 -> 5.0 / master -> 5.1.1

Test:
v5.1.1 -> build -> downgrade to v5.0(PATH to v5.0 is longer than to v5.1.1) -> build (without cleaning build folder) -> no parse compile commands error.

Repeat this one with custom build folder. LGTM 👍

@kolipakakondal kolipakakondal merged commit 822cebb into master Nov 14, 2023
7 checks passed
@kolipakakondal kolipakakondal deleted the IEP-1041 branch November 14, 2023 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@kolipakakondal kolipakakondal kolipakakondal approved these changes

@alirana01 alirana01 Awaiting requested review from alirana01

@AndriiFilippov AndriiFilippov Awaiting requested review from AndriiFilippov

Assignees

@sigmaaa sigmaaa

Labels
bug Something isn't working
Projects
None yet
Milestone
v2.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@sigmaaa @kolipakakondal @AndriiFilippov