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

Upgrade to ANTLR 4.10.1 (for the ANTLR 4.x series) #27298

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

Sanne
Copy link
Member

@Sanne Sanne commented Aug 15, 2022

This is in preparation of Hibernate ORM 6: we'll require this version, so might as well try to prepare for that, in case there's other projects that need to adapt.

N.B. versions v3 and v4 of ANTLR use a different package name, therefore this has no impact on users of ANTLR v3.x and previous.

@Sanne Sanne added area/hibernate-orm Hibernate ORM area/dependencies Pull requests that update a dependency file labels Aug 15, 2022
@quarkus-bot

This comment has been minimized.

@Sanne
Copy link
Member Author

Sanne commented Aug 16, 2022

@evacchi | @mariofusco | @lucamolteni hi :)

Looks like I'm going to need your help. Apparently org.kie:kie-dmn-core is depending on org.antlr:antlr4-runtime:jar:4.9.2:compile, but the generated code by ANTLR checks that the runtime actually matches exactly the version used at runtime.

Could you all upgrade to 4.10 ? In my experience it's a drop-in replacement, and that would solve the problem. This would be very useful to make progress with our experiments to migrate to Jakarta 10.

@quarkus-bot

This comment has been minimized.

@evacchi
Copy link
Contributor

evacchi commented Aug 16, 2022

fyi @tarilabs

@tarilabs
Copy link
Contributor

tarilabs commented Aug 16, 2022

hi @Sanne I might have limited ability to follow this through as I'm on PTO: Antlr4 is a bit delicate for DMN as it has some ties also with some GWT work, so I will be keeping 🤞 but worth giving it a shot, let's see what Jenkins says 🙃

Tracked with https://issues.redhat.com/browse/DROOLS-7105 and derivatives:

/cc @baldimir @danielezonca

@Sanne
Copy link
Member Author

Sanne commented Aug 17, 2022

we've merged a temporary workaround for Kogito in the quickstarts:

[Not opening an issue to get it cleaned up later as I'm sure we'll notice when Kogito will update]

With the quickstarts no longer breaking I think we can merge this - doing a last CI run.

@Sanne
Copy link
Member Author

Sanne commented Aug 17, 2022

damn this one is going to be very annoying:

Caused by: java.lang.UnsupportedOperationException: java.io.InvalidClassException: org.antlr.v4.runtime.atn.ATN; Could not deserialize ATN with version 3 (expected 4).
	at org.antlr.v4.runtime.atn.ATNDeserializer.deserialize(ATNDeserializer.java:56)
	at org.antlr.v4.runtime.atn.ATNDeserializer.deserialize(ATNDeserializer.java:48)
	at com.microsoft.sqlserver.jdbc.SQLServerLexer.<clinit>(SQLServerLexer.java:424)

Copy link
Member

@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.

With the quickstarts no longer breaking I think we can merge this - doing a last CI run.

@Sanne I disagree we can merge it. We need a clear plan for Kogito to upgrade before doing so as the Platform will use only one version and we can't expect Platform consumers to have to override the version locally.

So:

  • we need to make sure Kogito can upgrade
  • define in which version we upgrade and coordinate the upgrades to make sure the Platform is consistent

You might already have a plan but if so let's make it clear in this PR.

@Sanne
Copy link
Member Author

Sanne commented Aug 17, 2022

@gsmet sure I understand; my assumption is that the Kogito upgrade will be fairly simple, and it's going to be easier for them to test things if we do the change first - since it's a downstream project.

If it turns out it's a hell of a problem we can revert here; but it would be simpler to try to keep going so to identify other issues with it.

Already found a critical issue with the MSSQL JDBC driver so we might need to hold anyway. Wondering now why we didn't hit it in the ORM testsuite?! Seems related with native image's flow analsys, it seems the check is possibly in a code path not being triggered by the tests in Hibernate ORM.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Aug 17, 2022

since it's a downstream project

Kogito Quarkus is a downstream project, not Kogito. Kogito is a different upstream. Thus why we need to coordinate and make sure we upgrade together.

Given the SQL Server issue, let's make sure we can upgrade before pulling the trigger for Kogito too.

@tarilabs
Copy link
Contributor

FYI for as much as I can verify at present, on Drools DMN engine side we're ready to bump to Antlr 4.10 as you requested, ref this https://github.com/kiegroup/drools/pull/4614#issuecomment-1227070425 for the build errors in case let me know if I missed some CI-related considerations

@Sanne
Copy link
Member Author

