fix: Update lsp dependencies #1190
Conversation
WalkthroughThis update modernizes the Espressif Eclipse plugin's build and runtime environment by upgrading Java and Eclipse dependencies to newer versions, refactoring LSP/Clangd configuration integration, and improving Maven and classpath management. Java 21 replaces Java 17 across product, build, and CI configurations. The LSP/Clangd preference architecture is reworked, removing older configuration classes and XML components and introducing new metadata-based defaults. Several Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Eclipse IDE
participant IDFBuildConfigurationProvider
participant IDFBuildConfiguration
participant LspService
participant ClangdMetadataDefaults
User->>Eclipse IDE: Initiates build/configuration or LSP action
Eclipse IDE->>IDFBuildConfigurationProvider: Request build configuration
IDFBuildConfigurationProvider->>IDFBuildConfiguration: Create with launch target/toolchain
IDFBuildConfiguration->>IDFBuildConfigurationProvider: Return configuration
Eclipse IDE->>LspService: Update LSP/Clangd preferences
LspService->>ClangdMetadataDefaults: Retrieve metadata defaults
ClangdMetadataDefaults-->>LspService: Provide metadata (clangdPath, queryDriver, etc.)
LspService-->>Eclipse IDE: Preferences updated
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
|
Hi @sigmaaa I believe this is no more required right? I will close this. |
Hi @kolipakakondal, I moved my changes to this branch, so we can continue using this branch |
![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 (3)
.github/workflows/ci.yml (1)
41-46: Updated CI workflow to use JDK 21The CI workflow has been updated to use JDK 21 instead of JDK 17, and the GitHub Actions setup-java has been upgraded from v3 to v4.
Remove the trailing spaces on line 46 to fix the linting error.
- distribution: 'temurin' - + distribution: 'temurin'🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 46-46: trailing spaces
(trailing-spaces)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java (1)
71-83: Compile commands directory update mechanism rewrittenThe implementation has been updated to use the predefined metadata structure, but the core logic of updating the compile commands directory remains the same. There's a minor typo in the variable name
newCompuileCommandsDirString(should be "compile" not "compuile").Fix the variable naming typo:
- String newCompuileCommandsDirString = compileCommandsDirString + buildDir; + String newCompileCommandsDirString = compileCommandsDirString + buildDir; - String updatedOptions = existingOptions.contains(compileCommandsDirString) - ? existingOptions.replaceAll(compileCommandsDirString + ".+", //$NON-NLS-1$ - Matcher.quoteReplacement(newCompuileCommandsDirString)) - : newCompuileCommandsDirString; + String updatedOptions = existingOptions.contains(compileCommandsDirString) + ? existingOptions.replaceAll(compileCommandsDirString + ".+", //$NON-NLS-1$ + Matcher.quoteReplacement(newCompileCommandsDirString)) + : newCompileCommandsDirString;bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdMetadataDefaults.java (1)
1-53: Well-structured new class for Clangd metadata defaultsThis new class properly implements the ClangdMetadata interface and provides ESP-IDF specific default values for Clangd configuration. The implementation follows good practices:
- It appropriately filters predefined keys to override
- Uses utilities from IDFUtil to find the Clangd executable
- Leverages the ESP toolchain manager to find the correct compiler path for the query driver
- Handles potential null values safely with Optional
The
wrapWithCustomDefaultValuehelper method is a clean way to preserve the original metadata while changing just the default value.One suggestion to consider is adding more documentation comments, especially for the public methods, to explain the purpose and behavior of this class more clearly.
@Component(property = { "service.ranking:Integer=0" }) +/** + * Provides ESP-IDF specific default values for Clangd configuration. + * This class overrides the standard Clangd metadata defaults with values + * that are appropriate for ESP-IDF development. + */ public class IDFClangdMetadataDefaults extends ConfigurationMetadataBase implements ClangdMetadata { + /** + * Defines the preferences for Clangd configuration with ESP-IDF specific default values. + * + * @return A list of preference metadata with custom default values for ESP-IDF. + */ @Override protected List<PreferenceMetadata<?>> definePreferences()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/ci.yml(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfigurationProvider.java(5 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java(1 hunks)bundles/com.espressif.idf.lsp/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdConfigurationAccess.xml(0 hunks)bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdOptionsDefaults.xml(1 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdConfigurationAccess.java(0 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdMetadataDefaults.java(1 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdOptionsDefaults.java(0 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdPreferredOptions.java(0 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/CMakeBuildTab2.java(6 hunks)bundles/pom.xml(1 hunks)releng/com.espressif.idf.configuration/pom.xml(2 hunks)releng/com.espressif.idf.product/idf.product(2 hunks)releng/com.espressif.idf.product/pom.xml(1 hunks)releng/com.espressif.idf.target/com.espressif.idf.target.target(5 hunks)releng/com.espressif.idf.update/category.xml(0 hunks)
💤 Files with no reviewable changes (5)
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdOptionsDefaults.java
- releng/com.espressif.idf.update/category.xml
- bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdConfigurationAccess.xml
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdConfigurationAccess.java
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdPreferredOptions.java
🚧 Files skipped from review as they are similar to previous changes (1)
- releng/com.espressif.idf.target/com.espressif.idf.target.target
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/ci.yml
[error] 46-46: trailing spaces
(trailing-spaces)
🔇 Additional comments (22)
bundles/pom.xml (1)
49-49: Java execution environment updated to JDK 21The execution environment has been updated from JDK 17 to JDK 21 to align with the LSP dependencies update. This is a significant version upgrade that requires verification across development environments.
Please verify that all developers have updated their development environments to support JDK 21. This change will require adjustments to development setups and CI environments.
releng/com.espressif.idf.product/pom.xml (1)
117-117: Java execution environment updated to JDK 21The execution environment has been updated from JDK 17 to JDK 21, consistent with the changes in other configuration files.
releng/com.espressif.idf.product/idf.product (2)
17-22: Updated Java version requirement to 21 in launcher argumentsThe required Java version has been updated from 17 to 21 in the launcher arguments for all supported platforms (Linux, macOS, Windows).
45-48: Updated Java VM configuration to JDK 21Java VM configurations for Linux, macOS, and Windows have been updated to use JavaSE-21 instead of JavaSE-17.
Note that the Solaris configuration on line 47 still references JavaSE-11. Please verify if this is intentional or if it should also be updated to JavaSE-21 for consistency.
.github/workflows/ci.yml (2)
48-48: SpotBugs analysis removed from Maven build commandThe Maven build command has been simplified to remove the SpotBugs analysis.
Please confirm if removing the SpotBugs analysis was intentional. SpotBugs is a valuable static analysis tool that helps identify potential bugs. If it's causing compatibility issues with Java 21, consider addressing those issues rather than removing the analysis.
65-69: Updated macOS build job to use JDK 21The macOS build job has been updated to use JDK 21 and the GitHub Actions setup-java has been upgraded from v3 to v4.
bundles/com.espressif.idf.lsp/META-INF/MANIFEST.MF (1)
19-19: Service component configuration updatedThe
Service-Componententry now excludesIDFClangdConfigurationAccess.xml, which aligns with the broader restructuring of Clangd-related components in this PR. This change is consistent with moving away from the older configuration access pattern as part of the LSP dependencies update.releng/com.espressif.idf.configuration/pom.xml (2)
10-10: Tycho version updated from 4.0.6 to 4.0.9Updating the Tycho version brings in the latest improvements and fixes for the build system.
41-41:✅ Verification successful
JDK requirement upgraded from 17 to 21
The execution environment has been updated to Java 21, which is a significant change requiring developers and users to have JDK 21 installed. Ensure this change is clearly communicated in the documentation and that all CI/CD pipelines are updated accordingly.
Run this verification script to confirm if other POM files have been consistently updated with this JDK change:
🏁 Script executed:
#!/bin/bash # Check if all POM files have been updated to JDK 21 echo "Checking POM files for Java version references:" grep -r "org.eclipse.justj.openjdk.hotspot.jre.full" --include="pom.xml" . echo "Checking other files for Java version references:" grep -r "requiredJavaVersion" --include="*.product" .Length of output: 1340
JDK Upgrade Verification: Consistent Update to JDK 21
All relevant POM and product files have been updated to reference JDK 21:
- bundles/pom.xml:
<executionEnvironment>org.eclipse.justj.openjdk.hotspot.jre.full-21</executionEnvironment>- releng/com.espressif.idf.configuration/pom.xml:
<executionEnvironment>org.eclipse.justj.openjdk.hotspot.jre.full-21</executionEnvironment>- releng/com.espressif.idf.product/pom.xml:
<executionEnvironment>org.eclipse.justj.openjdk.hotspot.jre.full-21</executionEnvironment>- releng/com.espressif.idf.product/idf.product: Contains settings for
-Dosgi.requiredJavaVersion=21across Linux, Mac, and Windows configurationsPlease ensure that the documentation and CI/CD pipelines are updated accordingly to reflect the upgraded JDK requirement.
bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdOptionsDefaults.xml (1)
2-7:✅ Verification successful
Component definition aligned with upstream Eclipse CDT structure
The component has been significantly refactored:
- Component name changed to use Eclipse CDT namespace
- Service ranking changed from 100 to 0
- Service interface updated to use
ClangdMetadatainstead ofClangdOptionsDefaults- Implementation uses new
IDFClangdMetadataDefaultsclassThese changes align with the upstream Eclipse CDT architectural changes for LSP support.
Verify that the new
IDFClangdMetadataDefaultsclass properly implements theClangdMetadatainterface:
🏁 Script executed:
#!/bin/bash # Check the implementation of IDFClangdMetadataDefaults echo "Checking implementation of IDFClangdMetadataDefaults:" find . -name "IDFClangdMetadataDefaults.java" | xargs catLength of output: 2233
Component Definition and Interface Implementation Verified
The updates in the component definition have been confirmed:
- The component now uses the Eclipse CDT namespace (
org.eclipse.cdt.lsp.clangd.internal.config.ClangdMetadataDefaults).- The service ranking has been updated from 100 to 0.
- The service interface is correctly set to
ClangdMetadata, replacing the previousClangdOptionsDefaults.- The new implementation in
IDFClangdMetadataDefaultsproperly implements theClangdMetadatainterface as verified by the file content.Everything appears to align with the upstream Eclipse CDT architectural changes.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java (3)
55-60: Simplified query driver configurationThe method now directly sets the toolchain path obtained from
IDFUtil.getToolchainExePathForActiveTarget()as the query driver setting. This implementation is cleaner and more straightforward.
64-67: Streamlined clangd path configurationThe implementation now uses the predefined metadata identifier and its default value directly. This approach is more consistent with the architectural changes in the PR.
48-50:✅ Verification successful
Direct usage of ClangdMetadata predefined identifiers
The code now uses
ClangdMetadata.Predefinedconstants directly, simplifying the implementation. Note that there's a possible typo in the method name -identifer()instead ofidentifier(). This appears to be from the upstream API, not an error in this code.Run this check to verify the method name in the upstream API:
🏁 Script executed:
#!/bin/bash # Check if the method is really spelled as "identifer" in the upstream API find . -name "*.java" -exec grep -l "identifer()" {} \;Length of output: 242
Action: Confirmed usage of upstream API's method naming conventions
The shell script output shows that the method is indeed spelled as
identifer()in multiple locations (including the upstream API). Therefore, the direct usage of theClangdMetadata.Predefinedconstants is valid, and the misspelled method name is inherited from the upstream API rather than being an error in our code.Approved as-is.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfigurationProvider.java (3)
70-71: Updated constructor call to include launch targetThe constructor call for IDFBuildConfiguration has been updated to include the launch target parameter, which is necessary for consistency with the changes in the IDFBuildConfiguration class.
80-81: Method signature update for build configuration creationThe method has been renamed from
createBuildConfigurationtocreateCBuildConfigurationand now includes anILaunchTargetparameter. The constant used for the configuration name has changed fromDEFAULT_NAMEtoTOOLCHAIN_ID, which might better reflect its purpose.These changes align with updates to the corresponding constructor in the IDFBuildConfiguration class and ensure the launch target information is properly passed through the build system.
Also applies to: 107-107, 119-120
52-57:✅ Verification successful
Simplified launch target logic
The code has been updated to use a more direct approach for determining if a build configuration should be created. Instead of complex logic that might have previously validated the toolchain based on the active launch target, this change simply returns null if the configuration name matches the default configuration name.
The addition of retrieving the active launch target but not using it in the immediate check seems redundant, though it may be used elsewhere.
🏁 Script executed:
#!/bin/bash # Check if the active launch target is used elsewhere in the method rg -A 5 -B 5 "target" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfigurationProvider.javaLength of output: 1348
Clarification: Active Launch Target is Properly Utilized
After verification, we can confirm that although the active launch target is retrieved before the default configuration check, it is indeed used later in the method when handling a toolchain change. The simplified logic that returns null for the default configuration is intentional, and the early retrieval of the launch target supports its later usage in constructing a new build configuration.bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)
125-128: Updated constructor to include launch target parameterThe constructor has been updated to include an
ILaunchTargetparameter, which is correctly passed to the more detailed constructor with appropriate default values for other parameters. This change supports the updated signature in the parent class.
130-135: Updated constructor signature for full configurationThe full constructor now includes the
ILaunchTargetparameter and properly passes it to the superclass constructor. This ensures that launch target information is properly maintained throughout the build configuration hierarchy.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/CMakeBuildTab2.java (4)
16-17: Updated imports for CMake configuration interfacesThe import for CMakeBuildConfiguration has been changed from the internal package to the public API, and the ICMakeBuildConfiguration interface has been added. This is a good practice as it reduces dependency on internal implementation details.
175-176: Updated attribute references to use new constantsThe code now uses CMAKE_BUILD_COMMAND and CMAKE_CLEAN_TARGET constants from CMakeBuildConfiguration instead of the previous constants. This ensures consistency with the updated LSP dependencies.
Also applies to: 179-180
218-219: Using interface instead of implementation classThe code now references ICMakeBuildConfiguration interface instead of CMakeBuildConfiguration class for accessing constants. This follows good object-oriented design principles by programming to interfaces rather than concrete implementations.
Also applies to: 226-227, 232-233
264-265: Updated remaining attribute references for consistencyThe remaining references to CMakeBuildConfiguration constants have been updated to use the new constant names, maintaining consistency throughout the class. This aligns with the changes in the CMake and LSP dependencies.
Also applies to: 299-300, 309-310
|
@kolipakakondal @sigmaaa hi ! Tested under: created project -> got error on "idf.py reconfigure": |
Hi @sigmaaa Not sure if you recall, but we have discussed this issue earlier during our team meetings. This issue happens when you’re holding on to the Clangd path in the preferences while it has actually been deleted from the file system. That means, the environment has become invalid! @AndriiFilippov To verify, can you check if the Clangd path exists in the file system? |
|
Hi @kolipakakondal, I've reproduced the issue—it occurs due to the default clangd path after installing tools in a fresh workspace. Working on a fix now. |
|
Hi @AndriiFilippov , @kolipakakondal The issue should be resolved after the latest commit. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/FileOpenListener.java (3)
2-2: Check the copyright year.The copyright notice shows 2025, which is a future date. Consider updating it to the current year or a range (e.g., "2023-2024") depending on your project's copyright policy.
57-74: Good fix for LSP server initialization race condition.This addition properly addresses the race condition where LSP servers could be restarted before the launch bar is fully initialized during Eclipse startup. The error handling is appropriate, and the comments clearly explain the issue that's being fixed.
The empty comment line (
//) on line 65 should be removed or filled with descriptive text.try { ILaunchTarget launchtarget = launchBarManager.getActiveLaunchTarget(); - // if (launchtarget == null) { return; } }
57-75: Consider refactoring for better error flow.The current implementation has a good safety check but could be made more concise by combining the null check with the exception handler.
ILaunchBarManager launchBarManager = IDFCorePlugin.getService(ILaunchBarManager.class); // Ensure the launch bar is initialized before restarting LSP servers. // Sometimes, when restarting Eclipse with a C file open, attempting to restart LSP servers immediately // during startup can throw an exception if the launch bar is not yet initialized. // This issue has been observed on CDT 12 and LSP 3.0.0. +ILaunchTarget launchTarget = null; try { - ILaunchTarget launchtarget = launchBarManager.getActiveLaunchTarget(); - // - if (launchtarget == null) - { - return; - } + launchTarget = launchBarManager.getActiveLaunchTarget(); } catch (Exception e) { Logger.log(e); + return; +} + +if (launchTarget == null) +{ + return; } lspService.restartLspServers();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/FileOpenListener.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/FileOpenListener.java (1)
10-11: LGTM: Required imports for launch target handling.The added imports are necessary for the new functionality to access launch target information.
|
Tested under: open .c file and restart IDE -> after restart, error: |
|
OS - Windows 11 create project - open .c file - click "Switch back to the classic C/C++ editing" unresolved headers: rebuild does not help. the only thing helps is right clicking in .c file - then unresolved headers disappears |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java (1)
98-111: Good use of background job for LSP operations.Moving the LSP query driver updates to a background job improves UI responsiveness by preventing these potentially lengthy operations from blocking the main thread. The 1000ms delay also helps ensure other initialization tasks complete before the LSP update begins.
However, there are some areas for improvement:
- No error handling if LSP operations fail
- No progress reporting to the user
- No checks to prevent multiple identical jobs from running simultaneously
Consider enhancing the job implementation with error handling and status reporting:
Job updateLspQueryJob = new Job("Update Lsp Query Drivers...") { protected IStatus run(IProgressMonitor monitor) { + try { + monitor.beginTask("Updating LSP query drivers", IProgressMonitor.UNKNOWN); LspService lspService = new LspService(); lspService.updateLspQueryDrivers(); lspService.restartLspServers(); + return Status.OK_STATUS; + } catch (Exception e) { + Logger.log(e); + return new Status(IStatus.ERROR, IDFUIPlugin.PLUGIN_ID, "Failed to update LSP query drivers", e); + } finally { + monitor.done(); + } - return Status.OK_STATUS; } }; +updateLspQueryJob.setUser(false); // System job, not visible to users +updateLspQueryJob.setRule(ResourcesPlugin.getWorkspace().getRoot()); // Prevent concurrent runs updateLspQueryJob.schedule(1000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java (3)
20-23: Appropriate import additions for Job implementation.The new imports for IStatus, Status, and Job are necessary for the asynchronous job implementation.
105-105:❓ Verification inconclusive
Investigate server restart implications.
The
restartLspServers()method is called after updating query drivers, which could interrupt ongoing editor sessions. Consider if there are cases where this restart could be avoided or if users should be notified.
🏁 Script executed:
#!/bin/bash # Examine how restartLspServers is implemented and what its impact might be echo "Checking restartLspServers implementation..." rg -A 15 "restartLspServers" --glob "*.java" echo "Checking for any user notification related to LSP server restarts..." rg "restart.*server.*notif|notif.*restart.*server" --glob "*.java"Length of output: 7163
Review Update: Revisit LSP Server Restart Behavior
TherestartLspServers()method inLspService.javaforcefully shuts down and restarts language servers without any user notification. Our repository search confirms that this behavior is consistent across several contexts (e.g., inLaunchBarListener.java,FileOpenListener.java, and during build operations) and no notification mechanisms were found. Given that this restart occurs after updates to query drivers, there's a risk of inadvertently interrupting active editor sessions. Please verify if the restart is strictly necessary in all cases, or if it would be preferable to either suppress unnecessary restarts or implement a user notification to mitigate potential disruptions.
103-106:❓ Verification inconclusive
Verify Clangd path handling in LspService.
Based on PR comments, there were issues with Clangd not being found during testing. This might be related to how the LspService handles Clangd paths.
Run the following to check how LspService handles Clangd paths:
🏁 Script executed:
#!/bin/bash # Check LspService implementation for Clangd path handling echo "Searching for Clangd path handling in LspService..." rg -A 10 "Clangd" --glob "*.java" | grep -i "path\|find\|locate\|not found" echo "Checking related preference handling..." rg -A 5 "getClangdPath\|setClangdPath" --glob "*.java"Length of output: 3307
Clangd Path Handling in LspService – Please Verify Manually
- The grep output confirms that
updateClangdPath()is implemented inLspServiceand is called fromToolsActivationJob.java. In the core LspService, it retrieves the toolchain path viaIDFUtil.getToolchainExePathForActiveTarget()and updates the corresponding preference node usingInstanceScope.INSTANCE.getNode().- Note that the search for explicit getter/setter methods (e.g.
getClangdPath/setClangdPath) returned no results. This appears intentional as the implementation relies on direct preference node manipulation.- Given the previous PR issues regarding Clangd not being found, please run manual tests to ensure that the fallback mechanism using
IDFUtil.findCommandFromBuildEnvPath(ILSPConstants.CLANGD_EXECUTABLE)correctly resolves clangd across all expected environments.
Oh man! Having the “Switch to the C/C++ Classic editor” option will screw us since we don’t support it! I don’t know if there’s a way to disable it! |
|
Hi @kolipakakondal , @AndriiFilippov |
That helps! |
|
Tested under:
Screen.Recording.2025-04-09.114235.mp4 |
|
Hi @AndriiFilippov, should be fixed in the last commit |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
46-46: Remove trailing whitespaceThere's a trailing space at the end of line 46 that should be removed.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 46-46: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
.github/workflows/ci.yml(2 hunks)bundles/com.espressif.idf.branding/.classpath(1 hunks)bundles/com.espressif.idf.core/.classpath(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfigurationProvider.java(5 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java(2 hunks)bundles/com.espressif.idf.debug.gdbjtag.openocd/.classpath(1 hunks)bundles/com.espressif.idf.help/.classpath(2 hunks)bundles/com.espressif.idf.launch.serial.core/.classpath(1 hunks)bundles/com.espressif.idf.launch.serial.ui/.classpath(1 hunks)bundles/com.espressif.idf.lsp/.classpath(1 hunks)bundles/com.espressif.idf.lsp/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdConfigurationAccess.xml(0 hunks)bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdOptionsDefaults.xml(1 hunks)bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFEditorMetadataDefaults.xml(1 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/LspPluginStartup.java(1 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdConfigurationAccess.java(0 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdMetadataDefaults.java(1 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdOptionsDefaults.java(0 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdPreferredOptions.java(0 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFEditorMetadataDefaults.java(1 hunks)bundles/com.espressif.idf.sdk.config.core/.classpath(1 hunks)bundles/com.espressif.idf.sdk.config.ui/.classpath(1 hunks)bundles/com.espressif.idf.serial.monitor/.classpath(2 hunks)bundles/com.espressif.idf.terminal.connector.serial/.classpath(1 hunks)bundles/com.espressif.idf.terminal.connector/.classpath(1 hunks)bundles/com.espressif.idf.ui/.classpath(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/CMakeBuildTab2.java(6 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/FormBrowser.java(2 hunks)bundles/com.espressif.idf.wokwi/.classpath(1 hunks)bundles/pom.xml(1 hunks)releng/com.espressif.idf.configuration/pom.xml(2 hunks)releng/com.espressif.idf.product/idf.product(2 hunks)releng/com.espressif.idf.product/pom.xml(1 hunks)releng/com.espressif.idf.target/com.espressif.idf.target.target(4 hunks)releng/com.espressif.idf.update/category.xml(0 hunks)
💤 Files with no reviewable changes (5)
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdOptionsDefaults.java
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdConfigurationAccess.java
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdPreferredOptions.java
- bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdConfigurationAccess.xml
- releng/com.espressif.idf.update/category.xml
🚧 Files skipped from review as they are similar to previous changes (14)
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/LspPluginStartup.java
- releng/com.espressif.idf.product/pom.xml
- bundles/pom.xml
- releng/com.espressif.idf.product/idf.product
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFEditorMetadataDefaults.java
- bundles/com.espressif.idf.lsp/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFEditorMetadataDefaults.xml
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java
- bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdOptionsDefaults.xml
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/CMakeBuildTab2.java
- releng/com.espressif.idf.configuration/pom.xml
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdMetadataDefaults.java
- releng/com.espressif.idf.target/com.espressif.idf.target.target
🧰 Additional context used
🧬 Code Graph Analysis (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfigurationProvider.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePlugin.java (1)
IDFCorePlugin(20-77)
🪛 YAMLlint (1.35.1)
.github/workflows/ci.yml
[error] 46-46: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (61)
.github/workflows/ci.yml (3)
41-45: LGTM: Java version upgrade to JDK 21The update from JDK 17 to JDK 21 and the newer setup-java action (v4) aligns with the PR objective of updating dependencies. This change modernizes the build environment and ensures compatibility with updated LSP components.
48-48: Simplified build command looks goodRemoving the SpotBugs execution from the build command while retaining other parameters simplifies the build process.
65-69: LGTM: Consistent JDK 21 upgrade for macOS workflowThe update from JDK 17 to JDK 21 for the macOS build environment is consistent with the changes made in the Linux workflow. This ensures uniformity across different build environments.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/FormBrowser.java (1)
54-77: LGTM: Refactored LocationListener implementationThe refactoring of the LocationListener implementation makes the code more concise by using an inline listener. The removal of
formText.setText(text)from thechangingmethod is a behavior change but appears intentional as it prevents redundant text setting. The comment in thechangedmethod appropriately explains why it's empty.bundles/com.espressif.idf.lsp/.classpath (3)
3-7: LGTM: Updated JRE container for flexible Java versionReplacing the specific JavaSE-17 path with a more generic JRE container path provides flexibility for Java version compatibility. Adding the Maven attributes ensures proper integration with Maven-based builds.
9-14: LGTM: Updated source path configurationExplicitly specifying the output directory and adding Maven-related attributes improves build configuration clarity and ensures proper integration with Maven.
15-19: LGTM: Added Maven integration classpath containerThe addition of the Maven classpath container enhances dependency management and integration with Maven-based builds.
bundles/com.espressif.idf.serial.monitor/.classpath (3)
3-14: LGTM: Updated JRE container and source path configurationThe changes to the JRE container and source path configuration align with the updates in other modules, providing flexibility for Java version compatibility and ensuring proper Maven integration.
30-31: LGTM: Added JNA librariesThe addition of JNA libraries (version 5.11.0) is appropriate for the serial monitor functionality, providing necessary native access capabilities.
37-37: LGTM: Updated output pathStandardizing the output path to "bin" aligns with the project's build structure conventions.
bundles/com.espressif.idf.branding/.classpath (2)
3-7: JRE container configuration updated for Maven integrationThe JRE container path has been generalized and the attribute changed to support Maven integration. This change aligns with the PR's objective to update LSP dependencies and modernize the build configuration.
9-13: Maven classpath container addedAdded Maven classpath container with appropriate attributes to support Maven dependency management, which is consistent with the project's modernization efforts.
bundles/com.espressif.idf.wokwi/.classpath (3)
3-7: JRE container configuration updated for Maven integrationThe JRE container path has been generalized, removing the specific JavaSE-17 reference, and the attribute has been changed to support Maven integration. This is consistent with the project's dependency updates.
9-14: Source path configuration enhanced with Maven attributesThe source path entry now includes an output directory and Maven-specific attributes, improving build configuration consistency.
15-19: Maven classpath container addedMaven classpath container added with appropriate attributes to support Maven dependency management, which is consistent with other files in this PR.
bundles/com.espressif.idf.launch.serial.core/.classpath (3)
3-7: JRE container configuration updated for Maven integrationThe JRE container path has been generalized and the attribute changed to support Maven integration, following the same pattern as other files in this PR.
9-14: Source path configuration enhanced with Maven attributesThe source path entry now includes an output directory and Maven-specific attributes, improving build configuration consistency.
15-19: Maven classpath container addedMaven classpath container added with appropriate attributes to support Maven dependency management, which is consistent with other files in this PR.
bundles/com.espressif.idf.debug.gdbjtag.openocd/.classpath (4)
3-7: JRE container configuration updated for Maven integrationThe JRE container path has been generalized and the attribute changed to support Maven integration, following the same pattern as other files in this PR.
9-14: Source path configuration enhanced with Maven attributesThe source path now includes an output directory (target/classes) and Maven-specific attributes, improving build configuration.
15-15: SVD library path added as exportedA new exported library path for "svd" has been added, which appears to be a module-specific requirement.
16-20: Maven classpath container addedMaven classpath container added with appropriate attributes to support Maven dependency management, consistent with other files in this PR.
bundles/com.espressif.idf.sdk.config.core/.classpath (4)
9-14: Source path configuration updated for Maven compatibility.The source path now specifies an output directory and includes Maven-related attributes. This change improves build consistency and Maven integration.
15-19: Maven classpath container properly configured.The Maven classpath container is correctly configured with the necessary attributes, ensuring proper Maven integration.
20-20: Output path standardized to 'bin'.The output path has been changed from 'target/classes' to 'bin'. This better aligns with Eclipse plugin conventions and creates consistency across the project.
3-7:✅ Verification successful
JRE container path has been generalized.
The JRE container path has been changed from a specific Java version to a generic container. This provides more flexibility but ensure that the minimum Java version is still properly specified in the MANIFEST.MF file to prevent compatibility issues.
🏁 Script executed:
#!/bin/bash # Verify that MANIFEST.MF still contains proper Java version requirements grep -r "Bundle-RequiredExecutionEnvironment:" bundles/com.espressif.idf.sdk.config.core/META-INF/MANIFEST.MFLength of output: 156
JRE Container Generalization Verified
The updated classpath entry in
bundles/com.espressif.idf.sdk.config.core/.classpathnow uses a generalized JRE container path, and we verified that the minimum Java version requirement is still enforced—theMANIFEST.MFcorrectly specifiesBundle-RequiredExecutionEnvironment: JavaSE-17. This ensures the necessary Java environment is maintained without tying the build to a specific version.bundles/com.espressif.idf.help/.classpath (3)
3-7: JRE container path has been generalized.The JRE container path has been simplified from a specific Java version to a generic container. This provides more flexibility but ensure that the minimum Java version is still properly specified in the MANIFEST.MF file.
9-9: Source path output directory updated.The source path configuration now specifies 'bin' as the output directory, aligning with Eclipse plugin conventions and ensuring consistency with other modules.
20-20: Output path standardized to 'bin'.The output path has been updated from 'target/classes' to 'bin', which standardizes the build output location across the project.
bundles/com.espressif.idf.launch.serial.ui/.classpath (4)
3-7: JRE container path has been generalized.The JRE container path has been changed from a specific Java version to a generic container. This change provides more flexibility in development environments but ensure that the minimum Java version is still properly specified in the MANIFEST.MF file.
9-14: Source path configuration updated for Maven compatibility.The source path now specifies 'bin' as the output directory and includes Maven-related attributes. This improves build consistency and Maven integration.
16-20: Maven classpath container added.The Maven classpath container has been correctly added with the necessary attributes, ensuring proper Maven integration for dependency management.
21-21: Output path standardized to 'bin'.The output path has been changed from 'target/classes' to 'bin', aligning with Eclipse plugin conventions and creating consistency across the project.
bundles/com.espressif.idf.ui/.classpath (5)
3-7: JRE container path has been generalized.The JRE container path has been changed from a specific Java version to a generic container. This provides more flexibility but ensure that the minimum Java version is still properly specified in the MANIFEST.MF file to prevent compatibility issues.
8-14: Source and plugin configuration updated for Maven compatibility.Both the required plugins container and source path have been configured with appropriate attributes for Maven integration, improving build consistency across the project.
21-21: Commonmark dependency properly configured.The commonmark library dependency is correctly configured as an exported library, ensuring it's available to dependent plugins.
22-26: Maven classpath container properly added.The Maven classpath container has been added with the necessary attributes, ensuring proper Maven integration for dependency management.
27-27: Output path standardized to 'bin'.The output path has been changed from 'target/classes' to 'bin', which standardizes the build output location across the project and aligns with Eclipse plugin conventions.
bundles/com.espressif.idf.core/.classpath (5)
3-7: LGTM: JRE Container Configuration UpdatedThe JRE container definition has been updated with Maven attributes, which is consistent with the Maven-centric approach being adopted throughout the project.
9-14: Maven integration: Source path output configurationThe source path entry now properly specifies "bin" as the output directory and includes the necessary Maven attributes to ensure proper integration with the build system.
15-30: Reorganized dependency entries with proper attributesThe library entries have been reorganized and updated with proper source and javadoc references where applicable. The addition of commons-lang3-3.12.0.jar likely supports the LSP implementation.
33-37: Maven classpath container integrationAdding the Maven classpath container enables proper Maven dependency resolution, which is essential for the ongoing LSP dependencies update.
38-38: Output path standardizationThe output path has been standardized to "bin", consistent with the source path output configuration and Eclipse plugin development best practices.
bundles/com.espressif.idf.terminal.connector/.classpath (4)
3-7: LGTM: Updated JRE container configurationRemoving the specific Java SE 17 version reference in favor of a more generic container improves flexibility and allows for easier migration between Java versions.
9-14: Enhanced source path configuration with Maven attributesAdding the output attribute and Maven-specific attributes to the source path entry ensures proper integration with the Maven build system.
15-19: Maven classpath container addedThis addition supports the Maven-centric approach being implemented across the project and ensures proper dependency resolution.
20-20: Standardized output pathChanging the output path from "target/classes" to "bin" ensures consistency with other projects in the workspace and aligns with the source output configuration.
bundles/com.espressif.idf.sdk.config.ui/.classpath (4)
3-7: LGTM: JRE Container added with Maven attributesAdding the JRE container with Maven attributes supports the transition to a Maven-integrated build system.
9-14: Enhanced source path with Maven integrationThe source path now includes proper output directory specification and Maven integration attributes, consistent with other projects in the workspace.
15-19: Maven classpath container integrationAdding the Maven classpath container ensures proper dependency resolution through Maven, supporting the LSP dependencies update.
20-20: Standardized output pathThe output path has been updated to "bin", consistent with other projects and the source path configuration.
bundles/com.espressif.idf.terminal.connector.serial/.classpath (5)
3-7: LGTM: Updated JRE container configurationThe JRE container has been updated from a specific Java SE 17 reference to a more generic one, allowing for greater flexibility in Java version support.
9-14: Enhanced source path with Maven attributesThe source path now includes output directory specification and Maven integration attributes, consistent with the Maven-centric approach.
15-16: Library path maintainedThe required libraries for WebSocket functionality are properly maintained in the updated classpath.
17-21: Maven classpath container addedThe Maven classpath container has been added to support proper dependency resolution through Maven.
22-22: Standardized output pathThe output path has been updated to "bin", consistent with other projects and aligning with Eclipse plugin development best practices.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfigurationProvider.java (5)
52-55: Launch target integration added to build configurationThe code now retrieves the active launch target from ILaunchBarManager, which is a good improvement that enables build configurations to be target-aware. This change supports better integration with Eclipse's launch framework and likely helps with the LSP dependency updates mentioned in the PR objectives.
56-58: Simplified control flow for default configurationThe method now returns null directly for default configurations instead of attempting to process them further. This is a cleaner approach that avoids unnecessary processing.
70-71: Launch target now passed to IDFBuildConfiguration constructorThe IDFBuildConfiguration constructor is now called with the launch target parameter. This ensures that build configurations have access to target-specific information, which is essential for LSP functionality.
119-120: Updated constructor call includes launch targetThe IDFBuildConfiguration constructor is now called with the launch target parameter, which is consistent with the updated method signature. This ensures proper initialization of the build configuration with target-specific information.
107-107:✅ Verification successful
Configuration name constant changed
Changed from using
ICBuildConfiguration.DEFAULT_NAMEtoICBuildConfiguration.TOOLCHAIN_ID. This could affect how build configurations are identified and managed.
🏁 Script executed:
#!/bin/bash # Check how TOOLCHAIN_ID is defined and used elsewhere rg "TOOLCHAIN_ID" --type javaLength of output: 1003
Verified Change: Constant Update is Consistent Across the Codebase
The update replacing
ICBuildConfiguration.DEFAULT_NAMEwithICBuildConfiguration.TOOLCHAIN_IDhas been confirmed across multiple files. The grep output shows thatTOOLCHAIN_IDis consistently used in the build configuration provider, configuration utilities, and toolchain manager classes. There appear to be no inconsistencies that could inadvertently affect how build configurations are identified. Please ensure that the runtime behavior aligns with this design change.
- Confirmed usage in:
IDFBuildConfigurationProvider.javaRecheckConfigsHelper.javaIDFBuildConfiguration.javaESPToolChainManager.java
| public ICBuildConfiguration createCBuildConfiguration(IProject project, IToolChain toolChain, String launchMode, | ||
| ILaunchTarget launchTarget, IProgressMonitor monitor) throws CoreException |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API method signature changed and renamed
The method has been renamed from createBuildConfiguration to createCBuildConfiguration and a new ILaunchTarget parameter has been added. This is a breaking change that might require updates to any code calling this method.
🏁 Script executed:
#!/bin/bash
# Check for any other code that might be calling the old method signature
rg "createBuildConfiguration\(" --type javaLength of output: 231
Action Required: Update API Call to the New Method Signature
The API method in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfigurationProvider.java was renamed from createBuildConfiguration to createCBuildConfiguration and now includes an additional ILaunchTarget parameter. However, the following call still uses the old method signature:
buildConfig = configManager.createBuildConfiguration(this, project, name, monitor);
Please update this invocation (and any similar references throughout the codebase) to use the new method name and include the required ILaunchTarget argument.
|
@AndriiFilippov please check |
issue still persist. |
fixed ✅ |
this will be fixed once the builds are available for this eclipse-lsp4e/lsp4e#1249 |
|
Hi @kolipakakondal, @AndriiFilippov Added JustJ dependency to the update site. Now it will be possible to update the Espressif-IDE with the update site using this option:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
46-46: Remove trailing whitespace
YAML lint flags trailing spaces on this blank line, which can cause CI failures.- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 46-46: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.github/workflows/ci.yml(2 hunks)bundles/com.espressif.idf.branding/.classpath(1 hunks)bundles/com.espressif.idf.core/.classpath(1 hunks)bundles/com.espressif.idf.core/META-INF/MANIFEST.MF(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfigurationProvider.java(3 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java(2 hunks)bundles/com.espressif.idf.debug.gdbjtag.openocd/.classpath(1 hunks)bundles/com.espressif.idf.help/.classpath(2 hunks)bundles/com.espressif.idf.launch.serial.core/.classpath(1 hunks)bundles/com.espressif.idf.launch.serial.ui/.classpath(1 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java(2 hunks)bundles/com.espressif.idf.lsp/.classpath(1 hunks)bundles/com.espressif.idf.lsp/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdConfigurationAccess.xml(0 hunks)bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdOptionsDefaults.xml(1 hunks)bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFEditorMetadataDefaults.xml(1 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/LspPluginStartup.java(1 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdConfigurationAccess.java(0 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdMetadataDefaults.java(1 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdOptionsDefaults.java(0 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdPreferredOptions.java(0 hunks)bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFEditorMetadataDefaults.java(1 hunks)bundles/com.espressif.idf.sdk.config.core/.classpath(1 hunks)bundles/com.espressif.idf.sdk.config.ui/.classpath(1 hunks)bundles/com.espressif.idf.serial.monitor/.classpath(2 hunks)bundles/com.espressif.idf.terminal.connector.serial/.classpath(1 hunks)bundles/com.espressif.idf.terminal.connector/.classpath(1 hunks)bundles/com.espressif.idf.ui/.classpath(1 hunks)bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF(3 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/CMakeBuildTab2.java(7 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/FileOpenListener.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/FormBrowser.java(2 hunks)bundles/com.espressif.idf.wokwi/.classpath(1 hunks)bundles/pom.xml(1 hunks)docs/en/prerequisites.rst(1 hunks)releng/com.espressif.idf.configuration/pom.xml(2 hunks)releng/com.espressif.idf.product/idf.product(2 hunks)releng/com.espressif.idf.product/pom.xml(1 hunks)releng/com.espressif.idf.target/com.espressif.idf.target.target(4 hunks)releng/com.espressif.idf.update/category.xml(2 hunks)
💤 Files with no reviewable changes (4)
- bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdConfigurationAccess.xml
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdPreferredOptions.java
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdConfigurationAccess.java
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdOptionsDefaults.java
✅ Files skipped from review due to trivial changes (7)
- docs/en/prerequisites.rst
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/FileOpenListener.java
- bundles/com.espressif.idf.launch.serial.ui/.classpath
- bundles/com.espressif.idf.branding/.classpath
- releng/com.espressif.idf.update/category.xml
- bundles/com.espressif.idf.wokwi/.classpath
🚧 Files skipped from review as they are similar to previous changes (28)
- releng/com.espressif.idf.product/pom.xml
- bundles/com.espressif.idf.launch.serial.core/.classpath
- bundles/pom.xml
- bundles/com.espressif.idf.core/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/LspPluginStartup.java
- bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/FormBrowser.java
- bundles/com.espressif.idf.serial.monitor/.classpath
- bundles/com.espressif.idf.terminal.connector.serial/.classpath
- bundles/com.espressif.idf.sdk.config.ui/.classpath
- bundles/com.espressif.idf.help/.classpath
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/CMakeBuildTab2.java
- releng/com.espressif.idf.configuration/pom.xml
- bundles/com.espressif.idf.lsp/.classpath
- bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFClangdOptionsDefaults.xml
- bundles/com.espressif.idf.lsp/OSGI-INF/com.espressif.idf.lsp.preferences.IDFEditorMetadataDefaults.xml
- releng/com.espressif.idf.product/idf.product
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFEditorMetadataDefaults.java
- bundles/com.espressif.idf.debug.gdbjtag.openocd/.classpath
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java
- bundles/com.espressif.idf.terminal.connector/.classpath
- bundles/com.espressif.idf.lsp/src/com/espressif/idf/lsp/preferences/IDFClangdMetadataDefaults.java
- bundles/com.espressif.idf.lsp/META-INF/MANIFEST.MF
- bundles/com.espressif.idf.sdk.config.core/.classpath
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfigurationProvider.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java
- bundles/com.espressif.idf.ui/.classpath
- releng/com.espressif.idf.target/com.espressif.idf.target.target
🧰 Additional context used
🧬 Code Graph Analysis (1)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)
🪛 YAMLlint (1.35.1)
.github/workflows/ci.yml
[error] 46-46: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_macos
🔇 Additional comments (10)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (2)
28-28: Good change to use the public interface instead of internal implementation.Switching from the internal class
CMakeBuildConfigurationto the public interfaceICMakeBuildConfigurationimproves code stability. Public interfaces are more stable than internal implementations, which can change without notice in future Eclipse releases.
488-488: Consistent update to use the interface constant.This change correctly updates the attribute key to use
ICMakeBuildConfiguration.CMAKE_ARGUMENTSconstant from the public interface, maintaining consistency with the import change above. This is good practice for maintainability and forward compatibility.bundles/com.espressif.idf.core/.classpath (6)
3-7: JRE container simplification improves build flexibility.The JRE container has been simplified by removing the specific Java SE version reference and adding Maven attributes. This change supports the Java 17 to Java 21 upgrade mentioned in the PR summary while allowing Maven to manage the Java version.
8-8: Explicit PDE required plugins container added.Adding the PDE core required plugins container early in the classpath ensures proper Eclipse plugin dependencies resolution.
9-14: Source path configuration updated for Maven compatibility.The source path configuration now includes explicit output directory and Maven-specific attributes, improving integration with the Maven build system.
33-37: Maven classpath container added.Adding the Maven classpath container improves integration with Maven's dependency management system, making the project more maintainable.
38-38: Output path standardized to "bin".The output path has been changed from what was likely "target/classes" to "bin". While this deviates from Maven conventions, it aligns with traditional Eclipse plugin conventions and is consistent with other bundle updates in this PR.
15-30:✅ Verification successful
Library entries reorganized and expanded.
The library entries have been reordered and new dependencies added (commons-lang3, snakeyaml, opencsv, etc.). This standardizes dependency management across the plugin.
🏁 Script executed:
#!/bin/bash # Verify that the new library dependencies are properly included in the build.properties file cat bundles/com.espressif.idf.core/build.properties | grep -E "commons-lang3|snakeyaml|opencsv|commons-beanutils|commons-text"Length of output: 347
Confirmed new dependencies in build.properties
The following entries are present in
bundles/com.espressif.idf.core/build.properties, matching the added libraries in.classpath:
- lib/snakeyaml-1.30.jar
- lib/opencsv-5.7.0.jar
- lib/commons-lang3-3.12.0.jar
- lib/commons-beanutils-1.9.4.jar
- lib/commons-text-1.10.0.jar
Everything looks good—merging these changes.
.github/workflows/ci.yml (2)
41-46: Verify Java 21 compatibility across CI jobs
The JDK version has been bumped to Java 21 in both the Linux and macOS workflows usingactions/setup-java@v4with thetemurindistribution. Please ensure:
- All Maven plugins, project code, and custom scripts support Java 21.
- Third-party dependencies and Eclipse/IDE plugins used in CI are compatible with JDK 21.
- Any version checks or tooling (e.g., code-signing, test frameworks) are updated accordingly.
Also applies to: 65-68
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 46-46: trailing spaces
(trailing-spaces)
47-48: Confirm removal of SpotBugs analysis
The Maven build step now omits the SpotBugs plugin execution and its dedicated GitHub Action. If static analysis is still required, please verify it has been re-integrated elsewhere or confirm this removal was intentional.
|
@sigmaaa hi ! Tested under: update site works ✅ |
Description
Please include a summary of the change and which issue is fixed.
Fixes # (IEP-XXX)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores