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

Parrot parser not used with maven-compiler-plugin when configured as documented #1107

Closed
cstamas opened this issue Apr 30, 2020 · 16 comments
Closed
Assignees
Labels

Comments

@cstamas
Copy link

cstamas commented Apr 30, 2020

So if a the doco is followed to set up your Maven, the Parrot parser seems not used, see #1082

I believe this is worth documenting, as it causes HUGE discrepancy between what user reads on Groovy3 and whar Maven "eats" when configured as documented. Moreover, IDEs like Idea, when setup like this, does NOT behave as this (follows Groovy doco, does not need this system property to enable parrot parser).

Hence, this should be documented, on a very visible place. Plus: doco should get into detail how to setup maven compiler plugin to get "real" Groovy 3 as well.

@eric-milles
Copy link
Member

I'll definitely need some clarification on your use case. Parrot Parser is disabled by default in Eclipse IDE and this has been stated in the release notes for every release that includes the new parser. The Maven batch compiler was not released until Groovy 3 was released and the compiler artifact was designed and tested to use the Parrot Parser. Since live editing is not involved, there was no reason to disable the new parser. If you are not using Eclipse IDE or Maven builds, what is your situation where the new parser is expected but is not appearing to be used?

@cstamas
Copy link
Author

cstamas commented Apr 30, 2020

Am using it as documented on this site, but try-with-resource does NOT compile (on CLI, using mvn clean install), just like in referenced issue. Moreover, it DOES compile if I append -Dgroovy.antlr4=true to my command line. Hence, this shows that when used with Maven only (so no IDE involved), it still needs this system property.

@cstamas
Copy link
Author

cstamas commented Apr 30, 2020

So here is an example project
https://github.com/cstamas/maven-eclipse-compiler-1107

Is basic archetype + groovy compiler

  • hopefully is configured right
  • but does not compile

