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

[CSharp] CI build of Antlr CSharp runtime gives warnings #3549

Closed
kaby76 opened this issue Feb 22, 2022 · 12 comments
Closed

[CSharp] CI build of Antlr CSharp runtime gives warnings #3549

kaby76 opened this issue Feb 22, 2022 · 12 comments

Comments

@kaby76
Copy link
Contributor

kaby76 commented Feb 22, 2022

I don't recall seeing this before, but under the "Files changed" tab for a PR, we now see warnings for legitimate build issues of the C# code.

Check warning on line 25 in runtime/CSharp/src/Tree/Xpath/XPathLexer.cs

GitHub Actions
/ build (csharp)
runtime/CSharp/src/Tree/Xpath/XPathLexer.cs#L25
The using directive for 'System' appeared previously in this namespace

See https://github.com/antlr/antlr4/pull/3547/files

It's caused because the grammar file XPathLexer.g4 includes an @header { using System;}. The @header is not needed. But, Char.IsUpper() should be fully qualified in order to eliminate the assumption that System is included. Honestly, the grammar shouldn't have target-specific actions in the code, and should be in "target-agnostic format". But, that's another story.

But, it gets better....

The XPathLexer.cs is generated by the Antlr4 tool. It says that it was generated using 4.9.3, but it was not! I don't know what tool was used, but it's not the official 4.9.3 Antlr tool! The tables produced are different.

@KvanTTT
Copy link
Member

KvanTTT commented Feb 22, 2022

It was generated using the tool from the actual dev branch. The serialization format is not compatible with 4.9.3, I can fix the warning.

@kaby76
Copy link
Contributor Author

kaby76 commented Feb 22, 2022

Well, the file says it's generated by 4.9.3, which is misleading.

And the instruction here say to actually hand-edit the version number in the damn generated file. Really?

I don't know why a modern .csproj isn't used, which would actually reference a specific tool version, or have the mvn tool build generate the file.

This is a generated file. It shouldn't even be checked in. This is exactly why I rewrote Antlr4BuildTasks, and why it's used in grammars-v4.

Yes, please fix this.

@KvanTTT
Copy link
Member

KvanTTT commented Feb 22, 2022

Well, the file says it's generated by 4.9.3, which is misleading.

Agree

And the instruction here say to actually hand-edit the version number in the damn generated file. Really?

I'll just add info about hand-edit. This file is changing not frequently.

This is a generated file. It shouldn't even be checked in. This is exactly why I rewrote Antlr4BuildTasks, and why it's used in grammars-v4.

Yes, this file should be autogenerated, but it's not my priority to integrate it into the build tool for now.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Feb 22, 2022 via email

@KvanTTT
Copy link
Member

KvanTTT commented Feb 22, 2022

Generating the lexer and parser on every build would add complexity to the build but 0 value to the users, since the grammar never changes.

What users do you mean? It simplifies the ANTLR developer experience if the ATN format or grammar is changing. Ordinary users just use compiled runtime.

@ericvergnaud
Copy link
Contributor

Complex build processes that break every now and then do not simplify developer experience.
I'd rather run a script to regenerate lexers and parsers once every 5 years (when serialization changes) than deal every week with a broken build.

@kaby76
Copy link
Contributor Author

kaby76 commented Feb 22, 2022

There are a lot of problems here.

  • The build is ad hoc and fragile.
    • There's no "script to regenerate lexers and parsers once every 5 years".
    • There are (now was) no instructions in a readme.md to say that you need to run the tool and regenerate the lexer code.
    • The "post-generation edits" says to actually edit the generated XPathLexer.cs file version number.
  • The Java code uses a hand-written XPathLexer.java.
  • The Cpp XPathLexer.g4 is different from the CSharp XPathLexer.g4.
  • There's no XPath engine for half the targets (Dart, Go, JavaScript, PHP, and Swift).
  • The XPath engine within the runtime is not even XPath version 1. It's basically useless for most applications and why I ported Eclipse's XPath2 engine to C# (so much more productive that I would compare this to writing your parser vs. using Antlr to generate a parser and for you to focus on the grammar).

Yes, fix the warning message in any way. But I would recommend after the next release or two to partition this XPath into a third party solution/repo that wraps the tree in an editable DOM and uses a modern, native XPath engine. Antlr4 should not be a monolithic system trying to solve every problem.

@KvanTTT
Copy link
Member

KvanTTT commented Feb 22, 2022

I support the idea of dropping XPath from ANTLR core at all since it's outdated and is not supported by some runtimes.

@KvanTTT
Copy link
Member

KvanTTT commented Feb 22, 2022

Also, it is not covered by tests except for Java.

KvanTTT added a commit to KvanTTT/antlr4 that referenced this issue Mar 2, 2022
@KvanTTT
Copy link
Member

KvanTTT commented Mar 7, 2022

@parrt it can be closed.

@parrt parrt closed this as completed Mar 7, 2022
@parrt
Copy link
Member

parrt commented Mar 7, 2022

(I regenerated the file)

@parrt
Copy link
Member

parrt commented Apr 1, 2022

I've done my best to regenerate these lexers. #3600 Ugh yeah, it's a mess but I hesitate to take it out of the API completely at this point. maybe we can add some tests after the release of 4.10

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

No branches or pull requests

4 participants