-
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
Guice upgrade to 7.0.0+ #2056
Comments
hi, i dont know about cq. in doubt you can ask on the emo mailing list |
With the upcoming 2022-06 release Eclipse-platform fully supports to obtain dependencies directly from Maven-Central: If the Guice jars contain a OSGi compliant MANIFEST.MF Orbit can be skipped. |
@HannesWell no it cant, cause there is no solution for oomph and tycho 1.7 |
What Eclipse-Platform is doing is to include the I-Build repo (which includes all dependencies) in their Oomph setup, which contains those dependencies. Aren't you planning to update to Tycho 2.7.2?
That's the good thing: Nothing. The PGP-signature should be understood by P2 just like jar-signatures. Another point is that XText should make sure licences are vetted correctly. But also for this task GitHub workflows for Maven exist that can be copied: |
we dont want to use our old builds in oomph. oomph also has to work with older target platform, so we cannot use platform i-builds there. and we use tycho 1.7 for older versions / java 8 and tycho 2.7 only for build against latest. what i dont understand: |
Understand. With this requirement, especially using Tycho 1.x (which will likely wont get updates to support that), I don't see another solution than having a p2 repo from which you can get the new version. As far as I understand it the plan is to make Orbit obsolete where possible, so the Orbit maintainers maybe first will reject a request to include Guice with a reference to the improved situation to improve bundls. However I think it is not the goal to let projects create their own p2-mirrors. On the other hand Orbit would then only have to mirror the maven artifacts into a p2-repo. Or do new releases of Xtext also target older versions of the Eclipse-Platform? If so you would still require a jar-signed version of Guice, wouldn't you?
Which part to you mean exactly? |
we support platform back to oxygen and java 8,11 and 17 (partially) and i have zeroclue about jar vs pgp signing and what is verified where and how |
That's a long history. Then I suspect you need jarsigned dependencies. Unless it is acceptable that consumers get warnings about unsigned content when installing xtext (respectively Guice) into older platforms.
I never contributes to orbit so don't know the exact details how Orbit works but AFAICT it basically re-creates the jars from (for example) Maven-Central using the original class files but adds a custom Manifest (if necessary) with partially Eclipse-custom metadata and performs own jar-signing. On the other hand when using Tycho to mirror Maven artifacts into a P2 repo (like you did in https://github.com/xtext/xtext-orbit-replacement) it just copies to jar into a new folder structure and creates a corresponding artifact/content.xml metadata, if that artifact has a OSGi compliant MANIFEST. If not Tycho (like M2E) uses BND-tools to generate one suitable MANIFEST. So in the end that process is likely not that different to what Orbit is/was doing already. Maybe Orbit is already leveraging Tycho in this way, as I have not yet looked into the details of the Orbit process.
Yes that is not ideal.
I'm not an expert either. Basically these are two different techniques to very the integrity and creator of a jar. The former is can be used with tools build into the JDK the latter one is mandatory to publish artifacts to Maven-Central (I suspect they choose that technique because you don't need a jarsigning certificate from a Certification-authority which usually costs some money). Btw. if you want to proceed with https://github.com/xtext/xtext-orbit-replacement you maybe want to add the However as you said this should not be done by each project individually. So I suggest to ask Orbit to include the new Guice. |
the current orbit manifest has [30.1,31) @frankbenoit do you know if this can be fixed in bnd at orbit? |
@merks do you still have the report where one can see what depends on what so that we can see who else not related to xtext uses guice? |
These are all users with requirements that are satisfied by only one available version: Most would be satisfied also with a version < 6.0. Note that if you install a SimRel IDE using the Oomph link here: https://ci.eclipse.org/simrel/ You can open the simrel.aggran and view this information for the current state the SimRel contributions. |
the oom issue got merged in guice |
A new guice release is planned soon (probably guice 6.0), which will also support jakarta.inject (almost completely): It will also require guava 31. |
the error_prone dependency does not seem to have proper manifest. @HannesWell is there an option to exclude certain transitives with the maven deps in target feature? |
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
somehow does not work in 202203
|
junit5 tests also make problems https://ci.eclipse.org/xtext/job/xtext/job/cd_guice6x/3/testReport/junit/org.eclipse.xtext.testing.tests.junit5/InjectionExtensionNested2Test$NestedClass/innerTest/ this looks like some non matching guava and guice versions are mixed together when running the test from maven
will try to rebase on the lsp4j 0.21.0 branch |
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
I spent a lot of time trying to get this to work. You may wish to read the saga here: |
@LorenzoBettini do you know if there is a way to exclude specific p2 deps from the maven surefire run? |
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
also mwe has strict dependency to older guava |
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
did some experiments with guice 7 + guava 32 at https://github.com/eclipse/xtext/tree/cd_testg32guice7 now i try to get it to simrel maven https://github.com/eclipse-oomph/oomph.incubator/pull/1/files |
Just want to mention that with #2207 you would not have to deal with simrel maven and could just add any new version of guice to xtext's TP. |
i know |
@frankbenoit in case you also need assisted inject. can you get in contact with @merks to get it into simrel maven p2 repo |
What do you guys think needs to be there that isn't currently already there? |
in the old orbit there was also
which was not used by Xtext diretly but by Xtext consumers (@frankbenoit ) |
There's a 5.1.0 version but it fails during my build because it needs a very restricted version of com.google.common.base
There are also 6.x and 7.x versions, but they fail the build like this:
With both of these, resolution works:
So I guess that's what folks want? |
what did you do for normal guava/guice not to need errorprone |
I did nothing. All the dependencies come directly from Maven with the OSGi metadata as specified in the maven-hosted jar: https://github.com/eclipse-oomph/oomph.incubator/blob/master/maven/tp/Maven.target I think other places removed the package imports themselves or maybe with some gentle help from @HannesWell You can follow the links from the report: |
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
Yes, I made the error-prone dependency optional in guice via google/guice#1739, which is included in guice 6 and 7. For the extensions that still require the annotations, I also contributed proper metadata to error-prone itself via google/error-prone#3903, which is included in the recent error-prone 2.20.0 release. I assume one can use that as a drop-in replacement for 2.19, which guice requires.
Can't down-stream consumers add the extension to their target-platform by themself via Maven-Targets? |
Making it optional might have not the desired effect one do think, it should instead be scope |
is there such a thing in P2? |
P2 has nothing to do with maven, and as said "optional" in maven means something completely different than one might think:
this does not makes the dependency "optional" in the sense of that one don't need it to build the project, I think there is more a problem with einter BND, outdated BND or manual crafted manifests, as BND should already detect class only annotations and not include them in the manifest. |
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
Fixed in 2.32 |
Guice 7 does no more support javax.inject. |
i assume they can install old guices from old orbits |
At is listed in Guice 5.1.0, they fixed issues to support Java 17.
https://github.com/google/guice/wiki/Guice510
First step would be to bring it onto Eclipse Orbit.
If there is not yet such an activity already, I can do that receipt again.
@cdietrich a CQ is needed, i guess.
The text was updated successfully, but these errors were encountered: