jakartaee / rest

Jakarta RESTful Web Services
Other
351 stars 114 forks source link

Cleanup of pom files to use dependency management and dependency plug… #1211

Closed jamezp closed 4 months ago

jamezp commented 5 months ago

…ins to avoid repetition.

This is effectively a port of https://github.com/jakartaee/rest/commit/e5f23e8cb328aee010f9ea1dae85ec8186d7cf93.

One thing I did not do was remove the dependency on jakarta.xml.bind.api. I wasn't too clear on why that was needed.

This also sets the minimum JDK required and compiles to Java SE 17.

jamezp commented 5 months ago

Yes, I could use some clarification on why the jakarta.xml.binding.api was removed too. I also noticed I forgot to update the module-info for the API to include the requirement for the two CDI modules. We can do that now, or do it when we actually update the annotations to use CDI annotations.

jamezp commented 5 months ago

It seems like currently we need to keep that static dependency on JAXB. Just for reference this is discussed on https://github.com/jakartaee/rest/issues/906 and https://github.com/jakartaee/rest/issues/907. The latter makes it effectively optional. However, we still have a compile time dependency on it.

I did remove the references in the examples, which seems okay. I will wait to add the CDI dependencies on the module until we add the CDI annotations to the REST annotations.

mkarg commented 5 months ago

IIRC the target for 3.x originally was Java 11. This PR will set it to Java 17 (which I do like very much). But is that allowed in SemVer? I mean, downstreams that are not able to run in Java 17 yet would not be able to upgrade the 3.x line then.

jamezp commented 5 months ago

IIRC the target for 3.x originally was Java 11. This PR will set it to Java 17 (which I do like very much). But is that allowed in SemVer? I mean, downstreams that are not able to run in Java 17 yet would not be able to upgrade the 3.x line then.

That is a good question and I don't know the answer. I do know other Jakarta EE 10 specifications are moving to require Java SE 17. For now I've left this at 17, but if it's determined we cannot do this upgrade I will update the PR, or send a separate PR, to downgrade back to Java SE 11 as a mnimum.

jim-krueger commented 5 months ago

That is a good question and I don't know the answer. I do know other Jakarta EE 10 specifications are moving to require Java SE 17. For now I've left this at 17, but if it's determined we cannot do this upgrade I will update the PR, or send a separate PR, to downgrade back to Java SE 11 as a minimum.

I believe you meant ...other Jakarta EE 11 specifications.... and I posed this question in the JakartaEE Development slack channel and the replies seem to indicate that this change will not be a problem in a point release.

jamezp commented 5 months ago

That is a good question and I don't know the answer. I do know other Jakarta EE 10 specifications are moving to require Java SE 17. For now I've left this at 17, but if it's determined we cannot do this upgrade I will update the PR, or send a separate PR, to downgrade back to Java SE 11 as a minimum.

I believe you meant ...other Jakarta EE 11 specifications.... and I posed this question in the JakartaEE Development slack channel and the replies seem to indicate that this change will not be a problem in a point release.

Yes, I meant Jakarta EE 11, sorry about that typo :)

jim-krueger commented 5 months ago

@mkarg @spericas Can we get a couple more review approvals on this to get it in? Thanks

jamezp commented 5 months ago

Note that I changed the requirement of Java SE 17 back to Java SE 11 as we may not be able to change this in a minor version. If we find out we can, we can change it in a separate PR.

mkarg commented 5 months ago

Note that I changed the requirement of Java SE 17 back to Java SE 11 as we may not be able to change this in a minor version. If we find out we can, we can change it in a separate PR.

What is the benefit of 3.2 if it does only support Java 11? IIRC the sole idea of 3.2 was to support JDK 17.

jamezp commented 4 months ago

Note that I changed the requirement of Java SE 17 back to Java SE 11 as we may not be able to change this in a minor version. If we find out we can, we can change it in a separate PR.

What is the benefit of 3.2 if it does only support Java 11? IIRC the sole idea of 3.2 was to support JDK 17.

I personally don't see a benefit, but I thought you had concerns about it being allowed https://github.com/jakartaee/rest/pull/1211#issuecomment-1913088963. Maybe I misunderstood, but the idea was to not block this PR as we can upgrade the minimum JDK version later. We can't really progress with other changes until this is in without creating a bunch of conflicts.

jim-krueger commented 4 months ago

IIRC the target for 3.x originally was Java 11. This PR will set it to Java 17 (which I do like very much). But is that allowed in SemVer? I mean, downstreams that are not able to run in Java 17 yet would not be able to upgrade the 3.x line then.

Jakarta Rest-3.0 supported Java 8 and 3.1 moved to Java 11. So @jamezp, I would think you should be able to change back to your initial Java 17, Java 21 for 3.2. @Markus, are you ok with that, or am I missing something?

jamezp commented 4 months ago

Moved back to Java SE 17 :)

jim-krueger commented 4 months ago

@mkarg Can you please approve or express your concerns with this PR as it is holding up progress on 3.2. Thanks

jim-krueger commented 4 months ago

Merging as the number of approvers has been met and all review comments have been resolved.