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

Add Jakarta EE 8 support #53

Closed
wants to merge 2 commits into from
Closed

Add Jakarta EE 8 support #53

wants to merge 2 commits into from

Conversation

hantsy
Copy link

@hantsy hantsy commented Oct 2, 2021

@hantsy
Copy link
Author

hantsy commented Oct 2, 2021

fixes #52

Copy link
Contributor

@pelletier197 pelletier197 left a comment

Choose a reason for hiding this comment

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

Looks good, and will fix issues for many people, including me. Two questions for your

  • Should we update the readme to outline the existence of this classifier?
  • I'm not familiar with how this library is deployed, but will they need to do something special to deploy it ?

task copyJavaSources(type: Copy) {
into generatedSrc
from 'src/main/java'
filter { line -> line.replaceAll("javax\\.", "jakarta\\.") }
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a bit a funny hack to make it work. I don't mind much personally as this is rather temporary, but are we sure there are no changes between javax and jakarta outside the package name ?

Copy link
Author

Choose a reason for hiding this comment

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

I am sure for Java EE 8/Jakarta EE 9 developers there is no difference from the API level between Jakarta EE 8 and Jakarta EE 9, the biggest change is the namespace migrating to new jakarta.

@@ -60,6 +71,11 @@ task javadocJar(type: Jar, dependsOn: javadoc) {
from javadoc.destinationDir
}

jar {
// add jakarta classier for Jakarta EE 9
archiveClassifier = isJakarta ? "jakarta" : ""
Copy link
Contributor

Choose a reason for hiding this comment

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

is emtpy string the default value, or should it be null?

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure the default value of the classifier, I think empty string works.

@hantsy
Copy link
Author

hantsy commented Oct 30, 2021

@pelletier197 Thanks for reviewing my PRs.

But I need more help to polish this PR to make it more mature, as the check list in the original post.

Personally I hope to build a special jakarta version for Jakarta EE 9 , and keep the default version for Jakarta EE 8, but it seems the MavenPublishPlugin can not set the artifactId dynamically.

And if possible to add a new archive(jakarta) for Jakarta EE 9 besides the default archives(src, docs, jar, and a new jar with jakarta postfix in the name).

@sbilello
Copy link

sbilello commented Nov 2, 2021

Is there any update on this PR? So we can unblock: Netflix/dgs-framework#659

@hantsy
Copy link
Author

hantsy commented Nov 2, 2021

Another possible solution is maintaining a branch for Jakarta EE 8,and make the main/master for Jakarta EE 9. This is maybe the most common options to maintain multi versions.

@hantsy
Copy link
Author

hantsy commented Nov 2, 2021

@pelletier197 @sbilello Tried to create two branches(jakartaee8, jakartaee9) on my fork(not created PRs yet), the build.gradle looks more clear.

  • The jakartaee8 branch will need to sync to upstream jakartaee8, and this upstream project will maintain a jakartaee8 branch in future.( I need help to create such a jakartaee8 branch on this upstream)
  • The jakartaee9 branch just add postfix jakarta the artifact name to indicate the new namespace, which will be merged into the upstream master.

Looks more clear?

Hope the maintainer of this project can work together and decide the final solution. If this is acceptable, I will create two smaller PRs instead of this one to avoid the source code transforming process.

@sbilello
Copy link

sbilello commented Nov 3, 2021

If we can make a change that would make it clear and keep jakartaee8 working: it should allow any project to avoid dealing with runtime dependency conflicts. I guess it is a good workaround to avoid to upgrade jakartaee8 in the Spring Framework

@pelletier197
Copy link
Contributor

pelletier197 commented Nov 3, 2021

I'm personally completely okay with this and I would merge as is.

Unfortunately I'm not a maintainer of this repo, so even if we can merge it, I cannot deploy it. And the maintainers of the repo are not really reactive to PRs. I have not had any update on my recent PRs as well for a while.

I was personally able to use the library with Jakarta in spring. Adding Jakarta to your classpath should resolve this, but it is indeed a dirty workaround.

@bbakerman
Copy link
Member

Another possible solution is maintaining a branch for Jakarta EE 8,and make the main/master for Jakarta EE 9. This is maybe the most common options to maintain multi versions.

I think this is a much better option that gradle config that edits files at runtime. I think a branch is a much better option

@hantsy
Copy link
Author

hantsy commented Nov 25, 2021

I think this is a much better option that gradle config that edits files at runtime. I think a branch is a much better option

@bbakerman Described in this comment #53 (comment)
This is ready in my side, there is a jakartaee8 branch required to be created on this project,, thus I can create new PRs for different branches.

@bbakerman
Copy link
Member

I ended up making a PR and branch and released a new version

#54

https://github.com/graphql-java/graphql-java-extended-validation/releases/tag/17.0-hibernate-validator-6.2.0.Final

So we should be right to go. Please test that release (its in Maven now) and tell me if there are any problems

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.

None yet

4 participants