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

jakarta.inject support #1383

Closed
cogman opened this issue Aug 20, 2020 · 58 comments
Closed

jakarta.inject support #1383

cogman opened this issue Aug 20, 2020 · 58 comments

Comments

@cogman
Copy link

cogman commented Aug 20, 2020

Jakarta is coming to cause headaches for all :)

It would be nice if Guice could support processing Jakarta inject annotations as well as javax inject annotations. That would prevent us from potentially having to write a cross injection system constructor like

@Inject
@jakarta.inject.Inject

Fortunately, they don't share a package and it looks like jakarta pretty much only changed the package name.

@tandr
Copy link

tandr commented Mar 9, 2021

That's not a small undertaking - whole library sprinkled with java.inject namespace. Thinking about it - would an acceptable solution to have 2 libraries - guice-javax and guice-jakarta , and make them coexists somehow? (thinking out loud)

@GedMarc
Copy link

GedMarc commented Mar 10, 2021

You can use the guiced-ee artifact for jakarta namespacing across the board

https://mvnrepository.com/artifact/com.guicedee.services/guice

@kashike
Copy link
Contributor

kashike commented May 24, 2021

Are there any plans to look into support for this?

@d135-1r43
Copy link

I would be willing to contribute, maybe even with a small team.

@onacit
Copy link

onacit commented Feb 9, 2022

@GedMarc Check this out, yo.

@Azimuts
Copy link

Azimuts commented Oct 14, 2022

Guice's greatest virtue is its agility and that it is a DI library by itself with no frills added. Other DI and CDI solutions are fullstack If you want to use more modern application servers ( or other solutiones....) , on many occasions are forced to do a full stack migration. In itself a migration to , for example, toSpring, Micronaut, Quarkus is a full stack migration generating complicated dependencies that can condition the evolution of the applications.....

Indirectly, not supporting this small functionality is forcing developers to do full stack migrations and it's not easy to get out of there! Reconsider!

@trintaaho
Copy link

We need this.

@csisy
Copy link

csisy commented Mar 17, 2023

Bump on this, still not resolved. Is there any workaround?

@xvik
Copy link

xvik commented Apr 13, 2023

I did an automatic repackage of guice 5.1.0 (with tomcat migration tool) and published it yesterday into maven central (as rc.1 for now). Repackaged version use jakarta.inject, jakarta.servlet and jakarta.persistence.

Conversion performed on source jars (loaded from maven central) which are compiled then, so sources and javadoc are aligned in maven central publication. Pom and manifest files were manually rebuild (to replace dependencies and fix OSGI declarations). Struts2 and spring modules are not included (I doubt someone needs it).

Those who interesterested, please help me test it.

About 3rd party libraries: there would, of course, be problems with 3rd party libraies using javax.inject annotations: migrated guice would not see these annotations causing errors (e.g. not found constructor because of wrong annotation). In my libs I simply replaced all javax.inject.Inject annotations with gucie native annotations (so it could work with both javax and jakarta guice versions).

@sameb
Copy link
Member

sameb commented Apr 21, 2023

FYI all, I'm going to take some time early next week to fix this for core Guice.

I am still against outright forking the servlet extension into the jakarta namespace though, and will gladly welcome (and encourage) any community members that want to fork the servlet extension, fix its warts, and become the "official owners" of it.

@sameb
Copy link
Member

sameb commented Apr 25, 2023

Hi folks -- I have some changes prepared to support this for core Guice. However, the Dagger folks mentioned that they were informed of some issues related to supporting both javax.inject & jakarta.inject at the same time -- see google/dagger#2058 (comment). The TL;DR as best I can make out is:

  • javax.inject (from the javax.inject:javax.inject:1 maven coordinates) is old and doesn't have a "Automatic Module Name"
  • Jakarta folks re-published the javax inject APIs with a module name under jakarta.inject:jakarta.inject-api:1.0.5, but then went ahead and changed the packages to jakarta.inject but kept the jakarta coordinates and just incremented the version, to: jakarta.inject:jakarta.inject-api:2.0.1.
  • As a result, folks wanting JPMS-compatibility cannot use the original javax.inject:javax.inject:1 artifact (because it has no module name), and cannot use the jakarta.inject:jakarta.inject-api:1.0.5 and jakarta.inject:jakarta.inject-api:2.0.1 at the same time, because they're the same thing artifact (just with different versions). This means that folks wanting JPMS compatibility cannot reference both javax.inject and jakarta.inject at the same time.

