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

IPROTO-239 Use correct generated class #168

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

fax4ever
Copy link
Contributor

@fax4ever fax4ever requested review from pruivo and karesti July 29, 2022 16:51
@fax4ever fax4ever force-pushed the IPROTO-239-generated branch from 4158289 to 45a7954 Compare July 29, 2022 16:59
@pruivo
Copy link
Member

pruivo commented Jul 29, 2022

@fax4ever the minimum JDK version is 11. Can't we just use the JDK9+ annotation all the time (and probably update the annotation-api dependency)?

@gsmet
Copy link

gsmet commented Jul 29, 2022

@pruivo we talked about it with Katia but the README seems to indicate you still support a JDK 8 runtime for protostream: Target runtime platform is Java 8.. But maybe it hasn't been updated.

Copy link

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Should fix the issue we have with Quarkus, thanks!

@pruivo
Copy link
Member

pruivo commented Jul 29, 2022

The README isn't updated yet (we forgot).
We changed the baseline to JDK 11 a while back for Protostream 4.5+

protostream/parent/pom.xml

Lines 103 to 104 in 9379923

<maven.compiler.target>11</maven.compiler.target>
<maven.compiler.source>11</maven.compiler.source>

@gsmet
Copy link

gsmet commented Jul 29, 2022

OK, well, in this case, we can just switch to the new annotation. That will simplify things for sure :).

And maybe update the README too.

@fax4ever fax4ever force-pushed the IPROTO-239-generated branch from 45a7954 to dffbd56 Compare July 31, 2022 16:46
@fax4ever fax4ever force-pushed the IPROTO-239-generated branch from dffbd56 to d8194d4 Compare July 31, 2022 16:51
@fax4ever
Copy link
Contributor Author

fax4ever commented Jul 31, 2022

Thanks @pruivo and @gsmet for the review.
I pushed the change to remove the annotation-api dependency, we don't need it anymore with JDK 11+.
I put aside the patch to support both JDK 8- and 11+, maybe we can use it if we need to backport the fix to some old ProtoStream version, that still need to support both JDK 8 and 11+.

@tristantarrant tristantarrant merged commit 182672a into infinispan:main Aug 1, 2022
@tristantarrant
Copy link
Member

Merged, thanks

@fax4ever fax4ever deleted the IPROTO-239-generated branch August 1, 2022 06:53
@fax4ever
Copy link
Contributor Author

fax4ever commented Aug 1, 2022

Thank you!

@gsmet
Copy link

gsmet commented Aug 1, 2022

Thanks!

@gsmet
Copy link

gsmet commented Aug 1, 2022

@fax4ever @tristantarrant @pruivo it would be awesome if you could include this in the next Infinispan 14 dev release!

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