Sanne commented Aug 25, 2022

@tarilabs awesome, thanks!

@gsmet may I consider this un-stuck? I'll rebase...

@Sanne
Copy link
Member Author

Sanne commented Sep 9, 2022

Rebased, and updated to 4.10.1 which came out in the meantime.

It seems runtime-compatible with 4.10 so I don't expect this would require strict alignment over 4.10 - however people on 4.9 still need to upgrade.

@Sanne Sanne changed the title Upgrade to ANTLR 4.10 (for the ANTLR 4.x series) Upgrade to ANTLR 4.10.1 (for the ANTLR 4.x series) Sep 9, 2022
@Sanne
Copy link
Member Author

Sanne commented Sep 9, 2022

@quarkus-bot

This comment has been minimized.

@knutwannheden
Copy link
Contributor

No idea, no.

@holly-cummins
Copy link
Contributor

I'm having fun with Pact and the Antlr upgrade too, @knutwannheden - we raised pact-foundation/pact-jvm#1615. It may be something a Pact Quarkus extension can fix, since Pact-Quarkus-Antlr seems to be a recurring compatibility issue (I spotted you raised pact-foundation/pact-jvm#1380 a while ago).

I've raised quarkiverse/quarkus-pact#1 to track it for the extension.

@holly-cummins
Copy link
Contributor

Latest version is now 4.11.1. We are using PACT which in its most recent version was regenerated with ANTLR 4.11.1. We will just stay on an older version for now.

Ouch, do you know if code built with 4.11 will be compatible at runtime with 4.10?

I think it will. The ATN changed from 3 (used by 4.9.2) to 4 (used by 4.11.1) in Antlr 4.10. So as I understand it, that means 4.10 should be compatible with 4.11.

@knutwannheden
Copy link
Contributor

@holly-cummins That sounds about right. Thanks for the heads up. I will look into the new extension and may be able to provide some feedback, since we've already used Pact consumer and provider tests in Quarkus projects for quite some time now.

@Sanne
Copy link
Member Author

Sanne commented Oct 28, 2022

apologies, I don't remember about the plan here. @gsmet is it suitable for merging now ?

Rebased to get a fresh opinion from CI.

@quarkus-bot

This comment has been minimized.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 23, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Nov 23, 2022

Failing Jobs - Building af0973b

Status Name Step Failures Logs Raw logs
✔️ Gradle Tests - JDK 11
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 17 MacOS M1
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.QuarkusDevDependencyDevModeTest.main line 14 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@gsmet gsmet merged commit 7f4f141 into quarkusio:main Nov 23, 2022
@quarkus-bot quarkus-bot bot added this to the 2.15 - main milestone Nov 23, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 23, 2022
@rsvoboda
Copy link
Member

rsvoboda commented Nov 24, 2022

@Sanne Sanne deleted the ANTLR4.10 branch November 24, 2022 21:49
@Sanne
Copy link
Member Author

Sanne commented Nov 24, 2022

The error mentions KIE:

 Error: Class initialization of org.kie.dmn.feel.parser.feel11.FEEL_1_1Lexer failed.

We're aware that this will break KIE but agreed with the team to merge this anyway, they'll catch up and updat ANTLR on their side as well soon.
I wasn't aware of this test, sorry - I did prevent a similar problem here: https://github.com/quarkusio/quarkus-quickstarts/pull/1155/files

@Sanne
Copy link
Member Author

Sanne commented Nov 24, 2022

Actually that's exactly the same module that I had patched months ago to prevent this very issue. I know this worked, now I'm confused about why that would no longer be effective ?

@Sanne
Copy link
Member Author

Sanne commented Nov 24, 2022

I tried to build it all locally and kogito-dmn-quickstart is compiling fine to native; it's working because it's using the version I had enforced in quarkusio/quarkus-quickstarts#1155 .

I could use some suggestion as to why these versions wouldn't get applied on CI ; commenting here as well:

@tarilabs
Copy link
Contributor

Could it be that the CI failure pertains to another quickstart? 🤔 Sorry just wild guess based on comments as I can access only on mobile atm

@Sanne
Copy link
Member Author

Sanne commented Nov 25, 2022

@tarilabs you're right, I was blind yesterday night :) thanks. I'll send a PR shortly.

@tarilabs
Copy link
Contributor

thanks to you @Sanne , sorry I couldn't turn around faster here before you. Thanks again for the related PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/hibernate-orm Hibernate ORM area/jakarta triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants