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

Downgrade gson to 2.8.9 #426

Closed
wants to merge 1 commit into from
Closed

Conversation

sebthom
Copy link
Member

@sebthom sebthom commented Jun 28, 2022

In the last release we/I upgraded gson to 2.9.0. The upgrade was not done for a functional reason but for working with the latest bugfix release.

Doing so however seems to somehow break dependency resolution in tycho builds of plugins that depend on TM4E and on another plugin that requires gson exactly in version 2.8.9.

For example after upgrading the dependency to TM4E 0.5.0 of an eclipse plugin that also depends on the latest LSP4E, tycho builds suddenly fail claiming gson 2.8.9 is not resolvable even though it is provided by the configured LSP4E p2 repository.

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:2.7.3:validate-classpath (default-validate-classpath) on project org.haxe4e: Execution default-validate-classpath of goal org.eclipse.tycho:tycho-compiler-plugin:2.7.3:validate-classpath failed: org.osgi.framework.BundleException: Bundle org.haxe4e cannot be resolved:org.haxe4e [143]
[ERROR]   Unresolved requirement: Require-Bundle: org.eclipse.lsp4e
[ERROR]     -> Bundle-SymbolicName: org.eclipse.lsp4e; bundle-version="0.13.12.202206011407"; singleton:="true"
[ERROR]        org.eclipse.lsp4e [112]
[ERROR]          Unresolved requirement: Require-Bundle: org.eclipse.lsp4j.jsonrpc; bundle-version="[0.14.0,0.15.0)"
[ERROR]            -> Bundle-SymbolicName: org.eclipse.lsp4j.jsonrpc; bundle-version="0.14.0.v20220526-1518"
[ERROR]               org.eclipse.lsp4j.jsonrpc [116]
[ERROR]                 Unresolved requirement: Import-Package: com.google.gson; version="[2.8.9,2.9.0)"
[ERROR]          Unresolved requirement: Require-Bundle: org.eclipse.lsp4j; bundle-version="[0.14.0,0.15.0)"
[ERROR]            -> Bundle-SymbolicName: org.eclipse.lsp4j; bundle-version="0.14.0.v20220526-1518"
[ERROR]               org.eclipse.lsp4j [115]
[ERROR]                 Unresolved requirement: Import-Package: com.google.gson; version="[2.8.9,2.9.0)"
[ERROR]
[ERROR] -> [Help 1]

Interestingly, directly building in Eclipse IDE and even launch the plugins works just fine.

Since we have no functional requirement for gson 2.9.0 I would like to downgrade the dependency to 2.8.9 for the time being.

@akurtakov
Copy link
Contributor

The issue would better be reported to LSP4J as it has com.google.gson; version="[2.8.9,2.9.0)" which may cause issues unless others have also so strict version and no one really requires [2.9.0,)

@akurtakov
Copy link
Contributor

https://github.com/eclipse/lsp4j/blob/main/gradle/versions.gradle#L19 is where it's defined so strict.

@sebthom
Copy link
Member Author

sebthom commented Jun 29, 2022

There is already an open issues and from the comments it does not look like the maintainers see the need to changes this eclipse-lsp4j/lsp4j#611

Since this is about plugin internal OSGI dependencies and not Maven dependencies, it actually should work even if lsp4j pins to a specific gson version. I have no idea where or why the dependency resolution fails only in tycho builds but until this is solved I would suggest we downgrade our minimum gson version requirement to the one that currently is shipped with eclipse by default anyways.

@sebthom sebthom requested a review from mickaelistria June 29, 2022 13:29
Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

This seems more like the root bug comes from Tycho not working well with multiple versions. I don't think a bug in Tycho is a good reason to move dependency versions backwards in LSP4E.

@sebthom
Copy link
Member Author

sebthom commented Jul 8, 2022

I opened a ticket against tycho with a reproducer but I don't see a solution for this anytime soon eclipse-tycho/tycho#1103

And since we really don't need gson 2.9+ I would ask to be pragmatic and take the path of least resistance which is downgrading to gson 2.8.9 for now.

This issue is blocking any tycho builds that depend on TM4E latest and LSP4E latest at the same time.

@sebthom sebthom requested a review from mickaelistria July 8, 2022 20:15
@cdietrich
Copy link

cdietrich commented Jul 9, 2022

i assume geson 2.9 will be available with the next orbit Milestone (M2)
https://download.eclipse.org/tools/orbit/downloads/drops/I20220706225153/repository/plugins/

@rubenporras
Copy link
Contributor

I would also be happy if we are pragmatic here, I also hit this bug in our builds (we use Tycho) when upgrading.

Also, I do not know why, but the bug prevents us from launching our product with a launch configuration, so at least for us it is not only about the build. In any case it does not matter much, because if the tycho build does not work, we cannot upgrade.

