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

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

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

jamezp
Copy link
Contributor

@jamezp jamezp commented Jan 25, 2024

…ins to avoid repetition.

This is effectively a port of e5f23e8.

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.

@jim-krueger jim-krueger requested a review from spericas January 25, 2024 15:46
Copy link
Contributor

@jim-krueger jim-krueger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving but adding Santiago to review since he added the original commit referenced and James has added additional changes.

@jamezp
Copy link
Contributor Author

jamezp commented Jan 25, 2024

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
Copy link
Contributor Author

jamezp commented Jan 26, 2024

It seems like currently we need to keep that static dependency on JAXB. Just for reference this is discussed on #906 and #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.

Copy link
Contributor

@mkarg mkarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but I veto the replacement of my Copyright.

jaxrs-tck/pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@mkarg
Copy link
Contributor

mkarg commented Jan 27, 2024

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
Copy link
Contributor Author

jamezp commented Jan 29, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

jamezp commented Jan 31, 2024

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
Copy link
Contributor

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

@jamezp
Copy link
Contributor Author

jamezp commented Jan 31, 2024

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
Copy link
Contributor

mkarg commented Feb 3, 2024

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
Copy link
Contributor Author

jamezp commented Feb 5, 2024

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 #1211 (comment). 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
Copy link
Contributor

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
Copy link
Contributor Author

jamezp commented Feb 7, 2024

Moved back to Java SE 17 :)

@jim-krueger
Copy link
Contributor

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

@jim-krueger jim-krueger requested a review from mkarg February 8, 2024 16:37
pom.xml Outdated Show resolved Hide resolved
…ins to avoid repetition.

Signed-off-by: James R. Perkins <[email protected]>
@jim-krueger
Copy link
Contributor

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

@jim-krueger jim-krueger merged commit a7f5a0c into jakartaee:release-3.2 Feb 13, 2024
4 checks passed
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

Successfully merging this pull request may close these issues.

5 participants