I get error

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project maven-eclipse-compiler-1107: Compilation failure: Compilation failure: 
[ERROR] /home/cstamas/tmp/maven-with-groovy/maven-eclipse-compiler-1107/src/main/java/org/cstamas/test/App.groovy:[10,5] 
[ERROR] 1. ERROR in /home/cstamas/tmp/maven-with-groovy/maven-eclipse-compiler-1107/src/main/java/org/cstamas/test/App.groovy (at line 10)
[ERROR] 	try (FileInputStream fis = new FileInputStream('test')) {
[ERROR] 	    ^
[ERROR] Groovy:expecting '{', found '('
[ERROR] Found 1 error and 0 warnings.
[ERROR] -> [Help 1]

but, when I do mvn install -Dgroovy.antlr4=true, then it compiles just fine.

Am I doing something wrong?

@cstamas
Copy link
Author

cstamas commented Apr 30, 2020

And setting it for "real" (the thing I mention on OP) is quite cumbersome, as setting a system property from POM alone is not possible, unless you get an extra plugin, and use that.

Why not just plain compiler argument instead (that maven-compiler-plugin already supports)? Also, why this (IMO wrong) default when used from Maven?

@eric-milles
Copy link
Member

If you want to enable with no system property, you can use configScript option. Your script only needs one line:

configuration.pluginFactory = org.codehaus.groovy.control.ParserPluginFactory.antlr4()

I am re-checking the maven integration. It was supposed to work with no additional config, but has clearly failed.

@eric-milles eric-milles self-assigned this Apr 30, 2020
@cstamas
Copy link
Author

cstamas commented Apr 30, 2020

Thanks @eric-milles
Am fine for now with properties-maven-plugin doing it, but clearly was surprised with CLI Maven results, AND would be delighted to remove this "workaround" as well (as just adds bloat to project).

@eric-milles
Copy link
Member

The long term goal is to support recovery for the new parser and at that point there will be no reason to disable it by default.

@kriegaex
Copy link

kriegaex commented Jul 5, 2020

I am commenting here instead of in my duplicate #1137, in order to return something to the helpful community and hopefully for the benefit of others stumbling upon this problem too. Here is how I configure my IDE IntelliJ IDEA to use -Dgroovy.antlr4=true for both Maven and IDEA builds:

image

image

I do agree with @cstamas though: It should be a compiler option I can set directly in my Maven POM via Maven Compiler plugin.

@eric-milles eric-milles added this to the v3.10.0 milestone Sep 17, 2020
@brandones
Copy link

@cstamas Could you explain how you've used the properties-maven-plugin to set groovy.antlr4? Seeing a snippet of POM would be incredibly helpful.

@cstamas
Copy link
Author

cstamas commented Nov 20, 2020

Howdy, of course:

        <!-- Set system properties -->
        <plugin>
          <groupId>org.codehaus.mojo</groupId>
          <artifactId>properties-maven-plugin</artifactId>
          <version>1.0.0</version>
          <executions>
            <execution>
              <goals>
                <goal>set-system-properties</goal>
              </goals>
              <configuration>
                <properties>
                  <!-- Make groovy-eclipse-compiler use Groovy 3 compiler -->
                  <property>
                    <name>groovy.antlr4</name>
                    <value>true</value>
                  </property>
                </properties>
              </configuration>
            </execution>
          </executions>
        </plugin>

@brandones
Copy link

Perfect, thanks so much!

@eric-milles eric-milles modified the milestones: v4.0.0, v4.1.0 Dec 30, 2020
@eric-milles eric-milles modified the milestones: v4.1.0, v4.2.0 Mar 17, 2021
@eric-milles
Copy link
Member

eric-milles commented Jun 3, 2021

A couple notes that might be useful:

  1. If using maven to compile, the forked mode of the compiler does indeed enable the Parrot Parser for Groovy 3+. This can be done by adding <maven.compiler.fork>true</maven.compiler.fork> to your properties element or <fork>true</fork> to your maven-compiler-plugin's configuration element.

  2. As noted earlier, you can also enable the Parrot Parser by adding a compiler config script. This can be done by adding <compilerArguments><configScript>config.groovy</configScript></compilerArguments> to your maven-compiler-plugin's configuration element. config.groovy should contain:

    configuration.pluginFactory = org.codehaus.groovy.control.ParserPluginFactory.antlr4()
  3. IntelliJ does not use the Main-Class as specified by the groovy-eclipse-batch jar. This means it bypasses the extra config that was put in place to have the batch compiler use Parrot Parser by default.

@kriegaex
Copy link

kriegaex commented Jun 6, 2021

Thanks for the Maven Compiler fork setting. It works nicely for me. So now in IntelliJ IDEA I no longer need to set the property in the Maven runner VM settings as described in #1107 (comment). For the Groovy-Eclipse compiler, I still need the property, though. I do not want to use a compiler config script, because that would be even more complicated and just be another extra IDE setting I need, i.e. nothing would improve that way with regard to my goal "simply import the Maven project and it works".

@eric-milles, I created IDEA-270925 in order to track this problem and get it fixed in IntelliJ IDEA.

On a side note: Setting the property in a main method which is meant to be used via CLI, while integrating clients normally do what IDEA does, i.e. to extend the class and call super(), is quite counter-intuitive for any tool integrator. Would it not be better to pull the logic for selecting the correct parser default (i.e. Parrot, if the property is unset) into the constructor? Then the problem would simply go away for all integrators and magically be fixed in IDEA and Maven Compiler, too. No extra compiler parameters, no sacred knowledge about the need to fork. Compiling in process is actually the preferable thing to do in most cases.

@mattventura
Copy link

I also ran into this problem and only found this issue after a lot of searching. I think it would make sense to just update the wiki's example POM or at least add a comment there. It's an easy fix once someone knows that the problem is that they need to enable the antlr4 parser, but as it stands you have to dig through the issues or old release notes to find it.

@kriegaex
Copy link

Actually, I am not sure why the idea from the last paragraph of my previous comment #1107 (comment) was not implemented yet. It would be simple and fix the problem in several use cases.

@eric-milles
Copy link
Member

@cstamas @mattventura I updated the docs to make specific mention of the fork setting: https://github.com/groovy/groovy-eclipse/wiki/Groovy-Eclipse-Maven-plugin#how-to-use-the-compiler-plugin---setting-up-the-pom

@kriegaex The design of this was to respect the system properties of a shared jvm. When forking the compiler process, it is safe enough to set the system property that enables antlr4 parser. It was unanticipated that IntelliJ uses a different entry point. The long-term goal is still to improve parser error recovery and then enable it by default -- as groovyc does.

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

No branches or pull requests

5 participants