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 - EE 10 - Support CDI 4.0 in ArC #27574

Closed
gsmet opened this issue Aug 29, 2022 · 12 comments
Closed

Jakarta - EE 10 - Support CDI 4.0 in ArC #27574

gsmet opened this issue Aug 29, 2022 · 12 comments
Assignees
Labels
area/arc Issue related to ARC (dependency injection) area/jakarta

Comments

@gsmet
Copy link
Member

gsmet commented Aug 29, 2022

CDI 4.0.1 introduces some new methods and classes and we will need to implement them in ArC.

How to reproduce:

  • get the jakarta-rewrite branch from the upstream repository (this branch is already transformed to Jakarta) - it is rewritten daily (but these parts are not moving parts so it shouldn't be a problem)
  • mvn -Dquickly
  • cd independent-projects/arc
  • change version.cdi to 4.0.1 in pom.xml
  • mvn clean install

First failures will be:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project arc: Compilation failure: Compilation failure:
[ERROR] /data/home/gsmet/git/quarkus-jakarta/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ArcCDIProvider.java:[32,12] io.quarkus.arc.impl.ArcCDIProvider.ArcCDI is not abstract and does not override abstract method handles() in jakarta.enterprise.inject.Instance
[ERROR] /data/home/gsmet/git/quarkus-jakarta/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/InstanceImpl.java:[36,8] io.quarkus.arc.impl.InstanceImpl is not abstract and does not override abstract method handles() in jakarta.enterprise.inject.Instance
[ERROR] /data/home/gsmet/git/quarkus-jakarta/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/InstanceImpl.java:[155,40] handles() in io.quarkus.arc.impl.InstanceImpl cannot implement handles() in jakarta.enterprise.inject.Instance
[ERROR] return type java.lang.Iterable<io.quarkus.arc.InstanceHandle> is not compatible with java.lang.Iterable<? extends jakarta.enterprise.inject.Instance.Handle>
[ERROR] /data/home/gsmet/git/quarkus-jakarta/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/InstanceImpl.java:[150,30] getHandle() in io.quarkus.arc.impl.InstanceImpl cannot implement getHandle() in jakarta.enterprise.inject.Instance
[ERROR] return type io.quarkus.arc.InstanceHandle is not compatible with jakarta.enterprise.inject.Instance.Handle
[ERROR] /data/home/gsmet/git/quarkus-jakarta/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/InjectableInstance.java:[18,33] handles() in io.quarkus.arc.InjectableInstance clashes with handles() in jakarta.enterprise.inject.Instance
[ERROR] return type java.lang.Iterable<io.quarkus.arc.InstanceHandle> is not compatible with java.lang.Iterable<? extends jakarta.enterprise.inject.Instance.Handle>
[ERROR] /data/home/gsmet/git/quarkus-jakarta/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/InjectableInstance.java:[16,23] getHandle() in io.quarkus.arc.InjectableInstance clashes with getHandle() in jakarta.enterprise.inject.Instance
[ERROR] return type io.quarkus.arc.InstanceHandle is not compatible with jakarta.enterprise.inject.Instance.Handle

I think the easiest way to work on that would be to create a branch in one's fork and push a commit on top of the jakarta-rewrite branch.

Then I would get it and apply it on top of the transformation each night. And we would have an error if the patch doesn't apply cleanly but I don't think these areas change a lot. (I will handle that part)

Ideally, the version.cdi upgrade will be done in a separate first commit as I think I will rely on OpenRewrite to change the version so that everything is centralized.

Hopefully, we will be able to make these changes without having to change other parts of the source code (at least as a first step). One can dream :).

@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Aug 29, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 29, 2022

/cc @manovotn, @mkouba

@manovotn
Copy link
Contributor

Will take a look; there should be little changes needed to make the existing code compile and work.
Bigger changes would be, for instance, support for build compatible extensions which I think @Ladicek has a prototype of, but those can come at a later point.

@gsmet
Copy link
Member Author

gsmet commented Aug 29, 2022

Yes exactly, let's focus on getting things to compile, we can add new features later.

@Ladicek
Copy link
Contributor

Ladicek commented Aug 29, 2022

I have all the necessary changes for ArC to compile with CDI 4.0 in a private fork, so I can help, @manovotn let me know if you want (or if you're busy and would prefer me to take this over).

(Indeed I also have an implementation of Build Compatible Extensions, but that is currently waiting for me to finalize #26264.)

@gsmet
Copy link
Member Author

gsmet commented Aug 29, 2022

OK, so I thought a bit about it and the best outcome would be a branch named jakarta-10-cdi in the main repo with:

  • one commit to upgrade the versions in pom files (I won't take this commit)
  • one commit that actually adjust the APIs, I will apply this one on top of the changes (I will include the version upgrades in the Open API recipes)

As for further developments, we will see what is best. Not sure it's a good idea to drop enormous changes with this sort of setups and maybe we should wait for the actual merge. But it's open to discussion.

@manovotn
Copy link
Contributor

@Ladicek I can do it but if you already have the changes, I suppose I would just be duplicating your work. Feel free to go ahead and draft a PR if you have the time for it.

@Ladicek Ladicek assigned Ladicek and unassigned manovotn Aug 30, 2022
@Ladicek
Copy link
Contributor

Ladicek commented Aug 30, 2022

Good, assigned to myself. I should have a PR later today or tomorrow -- thanks @gsmet for detailed instructions :-)

@Ladicek
Copy link
Contributor

Ladicek commented Aug 31, 2022

Sorry it took me a bit long -- I got distracted with another ArC problem :-) Here's a branch on top of jakarta-rewrite: https://github.com/Ladicek/quarkus/commits/jakarta-arc It has one commit that adjusts dependency versions in ArC's top-level pom.xml, and another commit that adjusts the code. Does that work?

(I didn't add my code to implement Build Compatible Extensions. I agree that should go in after we merge the Jakarta effort to main.)

@gsmet
Copy link
Member Author

gsmet commented Aug 31, 2022

Exactly what I was expecting, thanks. Can you push it to the main repository as jakarta-10-cdi? It's going to be easier for me and I prefer having the branches we are relying on in the main repo rather than spread across various forks.

I'll work on including it in the jakarta-rewrite branch once it's available.

Thanks!

@Ladicek
Copy link
Contributor

Ladicek commented Aug 31, 2022

I wasn't sure I have permissions for that, but it seems I do. It's in jakarta-10-cdi branch in the main repo now.

@gsmet
Copy link
Member Author

gsmet commented Aug 31, 2022

Awesome, thanks. I will all let you know when it's incorporated.

@gsmet gsmet moved this from Todo to In Progress in Quarkus Roadmap/Planning Aug 31, 2022
@gsmet
Copy link
Member Author

gsmet commented Sep 1, 2022

It has been incorporated here: #27651 .

I had the first report this morning and I created: smallrye/smallrye-graphql#1531 . I think it's the only issue we have.

Thanks everyone.

@gsmet gsmet closed this as completed Sep 1, 2022
Repository owner moved this from In Progress to Done in Quarkus Roadmap/Planning Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/jakarta
Projects
Status: Done
Development

No branches or pull requests

3 participants