How much of an issue is this for folks? My plan is basically to keep the javax.inject:javax.inject:1 dependency, introduce a new jakarta.inject:jakarta.inject-api:2.0.1 dependency, and have Guice depend on both the javax.inject namespace & the jakarta.inject namespace. But IIUC, that may break strict JPMS compatibility.

I'm open to ideas from folks more familiar with Maven about how to handle this.

(@mcculls FYI)

@norrisjeremy
Copy link

Why attempt to support both namespaces simultaneously, as opposed to just declaring Guice version X.Y.Z only supports the jakarta.inject namespace only?
Are folks wanting to intermix usage of both namespaces simultaneously?

@sameb
Copy link
Member

sameb commented Apr 25, 2023

@norrisjeremy -- For one, Google has quite literally millions of files referencing javax.inject. It isn't feasible to just up and say "switch'm all to another package". I wouldn't want to demand that from any other users either. Even if a migration were to happen, it would need to be incremental -- we can't just say "switch your entire codebase in order to use the new version".

One possibility here is to introduce a separate published artifact with a -jpms-compat suffix, and that artifact could potentially shade the javax.inject references. A naive shade would produce some awkward APIs, where Binder (for example) would have toProvider(Class<? extends shaded.javax.inject.Provider>)-like APIs. (The toProvider methods in Binder are also a problem in general for supporting jakarta.inject, so maybe there's another solution that could deal with those too.)

@martin-g
Copy link

martin-g commented Apr 25, 2023 via email

@sameb
Copy link
Member

sameb commented Apr 25, 2023

If I were able to cajole folks into a new release of javax.inject (at the javax.inject maven coordinates) that included a module name, etc.. would that make these problems go away?

@hgschmie
Copy link

hgschmie commented May 2, 2023

@sameb planning to give 6.0.0-rc1 a whirl on the weekend. Do you have a timeline for the proposed jakarta-first 7.0.0 as well?

@sameb
Copy link
Member

sameb commented May 2, 2023

@hgschmie: Great! Note there's two relevant commits that didn't make it into rc1: 36ff122 & 4cea397. I'll cut a new rc2 for those this week.

Re: 7.0 timeline: I'm hoping in the next few days. I'm almost finished (I hope) on the necessary transformations to rewrite javax->jakarta (or strip where redundant) while pushing from our internal repo to github.

@Azimuts
Copy link

Azimuts commented May 3, 2023 via email

@sameb
Copy link
Member

sameb commented May 3, 2023

@Azimuts : That error is likely due to e68fab6. It is an intentional change to avoid leaks. You can pass an option to the persist module when installing to get the legacy behavior, if desired. I'll update the error message to indicate this, and mention it in the release notes when I create them.

copybara-service bot pushed a commit that referenced this issue May 4, 2023
… replace the rest with their jakarta variants.

This required upgrading some test dependencies, because the tests need some impls of the APIs which only exist in newer releases.

This also removes the struts2 extension from the maven & bazel build (although it keeps the now-unused source), because struts2 has no release that supports jakarta. For users that want to continue using the struts2 extension, please continue to use the Guice 6.0 release (instead of the 7.0+ line, which will only support the jakarta dependencies).

Fixes #1383.

(I'll cut the 6.0 candidates/releases from before this commit, and if we need to make further 6.0 changes, I'll manually merge those into the 6.0 branch.)

PiperOrigin-RevId: 528636271
@sameb
Copy link
Member

sameb commented May 4, 2023

FYI: #1726 will resolve this, after which I'll cut new RCs, and then shortly after that the actual releases.

I need to resolve a few issues with the PR, but it otherwise appears to work as expected.

Note: I am NOT updating the javax.annotation.Nullable references, because those don't seem necessary to change. I am updating javax.{inject,servlet,persistence}, though.

@ben-manes
Copy link

Note: I am NOT updating the javax.annotation.Nullable references, because those don't seem necessary to change.

I believe this type has the split package problem which makes it frowned upon due to the Java module system. That's not very common, but was expected to be a decade long transition for adoption anyway. You might consider using the annotation name instead of type like static analyzers have switched to.

@sameb
Copy link
Member

sameb commented May 4, 2023

@ben-manes: Guice's usage of javax.annotation.Nullable is solely just to annotate methods/params as themselves accepting or returning null values, for code documentation. For checking to see if an injected param can be null, we allow anything named "Nullable".

copybara-service bot pushed a commit that referenced this issue May 4, 2023
… replace the rest with their jakarta variants.

This required upgrading some test dependencies, because the tests need some impls of the APIs which only exist in newer releases.

This also removes the struts2 extension from the maven & bazel build (although it keeps the now-unused source), because struts2 has no release that supports jakarta. For users that want to continue using the struts2 extension, please continue to use the Guice 6.0 release (instead of the 7.0+ line, which will only support the jakarta dependencies).

Fixes #1383.

(I'll cut the 6.0 candidates/releases from before this commit, and if we need to make further 6.0 changes, I'll manually merge those into the 6.0 branch.)

PiperOrigin-RevId: 528636271
@ben-manes
Copy link

Oh great. I believe that is still not module safe, unfortunately. Guava migrated to the checker framework for a time, though I think regressed as they work towards jspecify. But this can probably be ignored as jspecify matures and should replace the usages, and module adoption is still fairly low to not be a blocker.

@sameb
Copy link
Member

sameb commented May 4, 2023

Hi everyone. I've pushed a 7.0.0-rc1 and a 6.0.0-rc2. The 7.0 is jakarta only, and the 6.0 is javax+jakarta (to the extent that supporting jakarta w/ javax is possible). (Note that it may take a couple hours before the releases are mirrored into maven central.)

Please try them out and let me know how they work for you.

I would like to cut proper releases soon, but I need feedback from folks that they work as expected.

(I'll write proper release notes pages for both 6.0 & 7.0 tomorrow, as well as email the user list about the RCs.)

@cdietrich
Copy link

i see this regression in 7 @sameb : #1729

@sameb
Copy link
Member

sameb commented May 5, 2023

Thanks for giving it a spin, @cdietrich, and glad to hear the regression was just a mistaken javax import.

All: I posted release notes & made a proper announcement post @ #1732. Please follow up there with your success or failure stories in adopting Guice 6.0 or 7.0. Thank you!

@mkurz
Copy link
Contributor

mkurz commented May 8, 2023

6.0.0-rc2 works with the Play Framework and we do have quite some testing going on. I didn't change anything besides bumping the guice version (keep using javax.inject):

7.0.0-rc1 works as well also, switched everything to the jakarta namespace: playframework/playframework#11792
On GitHub the build fails, but this is because other (Play) dependencies didn't switch to Jakarta yet. Therefore I build those dependencies on my machine which also made all the tests pass locally 👍

@hgschmie
Copy link

hgschmie commented May 8, 2023

We will be shipping support for both javax.inject (guice 5.x-6.x) and jakarta.inject (guice 7.x) in the next release of https://github.com/jdbi/jdbi. Testing looks good; ideally we could use the releases as base lines; everything works so far with the release candidates.

@ben-manes
Copy link

RestEasy dropped support for Guice when they migrated to Jakarta. That used to be the more popular JAX-RS framework to use with Guice since Jersey 2 had buggy integration due lifecycle clashes with HK2. I'm hoping they'll restore this module (RESTEASY-3329).

@GedMarc
Copy link

GedMarc commented May 9, 2023

@ben-manes they also dropped any future support for any modules in their entirety - by separating out the spi's,

Rest Easy is not compatible with modular development, Guice is a modular DI.
I would not hold my breath for that, intentional move by rest easy. Also, it's not future compliant in any way, zero loss. A rest handler and api isn't required in any JDK 9 and up In any shape or form

@sameb
Copy link
Member

sameb commented May 9, 2023

Thanks all, this is great to hear that things are working in your environments. If it's possible to continue the discussion on the release announcement @ #1732, it's a little easier to keep track of the one discussion post instead of comments inside a closed issue.

hanwen pushed a commit to GerritCodeReview/gerrit that referenced this issue Oct 2, 2023
Guice 6.x release is adding support for jakarta.inject, see this issue
for more details: [1]. Guice release 7.0.0 will remove support for
javax.inject annotation and only support jakarta.inject.

Also add new dependency to jakarta.inject library.

[1] google/guice#1383

Release-Notes: Update guice version to 6.0.0
Change-Id: I0eb5320289bde532ac57bdd376802b37c1cee47f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet