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 ANTLRv4 Runtime to 4.11.1 #4845

Merged
merged 8 commits into from
Dec 25, 2022
Merged

Conversation

lkishalmi
Copy link
Contributor

Well this one is an interesting one. ANTLRv4 4.10 broke it's backward compatibility, so Parsers built with previous ANTLRv4 versions are no longer work with ANTLRv4 4.10+ runtimes, the Parsers needs to be re-generated with the newer tool.

I've checked our repository, before the recent addition of TOML and ANTLR grammars we have a Json grammar which needs to be rebuilt. (That's the second commit.) That module is friendly used some other places. The module is exporting the generated Laxer/Parser, the sigcheck fails, so in this commit the signature file is missing. Though at first test the source is compatible.

BTW, the whole upgrade story is originated from, that the TOML java library which is used for the TOML support, just got upgraded for the official TOML 1.0.0 specs. I wished if I just could update the library and put it in delivery, but the whole thing got broken when it turned out that the new TOML parser got generated with ANTLVv4 4.11.1.

I'm still investigating the dependencies, but I would like some feedback on the way I handled the Json Parser upgrade.

@lkishalmi lkishalmi added Upgrade Library Library (Dependency) Upgrade ANTLR labels Oct 22, 2022
@lkishalmi lkishalmi added this to the NB17 milestone Oct 22, 2022
@lkishalmi
Copy link
Contributor Author

I had to split the Json.g4 mixed parser into a lexer and a parser grammar, as lexer channels are no longer supported in mixed grammars. That resulted some glitch in the generated visitor and listener names.

@matthiasblaesing
Copy link
Contributor

This is major problem and this moves antlr for me into the corner of "you should not use it". This is not antlr 4.11, but 5.1 (4.10 broke compatibility). From my POV we have to treat it as such. At least we have to bump the release version, so the module would then be named org.netbeans.libs.antlr4.runtime/2. Or a new module is created, that holds versions 4.10+. I have seen antrl4 in the truffle area, so all depending modules need to be checked.

The changes to the JSON parser look sane to me. These cause unittest errors in javascript2.editor module, but these look harmless (Patch: javascript2.editor.zip).

The transitive problems seem to be much harder.

@lkishalmi
Copy link
Contributor Author

Checked the other dependencies. Surprisingly, it is just the JSON and its dependencties, TOML, and the ANTL support (those are leaf modules, so that'd be fine.) Groovy uses ANTLR, but it provides it's own runtime.
We have the CSS and a JavaCommand line lexer/parser in ANTLRv3, and some reverences on external ANTLR v2 libs in eclipselink and struts.
I have not found ANTLR v4 references in Truffle.

Agree on the release version /2 and generally that this should be ANTLR 5.1. But that's what it is.

Thanks for the patch!

@lkishalmi lkishalmi marked this pull request as ready for review October 31, 2022 03:30
@matthiasblaesing
Copy link
Contributor

I think this is missing the update of the TOML parser (at least some of the test failures seem to indicate this). What is more, the licenses need to be fixed. This should make verify-libs-and-licenses happy: fix_verify_libs_and_licenses.patch.zip. The patch can be applied with "git apply".

@lkishalmi
Copy link
Contributor Author

Yes, my TOML update is on a separate branch, I have to integrate that here.

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks sane to me. The commits might be squashed a bit, but that is IMHO optional.

@lkishalmi
Copy link
Contributor Author

Rebased on master.

@lkishalmi
Copy link
Contributor Author

lkishalmi commented Dec 23, 2022

@matthiasblaesing I've updated the release version and the sig file in the last commit. It seems the tomlj API has changed a bit. Also removed orj.tomlj.internal from the sigtest. Could you please, check if that commit makes sense to you! Thanks!

@matthiasblaesing
Copy link
Contributor

Looks sane to me. The test failure indicates, that the json parser module needs it sigfiles regenerated and most probably the major version bumped (uuncriticial as it is a friend-only export and it is in-tree).

@lkishalmi lkishalmi merged commit f58663d into apache:master Dec 25, 2022
pepness pushed a commit to pepness/incubator-netbeans that referenced this pull request Jan 9, 2023
* Upgrade ANTLR Runtime 4.11.1

* Moved the Json parser to ANTLRv4 4.11.1

* Added release version 2 to ANTLR v4 runtime.

* Re-generated ANTLR v3 and v4 grammars with 4.11

* TOML Suppoer update for TOML 1.0 (tomlj-1.1.0)

* Updated ANTLR License for 4.11.1

* Updated release version of libs.tomlj

* Generated new signature file for javascript2.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ANTLR Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants