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

Keep Java SE 11 version as binary target for APIs #650

Closed
starksm64 opened this issue Feb 14, 2023 · 27 comments
Closed

Keep Java SE 11 version as binary target for APIs #650

starksm64 opened this issue Feb 14, 2023 · 27 comments
Milestone

Comments

@starksm64
Copy link
Contributor

Do we have any expectation that our base Java SE API requirements will be higher than Java SE 11? Currently SE 11 is sufficient, but there has been discussion around having Java SE 21 be the base for EE 11, and SE 17 would be the minimum runtime required version. I guess any reliance on SE 17 or higher would require a major CDI release.

@starksm64 starksm64 added this to the CDI 4.1/5.0 milestone Feb 14, 2023
@Ladicek
Copy link
Contributor

Ladicek commented Feb 14, 2023

I'm not aware of anything we'd wanna do that would require bumping to 17.

@manovotn
Copy link
Contributor

Unless we want to explicitly specify CDI behavior with some newer Java constructs (such as records), I can't think of anything forcing us to use newer JDK versions.

@hantsy
Copy link
Contributor

hantsy commented May 25, 2023

Jakarta EE 11 targets Java 21, CDI should consider new features in Java 21, such as Record.

@manovotn
Copy link
Contributor

Jakarta EE targets Java 21, ...

Jakarta 11?
Is that an official statement posted somewhere?
I am not against it, just checking as I am aware we need to align our min required version with what the platform uses.

@m-reza-rahman
Copy link

Official positioning here: https://jakarta.ee/about/meeting_minutes/marketing_committee/minutes-marketing-0112-2023.pdf

@hantsy
Copy link
Contributor

hantsy commented May 25, 2023

Jakarta 11?

yeah, Jakarta EE 11.

It(Java 21) is confirmed in this issue jakartaee/platform#553 (comment)

@Ladicek
Copy link
Contributor

Ladicek commented Jun 7, 2023

I think we agreed that the next version will be 4.1, which in my opinion prevents bumping the minimum Java version. We don't intend to do any significant API changes anyway, so I believe we can just stay on 11.

Records are a massive pain from CDI perspective, actually, because they are always final and therefore:

  • cannot be used to define normal scoped beans,
  • their methods cannot be intercepted or decorated,
  • there may be more.

@manovotn
Copy link
Contributor

manovotn commented Jun 7, 2023

which in my opinion prevents bumping the minimum Java version

TBF I don't think it prevents us from doing that; however, we should think about whether we want to do that.
Yes, we need to work correctly on 21 in order to run fine with all the other Jakarta specs and that shouldn't be an issue as it is mostly a collection of APIs. But since CDI is used outside of just Jakarta EE, we might want to keep the minimum for CDI lower. It's not uncommon to see projects move at a slower pace than Java releases do these days and going 11 -> 21 is a fairly huge leap after all
Just my 0.02$ 🤷
This is probably a good topic to discuss in one of the meetings as well

@Ladicek
Copy link
Contributor

Ladicek commented Jun 7, 2023

I know there is certain school of thought out there that considers bumping the class file version not a breaking change, which is totally beyond me. If we bump the minimum Java version, we should make it a major release in my opinion.

@manovotn
Copy link
Contributor

manovotn commented Jun 7, 2023

I know there is certain school of thought out there that considers bumping the class file version not a breaking change, which is totally beyond me. If we bump the minimum Java version, we should make it a major release in my opinion.

Just to be clear, my previous comment isn't meant to indicate that we should bump the JDK version. I am just not aware of any jakarta-specific restrictions on that WRT minor versus major version of the given spec.
In fact, I am rather in favor of keeping the version lower unless we somehow directly benefit from the upgrade. That way, more people can use CDI (likely outside of Jakarta itself).
Note that we can still state the limitations people can encounter in the spec WRT newer JDKs - such as mentioning java records for which we needn't do anything but we can still warn users that they are by definition restricted as beans (final class, no no-args constructor by default)

@mkouba
Copy link
Contributor

mkouba commented Jun 8, 2023

Hm, the spec can handle the new features (SE 21) but the API can still be compiled with 11, or?

@Ladicek
Copy link
Contributor

Ladicek commented Jun 8, 2023

Yeah, agree. The API doesn't need to use any new features, but the spec can specify the behavior. We could for example add something like the following.

Chapter Managed beans:

If a managed bean class is a record, it may not declare a normal scope. The container automatically detects the problem and treats it as a definition error.

Chapter Producer methods:

If the producer method return type is a record, the producer method may not declare a normal scope. The container automatically detects the problem and treats it as a definition error.

Chapter Producer fields:

If the producer field type is a record, the producer field may not declare a normal scope. The container automatically detects the problem and treats it as a definition error.

Chapter Unproxyable bean types:

Certain legal bean types cannot be proxied by the container:

  • ...
  • primitive types,
  • array types,
  • and records.

The modification to Unproxyable bean types covers interceptors and decorators. It would actually also cover normal scoped records, but that would mean normal scoped records are a deployment problem and I'm thinking definition error is perhaps better in this case.

@manovotn
Copy link
Contributor

manovotn commented Jun 8, 2023

If a managed bean class is a record, it may not declare a normal scope. The container automatically detects the problem and treats it as a definition error.

Note that statements like these will be untestable as TCK needs to have the same min. JDK (for instance 11) and as such won't know records.
We could OFC have a separate TCK module that holds newer ones but that would then require an adjustment to the definition of passing the TCKs which might be quite a hassle combined with the fact that we already have split TCKs in many ways.

@Ladicek
Copy link
Contributor

Ladicek commented Jun 8, 2023

Good point. We actually already have that problem in the Lang Model TCK, where we just don't test records at all :-)

But as far as I am concerned, we could bump the TCK to Java 17 (or 21) and leave the API on 11.

@manovotn
Copy link
Contributor

manovotn commented Jun 8, 2023

But as far as I am concerned, we could bump the TCK to Java 17 (or 21) and leave the API on 11.

Hmm, I am not sure you can do that.
If you want to claim min JDK version 11, you should allow any such (potential) impl to pass the TCK with that given JDK and that would no longer be possible. Or am I missing something?

@Ladicek
Copy link
Contributor

Ladicek commented Jun 8, 2023

Well, you couldn't run the TCK with Java 11, but you could run it with Java 17 even if the target runtime is built with Java 11, which would be kinda the point. You could claim compatibility with Java 17, but would still be able to run with Java 11.

It sounds weird, but also useful, to me at least.

@hantsy
Copy link
Contributor

hantsy commented Jun 8, 2023

If the producer field type is a record, the producer field may not declare a normal scope. The container automatically detects the problem and treats it as a definition error.

If possible to declare a record as Singleton, no proxy on the bean?

@Ladicek
Copy link
Contributor

Ladicek commented Jun 8, 2023

If possible to declare a record as Singleton, no proxy on the bean?

As far as I can tell, that would work -- if the bean is not intercepted or decorated.

@starksm64
Copy link
Contributor Author

If possible to declare a record as Singleton, no proxy on the bean?

As far as I can tell, that would work -- if the bean is not intercepted or decorated.

Maybe, but it still is not a very useful construct given that you have to deal with the zero arg constructor in the producer and how do you pass these arguments in? As seen by prototyping of what might be done in Validation, having generally useful records requires something like a proxy constructor via a qualifier at the injection site that has non-binding fields. A record is really more like an entity bean in terms of how it can be meaningfully produced.

It almost seems like a new @constructor qualifier annotation would be needed, but is that a useful pattern?

@Ladicek
Copy link
Contributor

Ladicek commented Feb 16, 2024

Yeah I wouldn't use records to declare CDI beans myself. Their usefulness on that front is very limited -- they basically only make sense for pseudo-scoped beans that are not intercepted nor decorated.

I wouldn't try to shoehorn records into normal scopes, interception and decoration. It's a lot of pain for very small gain.

@starksm64
Copy link
Contributor Author

The EL 6.0.0 API is going to be using Java SE 17 for its target version, so that means that the CDI API will have to increase its target version to 17. See the current build failure under SE 11 for #771. Since we have to do the deprecation step both the jakarta.enterprise.cdi-api and jakarta.enterprise.cdi-el-api artifacts expose EL api elements. Ideally, only the jakarta.enterprise.cdi-el-api would need an SE 17 target version.

@starksm64
Copy link
Contributor Author

Actually, we just need the build SDK to be 17. Both artifacts can use a target version of 11 even though the depend on a SE 17 artifact. That seems a little strange, but I guess the linkage to the class does not entail any preclude differing SE versions.

@manovotn
Copy link
Contributor

Actually, we just need the build SDK to be 17. Both artifacts can use a target version of 11 even though the depend on a SE 17 artifact. That seems a little strange, but I guess the linkage to the class does not entail any preclude differing SE versions.

Yea, this is pretty much what I discovered during Beta release of CDI API - I used JDK 17 but the compiler version in pom.xml is still set to 11 and that works just fine.

@Emily-Jiang
Copy link
Contributor

Actually, we just need the build SDK to be 17. Both artifacts can use a target version of 11 even though the depend on a SE 17 artifact. That seems a little strange, but I guess the linkage to the class does not entail any preclude differing SE versions.

Yea, this is pretty much what I discovered during Beta release of CDI API - I used JDK 17 but the compiler version in pom.xml is still set to 11 and that works just fine.

Should CDI 4.1 be better off with Java 17? Is there any use case that CDI 4.1 are used outside of EE 11 using Java 11? If it does, the EL bit is not working.

@starksm64
Copy link
Contributor Author

starksm64 commented Feb 23, 2024 via email

@Emily-Jiang
Copy link
Contributor

My question was about CDI 4.1 under Java 11 usage not about the standalone CDI 4.1 usage @starksm64 ?

@starksm64
Copy link
Contributor Author

starksm64 commented Feb 23, 2024 via email

@starksm64 starksm64 changed the title Base Java SE version to use in APIs Keep Java SE 11 version as binary target for APIs Feb 26, 2024
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

No branches or pull requests

7 participants