We also need to take into account that the meta-data written to your local Maven repository (e.g. ~/.m2/repository/.meta/p2-artifacts.properties) by Tycho 2.7 changed. That might make upgrading Tycho harder than usual (we are indeed stuck for the moment with Tycho 2.6).

@sebthom
Copy link
Member Author

sebthom commented Jul 25, 2022

@mickaelistria @rubenporras should we put this to a vote? this is a real blocker for us with no working solution in sight. my issue eclipse-tycho/tycho#1103 was closed as won't fix and the mentioned workaround to build with -Dtycho.equinox.resolver.uses=true does not work in our builds. So I again like to ask to simply downgrade the dependency and create a new minor release of tm4e.

@cdietrich
Copy link

please note, current lsp4j snapshot uses gson 2.9.0

@sebthom
Copy link
Member Author

sebthom commented Jul 25, 2022

that is good to know, thanks! however, downgrading would not prevent usage of 2.9.0 as we don't pin to a minor release but define the lowest version. so it will work with all lsp4j versions. and we don't have to wait for the next lsp4j or maybe even the next lsp4e release.

@rubenporras
Copy link
Contributor

@sebthom , I still think that being pragmatic and downgrade is the way to go if it solves the problem, but I seldom did any maintenance on TM4E and I did not dig enough into the problem to understand it, so while I would very much appreciate a solution and I would upgrade TM4E on our builds the next day ;), I would like to leave the review to @mickaelistria .

@mickaelistria
Copy link
Contributor

I'm personally against the idea of implementing workaround downstream, particularly when the workaround is intentionally moving against the current, for issues that happen upstream and can be fixed. So I won't support this change, but I don't veto it either
In the meantime, I've worked on a patch for Tycho 3.0.0-SNAPSHOT that intends to solve the root issue behind this whole story: eclipse-tycho/tycho#1201 . @sebthom if this patch is merged soon, then would this change still be useful?

@sebthom
Copy link
Member Author

sebthom commented Jul 27, 2022

@mickaelistria is there a snapshot repo with your patch available somewhere, I would Iike to give it a try to see the impact on our builds.

since we have no functional requirement to use gson 2.9.0+ I still favor to downgrade the dependency and create a release asap.

@mickaelistria mickaelistria dismissed their stale review July 27, 2022 12:48

Not blocking

@mickaelistria
Copy link
Contributor

A fixed Tycho 3.0.0-SNAPSHOT is now available. See https://github.com/eclipse/tycho/blob/master/CONTRIBUTING.md#try-snapshots-and-report-issues to enable it.

@rubenporras
Copy link
Contributor

I have notice that TM4E 6.0.0 requires Java 17 as well. The world moves fast :)

The requirement means that we will not be able to upgrade for a while, even if the problem with Tycho is solved.

@mickaelistria
Copy link
Contributor

The requirement means that we will not be able to upgrade for a while

I'm curious: what are the things that are preventing you from running against Java 17? Most users do use JustJ which is currently Java 17, so it's the best supported version in practice.

@rubenporras
Copy link
Contributor

We have one code base which we package in different ways by selecting only a subset of plugins to create different products. One product is the IDE which has the UI plugins, that one runs on Windows with a bundled JVM, so there we can upgrade quick if we want to.

However other part of the codebase is used for compilers and deployed on several UNIXs, there our customers provide the JVM, and we must announce months in advance that we require a newer JVM. We asked already our customers this year to go to Java 11 (we wanted Java 17 while we where at it, but sadly at that point in time we needed to do the announcement there was no JVM for Java 17 on some AIX variant). Our application is quite complex and hit several bugs of the JVMs they installed in AIX, so that is expected to happen again for Java 17 on AIX. We could probably require different versions of Java depending on the product and the OS, but that requires some work as well.

So quite some things to consider, not possible to move as quick as one would like to. In any case not before 2023.

@rubenporras
Copy link
Contributor

Also, I am not sure but I believe Xtext eclipse-xtext/xtext#1982 can run only with Java 11 compatibility enabled, correct, @cdietrich ? I am not aware there are committed plans to support Java 17 compatibility mode for the next release.

@mickaelistria
Copy link
Contributor

We have one code base which we package in different ways by selecting only a subset of plugins to create different products. One product is the IDE which has the UI plugins, that one runs on Windows with a bundled JVM, so there we can upgrade quick if we want to.

Isn't TM4E part of this UI group?

@rubenporras
Copy link
Contributor

Yes, it is.

@sebthom
Copy link
Member Author

sebthom commented Jul 28, 2022

@mickaelistria I gave the tycho snapshot version a try and it works. thanks a lot for your support!!

@sebthom sebthom closed this Jul 28, 2022
@sebthom sebthom deleted the patch-02 branch October 21, 2022 19:56
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.

5 participants