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

Downgrade antlr to 4.8 to match Nessie #48

Merged
merged 1 commit into from
Oct 21, 2021

Conversation

omarsmak
Copy link
Contributor

@omarsmak omarsmak commented Oct 20, 2021

Downgrade antlr version in CEL library to match what we have in Nessie (4.8). In Nessie build logs I saw some warnings and errors related to the version mismatch, e.g:

2021-10-20T13:46:37.3349023Z [ERROR] ANTLR Tool version 4.9.2 used for code generation does not match the current runtime version 4.8
2021-10-20T13:46:37.3352032Z [ERROR] ANTLR Runtime version 4.9.2 used for parser compilation does not match the current runtime version 4.8
2021-10-20T13:46:37.3749587Z [ERROR] ANTLR Tool version 
2021-10-20T13:46:37.3754222Z [ERROR] 4.9.2
2021-10-20T13:46:37.3759485Z [ERROR]  used for code generation does not match the current runtime version 4.8
2021-10-20T13:46:37.3769785Z [ERROR] ANTLR Runtime version 4.9.2 used for parser compilation does not match the current runtime version 4.8

And it seems we can't upgrade antlr in Nessie since it needs to match antlr version in Spark as it is using 4.8 as well per this PR

cc @snazy

@snazy
Copy link
Member

snazy commented Oct 20, 2021

I'm okay to downgrade it.
Feel free to merge #49 if CI looks good and rebase this one.

@omarsmak
Copy link
Contributor Author

I'm okay to downgrade it. Feel free to merge #49 if CI looks good and rebase this one.

It seems I have no enough permissions on this repo to merge

@laurentgo
Copy link

Is Antlr a requirement from the CEL format? Could it be possible to switch to a model where we don't depend on some specific runtime for code generation (which happens at compile time?)

@laurentgo laurentgo closed this Oct 20, 2021
@laurentgo laurentgo reopened this Oct 20, 2021
@laurentgo
Copy link

(Sorry, closed by mistake, reopening)

@omarsmak omarsmak requested a review from snazy October 21, 2021 07:43
@codecov
Copy link

codecov bot commented Oct 21, 2021

Codecov Report

Merging #48 (9c16819) into main (47041bd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main      #48   +/-   ##
=========================================
  Coverage     79.92%   79.92%           
  Complexity     1753     1753           
=========================================
  Files            97       97           
  Lines          7611     7611           
  Branches       1117     1117           
=========================================
  Hits           6083     6083           
  Misses         1021     1021           
  Partials        507      507           
Flag Coverage Δ
java 79.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47041bd...9c16819. Read the comment docs.

@snazy snazy merged commit 77ae5a9 into projectnessie:main Oct 21, 2021
@omarsmak omarsmak deleted the downgrade-antlr branch October 21, 2021 08:40
@snazy
Copy link
Member

snazy commented Oct 21, 2021

Is Antlr a requirement from the CEL format? Could it be possible to switch to a model where we don't depend on some specific runtime for code generation (which happens at compile time?)

@laurentgo it's tricky. all the CEL syntax parsing is based on the existing antlr grammar. I think cel-java should just shade the antlr classes (#52).

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.

3 participants