IEP-1374 GH #1088: Launch in run mode does a redundant re-building even after build was triggered before that . #1107
Conversation
WalkthroughThe pull request introduces significant simplifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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 (
|
There was a problem hiding this comment.
LGTM. @AndriiFilippov This needs in-depth testing to ensure we don't miss any use cases.
|
@sigmaaa hi ! Tested under: Issue still persist. |
![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
🧹 Nitpick comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1)
336-338: Consider implementing build caching to prevent redundant rebuilds.Given the feedback about persisting rebuild issues during flash operations, consider these optimizations:
- Implement build artifact caching
- Add build timestamp tracking
- Compare build artifacts before triggering rebuilds
This would help prevent unnecessary rebuilds while ensuring build consistency. Would you like me to provide a detailed implementation plan for these optimizations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)
499-502: LGTM! Well-implemented ccache configuration.The ccache configuration is properly implemented with:
- Correct environment variable check
- Proper default value handling ("0")
- Clear CMake variable setting
336-338: Verify the impact of unconditional CMake command execution.The removal of modification checks simplifies the build process, but please verify that this doesn't skip necessary CMake reconfigurations in scenarios like:
- Changes to CMake cache variables
- Updates to toolchain files
- Changes to sdkconfig
✅ Verification successful
Unconditional CMake command execution is safe and beneficial
The codebase already has proper mechanisms to handle configuration changes:
- Changes to sdkconfig are tracked through the configuration system
- CMake cache variables are managed via CMakeCache.txt
- The build system automatically handles toolchain updates
The removal of modification checks simplifies the build process without compromising functionality, as the system already ensures proper reconfiguration when needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for CMake-related files that might require reconfiguration echo "Searching for CMake-related files that might need reconfiguration..." fd -e cmake -e sdkconfigLength of output: 173
Script:
#!/bin/bash # Search for CMake-related configuration handling echo "Searching for CMake configuration patterns..." ast-grep --pattern 'runCmakeCommand($$$)' echo -e "\nSearching for build configuration handling..." rg "CMakeCache|sdkconfig" -A 3 echo -e "\nSearching for environment setup..." ast-grep --pattern 'getEnvironment($$$)'Length of output: 24108
Hi @AndriiFilippov, @kolipakakondal , @alirana01 I had a call with Andrew and discovered that the issue he encountered on Ubuntu was caused by the build running with To address this, I’ve fixed the use case in the latest commit. Now, the |
|
@sigmaaa hi ! Tested under: cleaning of build directories behaves as expected ✅ |
Description
In this PR, I have removed the cmakeListsModified field as it is no longer necessary, given that we do not utilize the default C/C++ indexer. The purpose of this field was to determine whether the CMake command should be executed. However, in scenarios such as opening sdkconfig, it is essential to run the CMake command to avoid redundancy, as idf.py flash would otherwise perform this step, resulting in an unnecessary rebuild of the project.
Additionally, I have removed all code related to this field and also deleted the cleanBuildDirectory(buildDir) method. This method is redundant because the build folder is now cleaned automatically when switching the target.
Fixes # (IEP-1374)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
Refactor
Chores