-
Notifications
You must be signed in to change notification settings - Fork 320
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
Make Xtext compatible with Guava 31 #2671
Conversation
Widen the version ranges in all MANIFEST.MF where the guava bundle is required to "[30.1.0,32.0.0)" and test Xtext with Guava 31 in the xtext-latest.target. Guice still requires Guava 30 so that version is still required, but this change should at least allow to use Xtext in conjunction with Guava 31.1. At the moment that is very problematic because Xtext plugins reexport the guava bundle. Because Guice uses Guava only internally and not in its API Guava classes of version 31 cannot 'leak' into Xtext and class-space consistency is ensured.
i have zero clue what could go wrong and thus cannot assess impact :( |
pushed to https://github.com/eclipse/xtext/tree/HannesWell-guava31compatibility so that jenkins can build repos to test with. |
as also mwe uses xtext i have no idea what happens.
do i need to install something extra to get this running still? |
running mwe fails with
|
so users would have to adapt build.properties |
same problem when running unit tests, this time from guice => is this a guava 31 specific problem? or caused by the mix? |
i also dont get this target resolved. @HannesWell any idea why jakarta is not found?
|
is this cause the artifact is also packaged to https://ci.eclipse.org/xtext/job/xtext/job/cd_guice6x_lsp4j0210/lastSuccessfulBuild/artifact/build/p2-repository/plugins/ |
=> this is also why i totally dislike the m2e pde feature. gives no clue where the problem is |
of course consuming with pure p2 works
but i still have to add transitive deps of guava directly. i assume this is why we do the reexport of the dependency to javax.inject in xtext |
Thanks for pushing and building the branch, will try it out with our product later this day. In your target in #2671 (comment), you have specified
This is because you pull in the com.google.inject 6.0.0 bundle from https://ci.eclipse.org/xtext/job/xtext/job/cd_guice6x_lsp4j0210/lastSuccessfulBuild/artifact/build/p2-repository, but this repo does not include jakarta.inject 2. Unfortunately it is currently not possible to resolve units from a So a solution would be to include jakarta-inject-2 in the repo that contains guice 6. |
@HannesWell yes i fixed the first manifest error by splitting it, but it still does not work.
|
Unfortunately I cannot reproduce that failure. But failureaccess is there and already a valid OSGi bundle. |
So you can run Workflows? How does you manifest and build.properties look like |
Sorry I just checked if the target resolves, assumed this was the issue. What should I try exactly? In a new workspace use the Target from your previous comment and try the Xtext examples you mentioned? |
Use the target, create new Xtext project, run workflow. Run generated init test if workflow succeeds |
OK, with that I get the same errors like you in #2671 (comment). 😞 I assume, MWE is not running in a OSGi runtime and has a flat classpath, doesn't it? |
Yes it is just java . But even if it succeeds pure java unit tests show same problem. |
OK, too bad. I still wonder why this happens, since plain Junit launches (and I assume also MWE) uses the dependency closure of the plugin. And if the plugin is resolved against guava 31, then the separate failure-access artifact should be included too, and if it is resolved against guava 30 from Orbit, those classes are in the guava-jar. Anyway, I'm closing this then since I assume it will not make it into the upcoming release. |
As suggested in #2056 (comment), this PR aims to widen the version ranges in all MANIFEST.MF where the guava bundle is required to "[30.1.0,32.0.0)".
Test Xtext with Guava 31 in the
xtext-latest.target
only and test it with Guava 30 in the older targets.Guice 5.0.1 requires Guava 30 so that version is still required, but this change should at least allow to use Xtext in conjunction with Guava 31.1 (altough this means that one needs two Guava versions if one wants to use Guava 31). At the moment one can not use Guava 31 when Xtext is used without errors because Xtext plugins reexport the guava bundle.
Because Guice uses Guava only internally and not in its API Guava classes of version 31 cannot 'leak' into Xtext and class-space consistency is ensured. If MWE does not reexport Guava and also only use it internally the same applies for that and a update in MWE is not required.