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

Complete conversion of ANTLR Kotlin into ANTLR5 #15

Closed
wants to merge 4 commits into from

Conversation

ftomassetti
Copy link
Collaborator

@ftomassetti ftomassetti commented Jan 30, 2024

As part of this PR, I updated the copyright notices for all files coming from ANTLR Kotlin. I also updated the files coming from ANTLR4 to specify that the Copyright is extended to the present time.

I then renamed the directory containing the files imported by ANTLR Kotlin but not used, into antlr-kotlin-runtime-remnanets.

Then I renamed the packages used by the ANTLR Kotlin runtime into org.antlr.v5.kotlinruntime.

This PR fix #14 .

@ftomassetti ftomassetti changed the title Complete conversion of ANTLR Kotlin into ANTLR5 by updating copyright… Complete conversion of ANTLR Kotlin into ANTLR5 Jan 30, 2024
@ftomassetti ftomassetti force-pushed the chore/copyrightAndPackages branch 3 times, most recently from f83a6e4 to 9e08763 Compare January 30, 2024 10:30
@ftomassetti
Copy link
Collaborator Author

The php target seems to fail, but I do not see how I could have caused that

@kaby76
Copy link
Collaborator

kaby76 commented Jan 30, 2024

The php target seems to fail, but I do not see how I could have caused that

It couldn't find composer: "composer: command not found". https://github.com/antlr/antlr5/actions/runs/7709758501/job/21011740363#step:4:8

I always put basic sanity checks into the Github Action workflow file after every single toolchain install. I don't trust anything, anyone, ...., nothing at all, forever. So, after

- name: Setup PHP 8.2
, add a step to execute php and composer, do a "whereis", etc., crash on failure. Also, I'm not sure it's best to use version 2.22.0 for the PHP plugin, https://github.com/antlr/antlr5/blob/79211eaea4dfe74db7b59a851d30696ef76bee23/.github/workflows/hosted.yml#L269C13-L269C35 , when it says in the release notes for version 2.23.0 "PHP 8.2 is now stable on setup-php".

@ftomassetti
Copy link
Collaborator Author

Thank you Ken, but I do not understand how my changes caused that specific task to fail, as it seems it was passing before

@kaby76
Copy link
Collaborator

kaby76 commented Jan 30, 2024

Thank you Ken, but I do not understand how my changes caused that specific task to fail, as it seems it was passing before

This can happen when something in the GH Actions changes. Happens all the time over in grammars-v4, and it's a pain to understand why. Again, just add some basic sanity check to the workflow file, like this: https://github.com/antlr/grammars-v4/blob/1d96a08da2f5c2b00c3d7f025e8370f7118bbd3f/.github/workflows/main.yml#L69-L72 You'll need to add in a call to composer.

$ composer --version
Composer version 2.4.4 2022-10-27 14:39:29
01/30-07:22:48 ~/antlr5
$ which composer
/c/ProgramData/ComposerSetup/bin/composer
01/30-07:23:02 ~/antlr5
$ whereis composer
composer: /c/ProgramData/ComposerSetup/bin/composer /c/ProgramData/ComposerSetup/bin/composer.bat /c/ProgramData/ComposerSetup/bin/composer.phar
01/30-07:23:07 ~/antlr5
$

@ftomassetti
Copy link
Collaborator Author

Thank you Ken, but I do not understand how my changes caused that specific task to fail, as it seems it was passing before

This can happen when something in the GH Actions changes. Happens all the time over in grammars-v4, and it's a pain to understand why. Again, just add some basic sanity check to the workflow file, like this: https://github.com/antlr/grammars-v4/blob/1d96a08da2f5c2b00c3d7f025e8370f7118bbd3f/.github/workflows/main.yml#L69-L72 You'll need to add in a call to composer.

$ composer --version
Composer version 2.4.4 2022-10-27 14:39:29
01/30-07:22:48 ~/antlr5
$ which composer
/c/ProgramData/ComposerSetup/bin/composer
01/30-07:23:02 ~/antlr5
$ whereis composer
composer: /c/ProgramData/ComposerSetup/bin/composer /c/ProgramData/ComposerSetup/bin/composer.bat /c/ProgramData/ComposerSetup/bin/composer.phar
01/30-07:23:07 ~/antlr5
$

Ok, I just tried doing that. Thank you for the suggestion

@ftomassetti ftomassetti force-pushed the chore/copyrightAndPackages branch 3 times, most recently from 17c7040 to 67f90cc Compare January 30, 2024 14:52
@ftomassetti
Copy link
Collaborator Author

Now we have two failures:

  • the C# tests on MacOs 11 failed: Error: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
  • the PHP tests on MacOs 11 failed (but the PHP tests on other platforms passed). Apparently because composer was not installed. I tried updating the actions used

I am also wondering if we could just remove all these tests for all these targets, as we are not using them anyway

@KvanTTT
Copy link
Member

KvanTTT commented Jan 30, 2024

The php target seems to fail, but I do not see how I could have caused that

Let's remove other targets at first (#2) to get rid of issues like that.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Jan 31, 2024

Legacy targets are ready to be removed, I suggest you rebase this PR once #17 is merged.

@ericvergnaud
Copy link
Contributor

@ftomassetti now that legacy targets have been removed, rebasing should eliminate issues unrelated to this PR (except maybe kotlin on mac which seems to frequently fail @KvanTTT ?)

@KvanTTT
Copy link
Member

KvanTTT commented Feb 1, 2024

except maybe kotlin on mac which seems to frequently fail @KvanTTT ?

I think it's a problem of GitHub MacOS server, I'm not sure if we can fix it in someway (maybe just turn off testing on MacOS). Also, I'll try to improve Kotlin tests performance later, maybe it will help.

@ftomassetti
Copy link
Collaborator Author

Ok, the tests seems to pass (some of them still running), however there has been some interactions with changes occurred in the meantime, and this PR is doing several things: that and the number of files committed make it very hard to review. So I would rather close this PR and do 2 or 3 separate PRs that get easier to review

@ftomassetti
Copy link
Collaborator Author

First step in replacing this PR: #19

@ftomassetti
Copy link
Collaborator Author

The plan is to merge #20, and then ensures that everything in here is covered. When we check that, I will close this PR

@ftomassetti ftomassetti closed this Feb 3, 2024
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.

Update copyright message and package name
4 participants