Feature #10190
Prototype the changes to the build to add ANTLR v4
100%
Related issues
History
#1 Updated by Florin Eugen Rotaru about 1 year ago
- Status changed from New to WIP
I have added the dependency for antlr4 in the build.gradle:
fwdConvertServer group: 'org.antlr', name: 'antlr4', version: '4.13.2'
And a minimal task to generate the parser and the lexer associated with a grammar file.
task generateGrammar(type: AntlrTask) {
source = files("./src/com/goldencode/p2j/uast/progress.g4")
antlrClasspath = files([
'./build/lib/antlr4-4.13.2.jar',
'./build/lib/antlr4-runtime-4.13.2.jar',
'./build/lib/antlr-runtime-3.5.3.jar',
'./build/lib/ST4-4.3.4.jar',
])
outputDirectory = file("./src/com/goldencode/p2j/uast/antlr4")
doFirst {
outputDirectory.mkdirs()
}
}
If run by itself (in an empty script), this works and manages to generate the antlr classes. Otherwise this fails with this :
stacktrace
So it is probably a conflict (ow2.asm?).
I think the next steps are
- Solve this conflict
- Find a way to contract the needed jars for the antl4 in a single jar.
- Decide upon the destination directory for the new generated classes.
- Decide when to perform the antlr4 generation.
#2 Updated by Florin Eugen Rotaru about 1 year ago
Florin Eugen Rotaru wrote:
I think the next steps are
- Solve this conflict
I have managed to get the task generateGrammar task running by adding an if in the configurations.all {}
if (gradle.startParameter.taskNames[0] == 'generateGrammar') {
exclude group: 'org.ow2.asm'
}
#3 Updated by Tomasz Domin about 1 year ago
Florin Eugen Rotaru wrote:
So it is probably a conflict (
ow2.asm?).
Is ow2.asm direct or indirect antlr4 dependency ?
In task #9687 I am forcing org.ow2.asm:asm:9.8 with fwdServer group: 'org.ow2.asm', name: 'asm', version: '9.8'
Did you/Can you try upgrading ow2.asm instead of removing it ?
#4 Updated by Florin Eugen Rotaru about 1 year ago
Tomasz Domin wrote:
Florin Eugen Rotaru wrote:
Most likely, I don't see other cause for that dependency conflict.
Did you/Can you try upgrading ow2.asm instead of removing it ?
I did try doing that but it didn't help.
#5 Updated by Tomasz Domin about 1 year ago
Florin Eugen Rotaru wrote:
Tomasz Domin wrote:
Did you/Can you try upgrading ow2.asm instead of removing it ?
I did try doing that but it didn't help.
Ok, we'll try to sort that out later on, after 10190 or 9687 is in trunk
#6 Updated by Florin Eugen Rotaru about 1 year ago
Made an initial commit to 10190/15996:
- Defined new antlr4 configuration for Antlr-related JARs.
- Added logic to exclude
org.ow2.asmdependency when running antlr4All.
- Implemented
registerGrammarTaskmethod to registerAntlrTaskdynamically.- Avoids regenerating parser if
.g4and generated parser.javafile have identical timestamps.
- Avoids regenerating parser if
- Registered grammar generation tasks for:
Progress,ExpressionEvaluator,ExpressionCompiler,Schema,Text,Braces,Hql,Fql,E4gl.
- Added entry point antlr4All task that depends on all grammar tasks.
#7 Updated by Florin Eugen Rotaru about 1 year ago
- % Done changed from 0 to 20
- reviewer Greg Shah, Hynek Cihlar added
#8 Updated by Florin Eugen Rotaru about 1 year ago
- % Done changed from 20 to 40
Florin Eugen Rotaru wrote:
- Added logic to exclude
org.ow2.asmdependency when running antlr4All.
- Implemented
registerGrammarTaskmethod to registerAntlrTaskdynamically.
- Avoids regenerating parser if
.g4and generated parser.javafile have identical timestamps.
I decided to change the way we are generating the grammars. Instead of using the bundled AntlrTask from gradle(which was introducing the dependency conflict), we can manually call the ANTLR driver to parse the grammars:
def registerGrammarTask(String taskName, String grammarDir, String grammarName) {
tasks.register(taskName, JavaExec) {
def grammar = new File(grammarDir, grammarName + '.g4')
def parser = new File(grammarDir, grammarName + 'Parser.java')
if (upToDate(grammar, parser)) {
println "${grammarName} and parser files are up-to-date."
enabled = false
return
} else {
println "${grammarName} and parser files are NOT up-to-date."
}
classpath = configurations.antlr4
mainClass.set('org.antlr.v4.Tool')
args = [
'-visitor',
'-no-listener',
grammar.path
]
}
}
This solves the dependency conflict for now. I have also made this change to build.xml:
<target name="ant-antlr"
depends="ant-prepare
,ant-antlr_progress
,ant-antlr_expression_evaluator
,ant-antlr_text
,ant-antlr_braces
,ant-antlr_e4gl
,ant-antlr_schema
,ant-antlr_expression_compiler
,ant-antlr_hql
,ant-antlr_fql
"
unless="run-gradle-run"
description="Run ANTLR transformer against all updated grammars"/>
<target name="g-antlr" if="run-gradle-run">
<exec executable="${gradle.exec}" dir="." failonerror="true">
<arg line="ant-antlr ${propArg}"/>
</exec>
+ <exec executable="${gradle.exec}" dir="." failonerror="true">
+ <arg line="antlr4All ${propArg}"/>
+ </exec>
</target>
<target name="antlr" depends="check-gradle, g-antlr, ant-antlr"/>
I'm having some errors when compiling the generated classes though. We are either missing some jars or the compile.classpath has to be supplied differently. I will keep investigating.
#9 Updated by Greg Shah about 1 year ago
Instead of using the bundled AntlrTask from gradle(which was introducing the dependency conflict), we can manually call the ANTLR driver to parse the grammars:
I like it.
#10 Updated by Florin Eugen Rotaru about 1 year ago
- % Done changed from 40 to 100
- Status changed from WIP to Review
I have committed the changes to 10190/15998. I added the antlr4All registered in gradle into the ant-antrl target, so the generation of ANTLR4 files will be consistent to the other ANTLR.
I've also made sure that the generated classes are compiling.
As an initial idea, I've created the directories braces compiler e4gl evaluator fql hql progress schema text in ~/gcd/branches/10190a/src/com/goldencode/p2j/uast/antlr4. These files hold the grammar .g4 files and also the generated files.
Tomasz/Hynek: please review the build.xml and build.gradle changes.
Greg: please review the overall idea and let me know what should follow.
#11 Updated by Greg Shah about 1 year ago
- reviewer Tomasz Domin added
Code Review Task Branch 10190a Revisions 15996 through 15998
The overall approach looks good.
1. We will have separate grammars for lexers and parsers. So the code in registerGrammarTask() that looks for *Parser.java is not complete.
2. The whitespace change in ThinClient should be reverted.
3. For now, we have no .g4 grammars. The changes in 10190a have no negative impact in that case?
#12 Updated by Florin Eugen Rotaru about 1 year ago
Greg Shah wrote:
Code Review Task Branch 10190a Revisions 15996 through 15998
The overall approach looks good.
1. We will have separate grammars for lexers and parsers. So the code in
registerGrammarTask()that looks for*Parser.javais not complete.
Do you mean that we will have two separate .g4 files for the lexer and the parser?
3. For now, we have no
.g4grammars. The changes in 10190a have no negative impact in that case?
I actually created some trivial .g4 files for each grammar, such as:
grammar Braces;
@header {
package com.goldencode.p2j.uast.antlr4.braces;
}
start : EOF;
In order for the BUILD not to fail, we can either add them as placeholders or we can add ignoreExitValue = true in the gradle task.
#13 Updated by Greg Shah about 1 year ago
Florin Eugen Rotaru wrote:
Greg Shah wrote:
Code Review Task Branch 10190a Revisions 15996 through 15998
The overall approach looks good.
1. We will have separate grammars for lexers and parsers. So the code in
registerGrammarTask()that looks for*Parser.javais not complete.Do you mean that we will have two separate
.g4files for the lexer and the parser?
Yes
3. For now, we have no
.g4grammars. The changes in 10190a have no negative impact in that case?I actually created some trivial
.g4files for each grammar, such as:
[...]In order for the BUILD not to fail, we can either add them as placeholders or we can add
ignoreExitValue = truein the gradle task.
Use ignoreExitValue = true.
#14 Updated by Florin Eugen Rotaru about 1 year ago
OK, I made the two changes and committed to 10190a/16000.
#15 Updated by Florin Eugen Rotaru about 1 year ago
- Related to Feature #1757: update ANTLR to latest version added
#16 Updated by Greg Shah about 1 year ago
- Status changed from Review to WIP
- % Done changed from 100 to 90
Code Review Task Branch 10190a Revisions 15999 and 16000
The registerGrammarTask() isn't quite right. Not all grammars will be matched sets of lexer and parser. For example, the schema parser will use the progress lexer. Instead of checking both from one function, we should just have a single call to registerGrammarTask() for each .g4 file and we can pass a flag to tell it if this is a lexer or parser grammar.
#17 Updated by Florin Eugen Rotaru about 1 year ago
- Status changed from WIP to Review
- % Done changed from 90 to 100
Greg Shah wrote:
Code Review Task Branch 10190a Revisions 15999 and 16000
The
registerGrammarTask()isn't quite right. Not all grammars will be matched sets of lexer and parser. For example, the schema parser will use the progress lexer. Instead of checking both from one function, we should just have a single call toregisterGrammarTask()for each.g4file and we can pass a flag to tell it if this is a lexer or parser grammar.
I have committed the fix to 10190a/16001.
#18 Updated by Greg Shah about 1 year ago
Code Review Task Branch 10190a Revision 16001
The changes are good.
#19 Updated by Greg Shah about 1 year ago
- Status changed from Review to Internal Test
#20 Updated by Florin Eugen Rotaru about 1 year ago
Building with Java 1.8 fails because the currently ANTLR4 version needs at least Java 11.
To ensure compatibility we need to downgrade from ANTLR4.13.2 to ANTLR4.9.2.
#21 Updated by Florin Eugen Rotaru about 1 year ago
I have committed the needed change to 10190a/16002
#22 Updated by Greg Shah about 1 year ago
Florin Eugen Rotaru wrote:
Building with Java 1.8 fails because the currently ANTLR4 version needs at least Java 11.
To ensure compatibility we need to downgrade from ANTLR4.13.2 to ANTLR4.9.2.
No objection. Assuming this merges to trunk soon, then as part of #9687, we would want to move back to 4.13.2.
#23 Updated by Florin Eugen Rotaru 10 months ago
Besides the version compatibility tested for antlr4 and Java8, perhaps we should also test some conversions to make sure there are no conflicts between the antlr2 and the newly introduced antrl4 dependencies.
Other than that, I don't think the changes could have a negative impact on anything.
#25 Updated by Paula Păstrăguș 9 months ago
I've checked out the branch and I have a few questions regarding the current approach:
- There seems to be a small typo, when the grammar is not up to date, the log message displays two consecutive 'not' words:
./src/com/goldencode/p2j/uast/antlr4/progress/ProgressLexer.g4 is not NOT up-to-date. ./src/com/goldencode/p2j/uast/antlr4/progress/ProgressParser.g4 is not NOT up-to-date.
- What happens if a grammar file does not exist? Currently, the message still says that it’s "not up to date", but it might be more intuitive to log a specific message such as
Grammar file not found, skipping task $taskName.
- Is the Gradle task intended to support running only one grammar 'conversion'? For instance, if I want to regenerate a single grammar, should I run it as
./gradlew <taskName>? I added a test grammar for this purpose, but running it still triggers all grammars listed in theallAntlr4Tasksarray, which is not the expected behavior, I guess.
- Since
allAntlr4Tasksaggregates all grammars, wouldn’t it make sense to include only those that actually exist or were explicitly requested, to avoid unnecessary checks or task registrations?
If I’ve misunderstood something, please feel free to correct me, I’m still getting used to the IDE workflow and trying to align with how things are done here.
#26 Updated by Greg Shah 9 months ago
There seems to be a small typo, when the grammar is not up to date, the log message displays two consecutive 'not' words:
[...]
Good catch. It should be fixed.
What happens if a grammar file does not exist? Currently, the message still says that it’s "not up to date", but it might be more intuitive to log a specific message such as
Grammar file not found, skipping task $taskName.
Agreed.
Is the Gradle task intended to support running only one grammar 'conversion'? For instance, if I want to regenerate a single grammar, should I run it as
./gradlew <taskName>? I added a test grammar for this purpose, but running it still triggers all grammars listed in theallAntlr4Tasksarray, which is not the expected behavior, I guess.
Yes, I think it should be possible to convert only one grammar. The default behavior should be to convert those grammars that have changed.
Since
allAntlr4Tasksaggregates all grammars, wouldn’t it make sense to include only those that actually exist or were explicitly requested, to avoid unnecessary checks or task registrations?
Eventually they all will exist but I think the idea is that registerGrammarTask() will detect if they are changed and only add them in that case. I don't have any problem to add a check for a missing grammar, it is a good idea. Also, as you say, if the grammar was explicitly requested, it should be added.
#27 Updated by Paula Păstrăguș 9 months ago
- Status changed from Internal Test to Review
Committed as rev 16003.
Greg, please review.
#30 Updated by Paula Păstrăguș 7 months ago
I tested the build changes and everything appears to be in order. The only missing piece is the src/com/goldencode/p2j/uast/antlr4 directory, which I committed in revision 16004.
Currently we are using ANTLR 4.9.3. Since #9687 is now merged into trunk, should I also update the ANTLR runtime to 4.13.2?
#32 Updated by Paula Păstrăguș 7 months ago
Updated the antlr4 version to 4.13.2 in rev 16005.
#33 Updated by Paula Păstrăguș 7 months ago
I completed a full conversion of a large GUI application, and it passed successfully. I also performed smoke testing, and no issues surfaced (I made sure to include dynamic queries as well, if there are additional areas I should test, please let me know).
However, there is one thing that doesn’t seem quite right. When running the ant jar target, the message Grammar not found. Skipping xyz. is printed twice. The first occurrence appears here:
g-jar:
[exec] To honour the JVM settings for this build a single-use Daemon process will be forked. See https://docs.gradle.org/7.6.4/userguide/gradle_daemon.html#sec:disabling_the_daemon.
[exec] Daemon will be stopped at the end of the build
[exec]
[exec] > Configure project :
[exec] [ant:echo] propArg:
[exec] Grammar not found. Skipping antlr4ProgressLexer.
etc
etc
And after that, the logs are displayed from the ant-antlr4All target again.
#34 Updated by Hynek Cihlar 7 months ago
Paula Păstrăguș wrote:
I completed a full conversion of a large GUI application, and it passed successfully. I also performed smoke testing, and no issues surfaced (I made sure to include dynamic queries as well, if there are additional areas I should test, please let me know).
However, there is one thing that doesn’t seem quite right. When running the
ant jartarget, the messageGrammar not found. Skipping xyz.is printed twice.
The logic is probably executed during both the configuration and execution phases. Please execute the build with more verbose logging.
#35 Updated by Paula Păstrăguș 7 months ago
Here are the logs for ant jar --verbose:
#36 Updated by Paula Păstrăguș 7 months ago
Hynek / Tomasz, could you share your thoughts on the logs being printed twice? Is this expected behavior, or something we should look into?
#37 Updated by Hynek Cihlar 7 months ago
Paula, ant-antlr4All should be protected with run-gradle-run. Try to add unless="run-gradle-run" to it.
#38 Updated by Paula Păstrăguș 7 months ago
Thanks, that did the trick! Committed as rev 16006.
#40 Updated by Paula Păstrăguș 6 months ago
The testing is finished, at that time I also converted one big application to be sure the new build changes does not affect the conversion. Only if you want to also test some specifics?
#42 Updated by Paula Păstrăguș 6 months ago
No, I’ll rebase the branch right away and do some retesting.
#43 Updated by Paula Păstrăguș 6 months ago
10190a was rebased, new revision is 16375.
#44 Updated by Paula Păstrăguș 6 months ago
- Status changed from Internal Test to Review
Testing revealed that the -lib option is required for the schema task. Since the schema parser and the Progress lexer are located in different directories, the dependency must be explicitly provided so that ANTLR4 can resolve the generated lexer tokens. This has been fixed in revision 16376.
Greg, please review.
Aside from that failing case, the other cases works fine. Should I also let the big app conversion run overnight, given the new Gradle version?
#46 Updated by Paula Păstrăguș 6 months ago
Conversion completed successfully. I’ll also run some smoke tests on the converted application, and if everything looks good, we can proceed with the merge.
#47 Updated by Paula Păstrăguș 6 months ago
Testing has passed successfully.
Greg, I’ll leave the final approval for the merge to you. If you’d like me to double-check anything else, just let me know. Otherwise, I believe we’re good to proceed with the merge.
#49 Updated by Paula Păstrăguș 6 months ago
- Status changed from Merge Pending to Test
10190a was merged into trunk as rev 16375 and archived.
#50 Updated by Vladimir Tsichevski 6 months ago
Paula Păstrăguș wrote:
10190a was merged into trunk as rev
16375and archived.
Now I see the following in the build log, it this expected?
> Configure project : [ant:echo] propArg: Grammar not found. Skipping antlr4ProgressLexer. Grammar not found. Skipping antlr4ProgressParser. Grammar not found. Skipping antlr4ExpressionEvaluatorParser. Grammar not found. Skipping antlr4ExpressionEvaluatorLexer. Grammar not found. Skipping antlr4ExpressionCompilerParser. Grammar not found. Skipping antlr4ExpressionCompilerLexer. Grammar not found. Skipping antlr4TextParser. Grammar not found. Skipping antlr4TextLexer. Grammar not found. Skipping antlr4BracesParser. Grammar not found. Skipping antlr4BracesLexer. Grammar not found. Skipping antlr4HqlParser. Grammar not found. Skipping antlr4HqlLexer. Grammar not found. Skipping antlr4FqlParser. Grammar not found. Skipping antlr4FqlLexer. Grammar not found. Skipping antlr4E4glParser. Grammar not found. Skipping antlr4E4glLexer. Grammar not found. Skipping antlr4Schema.
Also, I found no description in this task explaining what is this feature about :-(
#51 Updated by Paula Păstrăguș 6 months ago
Vladimir Tsichevski wrote:
Paula Păstrăguș wrote:
10190a was merged into trunk as rev
16375and archived.Now I see the following in the build log, it this expected?
[...]
Yes, it's expected.
Also, I found no description in this task explaining what is this feature about :-(
Basically, I enabled Java code generation from ANTLR4 grammars, paving the way for the ANTLR2 to ANTLR4 migration.