search

eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
613 stars 145 forks source link

Update version range for Gson to '[2.9.1,2.11)' #690

Closed HannesWell closed 1 year ago

HannesWell commented 1 year ago

As suggested by @jonahgraham in https://github.com/eclipse-m2e/m2e-core/pull/1146#issuecomment-1362278527 this updates Gson version range to include the latest Gson releases 2.10 and 2.10.1:

cdietrich commented 1 year ago

see also https://github.com/eclipse/lsp4j/pull/689 am not sure which variant we want

HannesWell commented 1 year ago

see also #689 am not sure which variant we want

Yours definitely looks more complete. And 2.10 also just added new methods (see https://github.com/eclipse-m2e/m2e-core/pull/1146#issuecomment-1362282140). So if lsp4j is using gson as consumer and not as provider, the lower can actually stay where it is. If that is wanted it is probably better to update your PR accordingly. And if not yours looks more complete.

So either way this can be closed I think. Thanks for the hint.

HannesWell commented 1 year ago

@jonahgraham reopened due to https://github.com/eclipse-pde/eclipse.pde/issues/482.

cdietrich commented 1 year ago

@HannesWell is there any documentation on versioning policy on googles side?

HannesWell commented 1 year ago

Unfortunately not really, no. The only thing I found is a Policy for guava: https://github.com/google/guava/wiki/ReleasePolicy

But this obviously does not apply to gson.

cdietrich commented 1 year ago

please note: we at xtext wont be able to build a new release to include this fix for 2023-03

HannesWell commented 1 year ago

please note: we at xtext wont be able to build a new release to include this fix for 2023-03

Understand. Does xtext use similar tight version ranges on gson like lsp4j? If not it is hopefully less problematic.

jonahgraham commented 1 year ago

BTW, I expect xtext to be strictly dependent on lsp4j 0.20.0, so a new lsp4j but no new xtext will mean three versions of lsp4j?

HannesWell commented 1 year ago

BTW, I expect xtext to be strictly dependent on lsp4j 0.20.0, so a new lsp4j but no new xtext will mean three versions of lsp4j?

If the version range is very strict and does not even allow a 0.20.1, then yes. :/

I tried it, but didn't find out where exactly xtext depends on lsp4j? @cdietrich can you please clarify in this regard?

jonahgraham commented 1 year ago

If the version range is very strict and does not even allow a 0.20.1, then yes. :/

OK. Thank you for pointing this part out as I otherwise would have done a 0.21.0 release. We (LSP4J) haven't done a x.x.1 release in a while, but I can give it a go.

jonahgraham commented 1 year ago

I tried it, but didn't find out where exactly xtext depends on lsp4j? @cdietrich can you please clarify in this regard?

See https://github.com/eclipse/xtext-core/issues/2019 for xtext tracking issue. It has links to all the code updates needed for LSP4J change.

HannesWell commented 1 year ago

If the version range is very strict and does not even allow a 0.20.1, then yes. :/

OK. Thank you for pointing this part out as I otherwise would have done a 0.21.0 release. We (LSP4J) haven't done a x.x.1 release in a while, but I can give it a go.

I just checked the content metadata in https://download.eclipse.org/modeling/tmf/xtext/updates/releases/2.30.0/ and it shows me that org.eclipse.xtext.ide requires org.eclipse.lsp4j(.jsonrpc) [0.20.0,0.21.0)

        <required namespace='osgi.bundle' name='org.eclipse.lsp4j' range='[0.20.0,0.21.0)' optional='true' greedy='false'/>
        <required namespace='osgi.bundle' name='org.eclipse.lsp4j.jsonrpc' range='[0.20.0,0.21.0)' optional='true' greedy='false'/>

The Plugin org.eclipse.xtext.testing just requires lsp4j without a version range. Besides that nothing else in xtext seems to require lsp4j/e.

Therefore I think a 0.20.1 lsp4j should fully supersede lsp4j 0.20.0 in SimRel. But maybe @merks can also check with his tools.

cdietrich commented 1 year ago

Yes we would accept a 0.20.1 I guess. also the deps are optional afaik But I don’t know what happens with tycho When maven and eclipse dependencies are mixed As our bom (see Xtext-lib) points to 0.20.0 as maven version And newer tycho versions also seem keen to look at maven coords

cdietrich commented 1 year ago

native question: a lsp4e bump based on the last release with just lsp4j/gson dep. bumped is not an option?

HannesWell commented 1 year ago

But I don’t know what happens with tycho When maven and eclipse dependencies are mixed As our bom (see Xtext-lib) points to 0.20.0 as maven version

I cannot tell that. Maybe @laeubi knows in detail? But at least in the p2-world (or Tycho builds based on targets) maven bom's are not considered AFAIK.

native question: a lsp4e bump based on the last release with just lsp4j/gson dep. bumped is not an option?

I wondered that as well. @mickaelistria would it be?

merks commented 1 year ago

@HannesWell

Yes xtext would accommodate a 0.20.1 version of lsp4j. Is the only thing strictly requiring >= 0.20.0:

image

If you have snapshot versions of your proposed changes available somewhere I can repeat the analysis. I can even locally build the aggregation and use that to test creating the DSL package, so it would be really great to have snapshot versions (of lsp4j and m2e I guess?) ASAP...

cdietrich commented 1 year ago

the tycho maven stuff is this one: https://github.com/eclipse/xtext-eclipse/issues/1931 as our p2 stuff is created from maven artifact, tycho seems to look at maven artifacts. so i dont know what would happen.

cdietrich commented 1 year ago

@merks you can find p2 repo for this pr at https://ci.eclipse.org/lsp4j/job/lsp4j-github-pullrequests/399/artifact/build/p2-repository/

HannesWell commented 1 year ago

@merks you can find p2 repo for this pr at https://ci.eclipse.org/lsp4j/job/lsp4j-github-pullrequests/399/artifact/build/p2-repository/

Thanks. Btw. is there a reason, why you don't have a dedicated job per PR, for example like in m2e? https://ci.eclipse.org/m2e/job/m2e/ This makes it easier to track the results of one PR.

If you have snapshot versions of your proposed changes available somewhere I can repeat the analysis. I can even locally build the aggregation and use that to test creating the DSL package, so it would be really great to have snapshot versions (of lsp4j and m2e I guess?) ASAP...

That is what I plan to contribute to SimRel for M2E tonight: https://download.eclipse.org/technology/m2e/snapshots/latest/

cdietrich commented 1 year ago

i see it for my branches, they are at eclipse/lsp4j, but you pr'ed from your account

HannesWell commented 1 year ago

i see it for my branches, they are at eclipse/lsp4j, but you pr'ed from your account

Yes, but the way for example the m2e Jenkins project is set up (I don't know exactly what the infra team did different) and AFAICT for all platform TLP projects, a new job is created for each PR in a dedicated tab, regardless from where the branch is coming. See for example PDE: https://ci.eclipse.org/pde/job/eclipse.pde/view/change-requests/

cdietrich commented 1 year ago

i dont know enhough about jenkinsfiles to answer this.

merks commented 1 year ago

Wasn't the plan here for a 0.20.1 version?

image

That's not what I see here:

image

Of course xtext's dependencies don't resolve those those:

image

Did I misunderstand something?

cdietrich commented 1 year ago

argg of course hannes pred against master which has already version bumped too far

merks commented 1 year ago

So is there a branch for building 0.20.1? I just don't understand what I should b expecting here... I certainly didn't expect these things to be progressed beyond their release state before/during RC1.

HannesWell commented 1 year ago

So is there a branch for building 0.20.1? I just don't understand what I should b expecting here... I certainly didn't expect these things to be progressed beyond their release state before/during RC1.

Just rebased the PR on the v0.20.0 tag and replaced fcd6b76a458fb8c2b23ddf04666e66341e313f1a with a corresponding commit for 0.20.1. Mainly for testing now if this change helps. The lsp4j committers then have to decide if and how they use this change and its parts.

I just don't understand what I should b expecting here... I certainly didn't expect these things to be progressed beyond their release state before/during RC1.

Me neither. I just try to help a bit. :)

HannesWell commented 1 year ago

@merks you can find p2 repo for this pr at https://ci.eclipse.org/lsp4j/job/lsp4j-github-pullrequests/399/artifact/build/p2-repository/

The new build ist hopefully a 0.20.1.

@merks can you please try:

https://ci.eclipse.org/lsp4j/job/lsp4j-github-pullrequests/400/artifact/build/p2-repository/

merks commented 1 year ago

That looks better with only features requiring the higher version of gson.

image

And xtext.ide resolving:

image

I'll try to build the aggregation locally (with my horrible internet here)....

merks commented 1 year ago

Woo hoo!

The DSL package installed from the aggregated result of the new m2e and this lsp4j version contains only a single version of gson:

image

Visiting all preference pages which tends to activate all bundles works well too.

I will now test the JEE package as well but given it was working fine before I don't expect a problem....

laeubi commented 1 year ago

with only features requiring the higher version of gson

It might be good to not include such dependencies in features, they should be pulled in by their using bundle anyways?

merks commented 1 year ago

Indeed, though often when the platform has tried to remove includes, something stopped working as @akurtakov can confirm. Also, includes are often present because that helps control what's included in the p2 repository; not everyone can or should just include all dependencies, which is a nice and easy solution when you are at the bottom of the food chain and your dependencies tend to be mostly just third party ones...

merks commented 1 year ago

The JEE package still has two versions:

image

But none of the exports have been substituted:

g! b 10 com.google.gson_2.9.1.v20220915-1632 [10] Id=10, Status=RESOLVED Data Root=D:\Users\test17\jee-latest-local-aggr\eclipse\configuration\org.eclipse.osgi\10\data "No registered services." No services in use. Exported packages com.google.gson.internal; version="2.9.1"[exported] com.google.gson.internal.bind; version="2.9.1"[exported] com.google.gson.internal.bind.util; version="2.9.1"[exported] com.google.gson.internal.reflect; version="2.9.1"[exported] com.google.gson.internal.sql; version="2.9.1"[exported] com.google.gson; version="2.9.1"[exported] com.google.gson.annotations; version="2.9.1"[exported] com.google.gson.reflect; version="2.9.1"[exported] com.google.gson.stream; version="2.9.1"[exported] Imported packages sun.misc; version="0.0.0" <org.eclipse.osgi_3.18.300.v20230211-2021 [0]> No fragment bundles No required bundles

So that still looks fine.

laeubi commented 1 year ago

I think we addressed already a lot of the "something", so I think its worth to try some things (again) e.g. we have now:

So one can use "include all" but still filter items provided from another update-site, and one can include any source if desired (not sure how good this applies to the epp / simrel builds)

merks commented 1 year ago

@jonahgraham

FYI, it looks like we are zeroing in on a solution to the gson/lsp4j problem!

akurtakov commented 1 year ago

Platform issues when removing includes are exactly that things didn't end up in the p2 site so as long as one verifies the proper content is in the p2 site I believe using import is better.

merks commented 1 year ago

@akurtakov We also ran into strange problems with removing includes from the test feature and we didn't have time to track that down. I totally agree we should try this again during the 4.28 release cycle. It would make updating the target platform much less painful and way less time consuming...

merks commented 1 year ago

@laeubi

We digress, but this sounds interesting!

https://github.com/eclipse-tycho/tycho/blob/master/RELEASE_NOTES.md#new-parameter-for-tycho-p2-repository-pluginassemble-repository

I found the docs here:

https://tycho.eclipseprojects.io/doc/master/tycho-p2/tycho-p2-repository-plugin/assemble-repository-mojo.html

It's not entirely clear what a "referenced repository" is but I guess that's a repository referenced from the category.xml; that would indeed be a useful feature...

laeubi commented 1 year ago

@merks contributions for better wording are always welcome, developers (at least me) are often the wrong person to write docs because for them everything is "clear from the code"

This is how it is used with m2e: https://github.com/eclipse-m2e/m2e-core/blob/e466baf6a855762072bb487fc4858db1a11ff9c2/org.eclipse.m2e.repository/category.xml#L30-L32

jonahgraham commented 1 year ago

Thanks all for your input and tests. I will ASAP make a 0.20.1 release that contains this change.

HannesWell commented 1 year ago

Thanks all for your input and tests. I will ASAP make a 0.20.1 release that contains this change.

Thank you Jonah for performing this release. Should I create another PR targeting main, so that we also have the wider Version range for 0.21?

jonahgraham commented 1 year ago

Should I create another PR targeting main, so that we also have the wider Version range for 0.21?

I did that already but I hadn't pushed it yet. It is now here in #701