antlr / antlr-php-runtime

PHP Runtime for ANTLR4
BSD 3-Clause "New" or "Revised" License
84 stars 20 forks source link

ATN serialized data: remove shifting by 2, remove UUID #17

Closed KvanTTT closed 2 years ago

marcospassos commented 2 years ago

@KvanTTT could you please reference what change motivated this PR for the record? I mean, what changed in the ANTLR, so it becomes necessary.

KvanTTT commented 2 years ago

Reference PR in the main repository https://github.com/antlr/antlr4/pull/3516 and the issue https://github.com/antlr/antlr4/issues/3515. Please let @parrt merge it because changes should be synched as I guess.

parrt commented 2 years ago

woohoo!

parrt commented 2 years ago

@KvanTTT looks like we're failing php in circlei due to ATN still. Did I mess up the merge?

https://app.circleci.com/pipelines/github/antlr/antlr4/1209/workflows/6ba0e51b-97f8-48be-b68c-19fd4c792dc0/jobs/14147

  TestSets>BaseRuntimeTest.testOne:182->BaseRuntimeTest.testParser:229->BaseRuntimeTest.assertCorrectOutput:430 [PHP:UnicodeEscapedBMPRangeSet] Parse output is incorrect: expectedOutput:<aáäáâåd
>; actualOutput:<>; expectedParseErrors:<>; actualParseErrors:<PHP Fatal error:  Uncaught InvalidArgumentException: Could not deserialize ATN with version 3 (expected 4). in /home/circleci/project/runtime/PHP/src/Atn/ATNDeserializer.php:128
Stack trace
KvanTTT commented 2 years ago

Now ANTLR master branch depends on the recent master branch of https://github.com/antlr/antlr-php-runtime with removed shifting by 2. A similar structure with master and dev branches should be introduced into antlr-php-runtime.

parrt commented 2 years ago

Ok, so i cut dev from master, reset hard master to 4.9.3 right? Heads up @marcospassos :)

marcospassos commented 2 years ago

Sorry, I just fell into this thread. Is it all about releasing a new version, or still are there things to do to get it working?

KvanTTT commented 2 years ago

Ok, so i cut dev from master, reset hard master to 4.9.3 right?

Yes, and set up dev in ANTLR for dev branch (leave master for master). Honestly, it looks a bit complicated. It seems we should manually fix CI files for every merge of dev into master.

marcospassos commented 2 years ago

Is working on two branches really needed?

marcospassos commented 2 years ago

v0.6.0 is out

parrt commented 2 years ago

Hi @marcospassos unfortunately we are now experiencing a disconnect between the latest stable release and development because target languages are pulling from github instead of a released zip/jar/binary or whatever. So we have changed the way ATN serialization works in the development branch, but the fact that we changed your repo and Go Target, for example, means that now the runtime will not work with they released tool (which is a jar that must be created and pushed out during release).

I'm open to suggestions. Maybe php users can pull from a specific tag that corresponds to the latest java tool jar?

KvanTTT commented 2 years ago

I'm wondering, could CI detects the branch it is being run on? If so, we can automatically detect the correct branch of antlr-php-runtime to pull during CI tests. This line: https://github.com/antlr/antlr4/blob/master/.circleci/scripts/install-linux-php.sh#L15

parrt commented 2 years ago

yeah was wondering about that. maybe that is simpler but php users will still need to ref a tag NOT the HEAD in github when install php antlr runtime, right? If @marcospassos is ok with that, we can change the php doc.

parrt commented 2 years ago

It looks like main java tool is now out of sync with php target repo.

marcospassos commented 2 years ago

I understand the problem now. Maybe we have better alternatives here: how about creating a tag, like latest, that always points to the latest release? This way, we don't need to change anything on the current workflow, and on the ANTLR repo, we only need a slight change to make it checkout the latest tag instead of the master branch.

If we decide to go this way, I'll probably install this action on the repo to handle the tag update automatically whenever I release a new version.

parrt commented 2 years ago

Hi! Well, I don't think the main repo actually pulls in the php repo even during a release so as long as the documentation is consistent for PHP users, then I think we are OK. The antlr repo can have the master and dev branch so that we can continue to deliver stable release from master but also develop things in dev that break runtimes in the master branch.

We'll need the CI settings so that we pull from master php but no big deal there. For the other targets, we will have the continuous integration pull from dev. We are currently doing this in the install script for CI:

git clone https://github.com/antlr/antlr-php-runtime.git runtime/PHP

Maybe we need to add a line that does

git checkout latest

??

marcospassos commented 2 years ago

Yeah, the point is that we can't just pull from the master if we keep developing it there. But no worry, I can adapt the workflow to keep the work going on the dev branch and the master pointing out the latest version.

marcospassos commented 2 years ago

Done!

parrt commented 2 years ago

CI tests appear to be passing now!!! woot