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

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

Merged
merged 1 commit into from
Feb 6, 2022

Conversation

KvanTTT
Copy link
Member

@KvanTTT KvanTTT commented Jan 30, 2022

No description provided.

@marcospassos
Copy link
Collaborator

@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
Copy link
Member Author

KvanTTT commented Jan 31, 2022

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

Copy link
Collaborator

@marcospassos marcospassos left a comment

Choose a reason for hiding this comment

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

Please let me know when it's good to merge.

@parrt parrt merged commit 12f51dc into antlr:master Feb 6, 2022
@parrt
Copy link
Member

parrt commented Feb 6, 2022

woohoo!

@KvanTTT KvanTTT deleted the atn-remove-uuid-and-shifting-by-2 branch February 6, 2022 21:23
@parrt
Copy link
Member

parrt commented Feb 10, 2022

@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
Copy link
Member Author

KvanTTT commented Feb 10, 2022

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
Copy link
Member

parrt commented Feb 10, 2022

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

@marcospassos
Copy link
Collaborator

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
Copy link
Member Author

KvanTTT commented Feb 10, 2022

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
Copy link
Collaborator

Is working on two branches really needed?

@marcospassos
Copy link
Collaborator

v0.6.0 is out

@parrt
Copy link
Member

parrt commented Feb 10, 2022

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
Copy link
Member Author

KvanTTT commented Feb 10, 2022

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
Copy link
Member

parrt commented Feb 10, 2022

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
Copy link
Member

parrt commented Feb 10, 2022

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

@marcospassos
Copy link
Collaborator

marcospassos commented Feb 10, 2022

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
Copy link
Member

parrt commented Feb 11, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

Done!

@parrt
Copy link
Member

parrt commented Feb 12, 2022

CI tests appear to be passing now!!! woot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants