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.

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

Use LSP4J 0.19.0 #305

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Use LSP4J 0.19.0 #305

merged 1 commit into from
Nov 18, 2022

Conversation

mickaelistria
Copy link
Contributor

No description provided.

@@ -13,7 +13,7 @@
<unit id="org.eclipse.lsp4j.jsonrpc" version="0.0.0"/>
<unit id="org.eclipse.lsp4j.debug" version="0.0.0"/>
<unit id="org.eclipse.lsp4j.jsonrpc.debug" version="0.0.0"/>
<repository location="https://download.eclipse.org/lsp4j/updates/releases/0.18.0/"/>
<repository location="https://download.eclipse.org/lsp4j/updates/releases/0.19.0/"/>
Copy link
Member

@cdietrich cdietrich Nov 18, 2022

Choose a reason for hiding this comment

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

this file (some lines above) still uses the old orbit/guava with the signing issue

@rubenporras
Copy link
Contributor

I do not see any code change, which implies that LSP4E would work still with LSP4J 0.18, why don't we keep as compatible in the MANIFESTs? That makes upgrading easier. Xtext is for example, still on 0.18.

@mickaelistria
Copy link
Contributor Author

I do not see any code change, which implies that LSP4E would work still with LSP4J 0.18, why don't we keep as compatible in the MANIFESTs? That makes upgrading easier. Xtext is for example, still on 0.18.

The issue is that if we don't make 0.19 a requirement, then people who update to latest LSP4E from a former installation may still use LSP4J 0.18 (which has some bugs). Forcing a newer version ensure downstream fixes are included.
LSP4J is not a singleton; LSP4E and XText can coexist in the same installation/runtime, using different versions of LSP4J.

@cdietrich
Copy link
Member

@rubenporras PRs for Xtext are already open. i am just waiting for the maven artifacts to be published which might have slipped through in the 0.19.0 lsp4j release by @jonahgraham

@rubenporras
Copy link
Contributor

Reading LSP4J, there was no breaking change in LSP4J 0.18 either, so we could actually accept 0.17 as well.

@cdietrich , good to know that, but even then, why force upgrading both projects in lockstep if there is no need to?

@cdietrich
Copy link
Member

for me this is a testing issue, at least on our side. if we have 0.18.0,0.20.0 we need to be very careful when we bump to 0.18.0,0.21.0 next time

@mickaelistria
Copy link
Contributor Author

why force upgrading both projects in lockstep if there is no need to?

This forces bugfixes and other improvements to be delived; this improved the quality of the overall software.

@mickaelistria mickaelistria merged commit 9a35f11 into eclipse:master Nov 18, 2022
@rubenporras
Copy link
Contributor

Okay, I understand. I was not aware one could have several versions of LSP4J, I will try that.

@jonahgraham
Copy link
Contributor

I know this is resolved, but I wanted to add some additional detail in the hope it is useful

LSP4J 0.17, 0.18 and 0.19 are all pretty compatible with each other, but there are bug fixes. 0.19 also does break the API, although I suspect few people are affected by that.

However even if today LSP4E works with 0.17, 0.18, and 0.19, if the version range isn't limited and someone does start referencing the new class added in 0.19 in LSP4E, there will be no errors in LSP4E at compile time. Only a class error at runtime.

LSP4J does not have a stable, backwards compatible API like many other projects do, so I strongly recommend continuing to tightly constrain the versions.

Finally, I am very grateful to see @mickaelistria quickly updating LSP4E to the latest LSP4J release we put out. The releases have been much more frequent recently and the effort has not gone unnoticed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants