IEP-1639: Update query drivers after installing tools #1319
Conversation
WalkthroughAdds LspService.updateQueryDriver() to set ClangdMetadata.Predefined.queryDriver to "**". Updates ToolsActivationJob.run() to call updateClangdPath() and updateQueryDriver(), and to show a new user message. Introduces a new Messages key/value (ClangdPreferences_UpdatedMsg) and adjusts an existing messages.properties entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TAJ as ToolsActivationJob
participant LSP as LspService
participant Pref as Preferences Store
TAJ->>LSP: new LspService()
TAJ->>LSP: updateClangdPath()
LSP->>Pref: set clangdPath (current qualifier)
Pref-->>LSP: ok
TAJ->>LSP: updateQueryDriver()
LSP->>Pref: set queryDriver = "**" (current qualifier)
Pref-->>LSP: ok
TAJ->>TAJ: print(Messages.ClangdPreferences_UpdatedMsg)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java (1)
73-77: Verify the permissive query driver setting.The method unconditionally sets the query driver to
"**", which allows clangd to query all compilers in the PATH. While this provides maximum flexibility, it's a permissive setting that could potentially allow clangd to interact with any compiler, including potentially problematic ones.Additionally, unlike
updateClangdPath()(lines 61-71), this method lacks logging. Consider adding a log statement for consistency and debugging purposes.Apply this diff to add logging:
public void updateQueryDriver() { String qualifier = configuration.qualifier(); InstanceScope.INSTANCE.getNode(qualifier).put(ClangdMetadata.Predefined.queryDriver.identifer(), "**"); //$NON-NLS-1$ + Logger.log("Query driver set to '**' for qualifier: " + qualifier); //$NON-NLS-1$ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1)
Messages(9-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_macos
- GitHub Check: build_windows
🔇 Additional comments (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1)
78-78: LGTM!The new message field follows the established naming convention and integrates properly with the NLS message framework.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java (1)
179-179: LGTM!Minor formatting improvement to the comment.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1)
71-71: LGTM!The new message text clearly communicates that both the clangd path and query driver path have been updated.
| LspService lspService = new LspService(); | ||
| lspService.updateClangdPath(); | ||
| lspService.updateQueryDriver(); | ||
| console.println(Messages.ClangdPreferences_UpdatedMsg); |
There was a problem hiding this comment.
Success message may be misleading if clangd path update fails.
The code prints Messages.ClangdPreferences_UpdatedMsg unconditionally after calling updateClangdPath() and updateQueryDriver(). However, updateClangdPath() can fail silently if clangd is not found in the build environment (see LspService.java lines 64-67), while updateQueryDriver() always succeeds.
The success message states "clangd path and query driver path have been set" but may not be accurate if clangd was not found.
Consider checking the result or handling the case where clangd path update might fail:
LspService lspService = new LspService();
lspService.updateClangdPath();
lspService.updateQueryDriver();
-console.println(Messages.ClangdPreferences_UpdatedMsg);
+// Note: updateClangdPath() logs internally if clangd not found
+console.println(Messages.ClangdPreferences_UpdatedMsg);Alternatively, modify LspService.updateClangdPath() to return a boolean indicating success, and adjust the message accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LspService lspService = new LspService(); | |
| lspService.updateClangdPath(); | |
| lspService.updateQueryDriver(); | |
| console.println(Messages.ClangdPreferences_UpdatedMsg); | |
| LspService lspService = new LspService(); | |
| lspService.updateClangdPath(); | |
| lspService.updateQueryDriver(); | |
| // Note: updateClangdPath() logs internally if clangd not found | |
| console.println(Messages.ClangdPreferences_UpdatedMsg); |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java
around lines 99-102, the code prints a success message unconditionally even
though LspService.updateClangdPath() can fail silently; change the flow so you
detect whether the clangd path update succeeded (either by making
updateClangdPath() return a boolean or by adding a new LspService method like
isClangdConfigured()/getClangdPath() to check result), call that after
updateClangdPath(), and print the original success message only when clangd was
found/updated; otherwise print or log a different warning/error message
indicating clangd was not located while still updating query driver normally.
|
@AndriiFilippov Please verify |
|
Tested under: do see LGMT 👍 |
Description
Updating the query driver's path after installing the tools
Fixes # (IEP-1639)
Type of change
Please delete options that are not relevant.
How has this been tested?
modify query driver's path -> update tools -> verify that query driver's path is default
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes