-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Bump ANTLR from 4.8 to 4.9.2 #19551
Bump ANTLR from 4.8 to 4.9.2 #19551
Conversation
/cc @phillip-kruger @jmartisk not sure that GraphQL update is something for you because I saw graphql-java coming from |
@@ -75,7 +75,7 @@ | |||
<microprofile-graphql-api.version>1.1.0</microprofile-graphql-api.version> | |||
|
|||
<!-- Antlr is used by the PanacheQL parser--> | |||
<antlr.version>4.8</antlr.version> | |||
<antlr.version>4.9.2</antlr.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a duplicate? If it's in the BOM, it could probably be removed from the build-parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used for antlr4-maven-plugin
, so the BOM is not of help here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be moved to the root parent then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version property? So that there is only one property, also for the BOM? Or just for the plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, to have it in one place. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this should work, but then again it would be a paradigm shift because currently, all BOM-relevant properties are set only in the BOM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can discuss changing how we do things but that's for another day. For now, properties are either in the BOM or in the build-parent.
Maybe having all of them in the parent would indeed be an improvement but let's discuss this separately.
Failing Jobs - Building 54c14a2
Full information is available in the Build summary check run. Test Failures⚙️ Gradle Tests - JDK 11 Windows #📦 integration-tests/gradle✖
|
This removes warnings like:
AFAICS, the only one complaining now (in the reverse way) is GraphQL:
But seeing how little was changed in GraphQL 17.0 to use ANTLR 4.9.2, I'd say it should be ok: graphql-java/graphql-java#2323
Ideally, GraphQL should be updated in vertx-web-graphql and then updated in Quarkus.