IEP-1419 Clean up CDT LSP/CDT internal dependencies #1149
Conversation
WalkthroughThis pull request updates two components. The first change modifies the manifest file for the bundle by adding the Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant Lsp as LspService
participant Accessor as LanguageServiceAccessor
Config ->> Lsp: Instantiate LspService with configuration and wrappers
Lsp ->> Accessor: getStartedWrappers(null, true)
Accessor -->> Lsp: Returns stream of LanguageServerWrapper
Lsp ->> Lsp: Filter wrappers and convert stream to List
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 (
|
![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/META-INF/MANIFEST.MF (1)
34-35: Consider adding version constraints for LSP dependencies.To prevent potential compatibility issues, consider adding version constraints for both LSP4E and LSP4J dependencies. This ensures that compatible versions are used and helps prevent breaking changes from future updates.
Example:
- org.eclipse.lsp4e, - org.eclipse.lsp4j + org.eclipse.lsp4e;bundle-version="[0.13.0,1.0.0)", + org.eclipse.lsp4j;bundle-version="[0.12.0,1.0.0)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (5)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LspService.java (4)
7-7: LGTM!The added imports are necessary for the updated implementation.
Also applies to: 14-14
23-23: LGTM! Good improvement.Changing from
StreamtoListis a better choice as:
- Lists are reusable, while Streams can only be consumed once
- The collection is used in
restartLspServers()which needs to iterate over the wrappers
27-29: LGTM! Successfully migrated to LSP4E.The changes successfully:
- Replace the Eclipse LSP CDT utility with LSP4E's
LanguageServiceAccessor- Maintain the filter for CDT language servers
- Update the constructor parameter to match the field type change
Also applies to: 32-32
25-30: Add unit tests for the LSP changes.Given that this change affects core LSP functionality:
- Add unit tests to verify the initialization of language server wrappers
- Add tests to ensure only CDT language servers are selected
- Verify the restart functionality with the new implementation
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1)
34-35: LGTM! The dependencies align with the PR objectives.The addition of
org.eclipse.lsp4jand reordering oforg.eclipse.lsp4esupport the migration from CDT LSP internal dependencies to LSP4E functionality. This change helps prevent potential errors during LSP CDT plugin updates.However, let's verify the version compatibility of these dependencies.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check the version compatibility between LSP4E and LSP4J # Get the target platform configuration fd -t f "target.platform" -x cat {} \; | grep -E "org.eclipse.lsp4[ej]" # Check if there are any version constraints in other manifest files fd -t f "MANIFEST.MF" -x grep -l "org.eclipse.lsp4[ej]" {} \; | xargs cat | grep -E "org.eclipse.lsp4[ej].*;bundle-version"Length of output: 201
I'll run an additional shell script to search all MANIFEST.MF files for "Require-Bundle:" lines as well as any explicit version constraints (using a semicolon after the dependency name) for the LSP dependencies.
🏁 Script executed:
#!/bin/bash echo "Searching for Require-Bundle lines with LSP dependencies:" fd -t f "MANIFEST.MF" -x grep -H "Require-Bundle:" {} \; echo "" echo "Searching for version constraints for org.eclipse.lsp4e or org.eclipse.lsp4j (if any):" fd -t f "MANIFEST.MF" -x grep -H "org.eclipse.lsp4[ej];" {} \;Length of output: 2313
Version Compatibility Requires Manual Verification
Our automated searches did not reveal any explicit version constraints (e.g. using the “;bundle-version” syntax) for the
org.eclipse.lsp4eandorg.eclipse.lsp4jdependencies in the manifest files. This absence suggests that version resolution for these bundles is likely managed by the overall target platform or OSGi framework rather than being hard-coded here.Given the importance of ensuring that the correct versions are in use, please manually verify that the versions provided by the target platform meet the compatibility requirements for the LSP4E functionality introduced by this PR.
|
Hi @sigmaaa LGTM. Which version of CDT-LSP plugin required for this? |
Hi @kolipakakondal. We are using the API of the LSP CDT 2.0 version |
|
@sigmaaa hi ! Tested under: Autocompletion ✅ |
Description
Replaced the Eclipse LSP CDT restart server utility call with the corresponding class from LSP4E to avoid errors during updating LSP CDT plugin to the master.
Fixes # (IEP-1419)
Type of change
Please delete options that are not relevant.
How has this been tested?
Verify that the Language Server works the same way as before.
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit