javacc / javaccPlugin

A JavaCC plugin for Gradle
MIT License
33 stars 16 forks source link

Upgrade build to use Gradle 6.4 #51

Closed kpitt closed 3 years ago

kpitt commented 3 years ago

I recently started working on converting a rather large legacy build from Ant to Gradle, and I was in need of a few updates to the JavaCC plugin. The first step was to get the plugin build running on a more modern version of Gradle. I saw that this has been an outstanding item for this plugin for awhile, so I thought I would contribute my work in the hopes of maybe helping to get things moving forward again.

I saw that there was an existing release-3.0.0 branch, but I did not base my changes directly off that branch because those commits would have become part of the pull request against master. I did, however, cherry pick several of the changes and I believe I have covered all of the other updates in one way or another.

All of the unit and acceptance tests are passing when building with Java 8 and Gradle 6.4. I set the automated tests to run against Gradle 6.0.1 to try to maximize compatibility. I wasn't able to do a full test of all of the publishing tasks for obvious reasons, but I was able to publish successfully to the local Maven cache.

zosrothko commented 3 years ago

Hi, welcome to the JavaCC project and thanks for your contribution. There is a check failure on Travis CI. Could you check it and maybe fix it before I merge your pull request?

kpitt commented 3 years ago

Looks like Travis CI was unable to install the JDK. The error message gives the impression that they no longer support Java 8.

Installing oraclejdk8
$ export JAVA_HOME=~/oraclejdk8
$ export PATH="$JAVA_HOME/bin:$PATH"
$ ~/bin/install-jdk.sh --target "/home/travis/oraclejdk8" --workspace "/home/travis/.cache/install-jdk" --feature "8" --license "BCL"
Ignoring license option: BCL -- using GPLv2+CE by default
install-jdk.sh 2020-06-02
Expected feature release number in range of 9 to 17, but got: 8
The command "~/bin/install-jdk.sh --target "/home/travis/oraclejdk8" --workspace "/home/travis/.cache/install-jdk" --feature "8" --license "BCL"" failed and exited with 3 during .

I'll have to take a look at the Travis config and see what the options are. Unfortunately, it could be problematic if they no longer allow Java 8. The plugin tests use Powermock in a way that isn't compatible with the module access restrictions in Java 9+, and I already wasted quite a few hours trying to get them to work with Java 11 before giving up and falling back to Java 8. It certainly isn't the preferred mechanism for testing and it would be nice to eliminate that dependency, but I don't have a good feel right now for what it would take to accomplish the same results without Powermock.

codecov-commenter commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@48917fa). Click here to learn what that means. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #51   +/-   ##
=========================================
  Coverage          ?   87.73%           
  Complexity        ?      149           
=========================================
  Files             ?       23           
  Lines             ?      432           
  Branches          ?       28           
=========================================
  Hits              ?      379           
  Misses            ?       49           
  Partials          ?        4           
Impacted Files Coverage Δ
...gins/javacc/compiler/JavaccSourceFileCompiler.java 100.00% <ø> (ø)
.../javacc/programexecution/JavaccExecutorAction.java 100.00% <ø> (ø)
...s/javacc/programexecution/JjdocExecutorAction.java 100.00% <ø> (ø)
.../javacc/programexecution/JjtreeExecutorAction.java 100.00% <ø> (ø)
...ca/coglinc/gradle/plugins/javacc/JavaccPlugin.java 93.93% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 48917fa...079e931. Read the comment docs.

kpitt commented 3 years ago

I changed the config to use OpenJDK 8 instead of the Oracle JDK, and it looks like everything passed this time.

zosrothko commented 3 years ago

Looks like Travis CI was unable to install the JDK. The error message gives the impression that they no longer support Java 8.

Installing oraclejdk8
$ export JAVA_HOME=~/oraclejdk8
$ export PATH="$JAVA_HOME/bin:$PATH"
$ ~/bin/install-jdk.sh --target "/home/travis/oraclejdk8" --workspace "/home/travis/.cache/install-jdk" --feature "8" --license "BCL"
Ignoring license option: BCL -- using GPLv2+CE by default
install-jdk.sh 2020-06-02
Expected feature release number in range of 9 to 17, but got: 8
The command "~/bin/install-jdk.sh --target "/home/travis/oraclejdk8" --workspace "/home/travis/.cache/install-jdk" --feature "8" --license "BCL"" failed and exited with 3 during .

I'll have to take a look at the Travis config and see what the options are. Unfortunately, it could be problematic if they no longer allow Java 8. The plugin tests use Powermock in a way that isn't compatible with the module access restrictions in Java 9+, and I already wasted quite a few hours trying to get them to work with Java 11 before giving up and falling back to Java 8. It certainly isn't the preferred mechanism for testing and it would be nice to eliminate that dependency, but I don't have a good feel right now for what it would take to accomplish the same results without Powermock.

OK no problem to let it as it is as soon the Travis build is ok

zosrothko commented 3 years ago

Thanks for you time spent in solving the Travis stuff. Woudl you agree to be the maintainer of this plugin since no one among the traditional contributors of JavaCC are knowledgeable enough with Gradle to maintain this plugin?

kpitt commented 3 years ago

Well, I guess I can give it a try. I'm not sure I'll have a wealth of time to apply to it, but it looks like it has been almost 3 years since the last active development so hopefully I can give it a little more than that.

zosrothko commented 3 years ago

@kpitt Another important point missed. Could you update the README.md to reflect the change you made? TIA

kpitt commented 3 years ago

I updated the README template file. The build is currently set up to regenerate the actual README file from the template as part of the release process. I can’t say I’m a fan of that approach, but I didn’t want to do anything that would upset the process too much as part of this initial PR so I just followed the lead of what John had been doing in his original 3.0.0 branch.

About the only thing that the template does is automatically insert the correct plugin version numbers into the instructions for installing the plugin, so it definitely feels like an unnecessary extra step. If you’d like, I can submit another PR that moves everything directly into the README and eliminates the extra template step.

kpitt commented 3 years ago

I’m not sure what your plans are for releasing an update. Since 3.0.0 would be a major breaking release anyway, it might be a good time to consider a few other updates such as changing the plugin id to align with the JavaCC organization before putting out the final release.

zosrothko commented 3 years ago

Aligning the JavaCC Gradle plugin with the JavaCC organisation is a very good point. Make the changes as you want, no problem. The organization prefix is: org.javacc. I would suggest to use: org.javacc.gradle as prefix, but feel free to choose the best one for the breaking release 3.0.0. Could you also manage to support JavaCC 8 version? Thanks

kpitt commented 3 years ago

org.javacc.gradle works great for the Java package namespace. For the plugin id, I propose “org.javacc.javacc-gradle-plugin” on the Gradle Plugin Portal and “org.javacc:javacc-gradle-plugin” on Maven Central. I’ll try to get a pull request together for it next week.

The plugin has a default JavaCC version (currently at 6.1.2) but allows the user to define a different version to use. Version 7.0.10 should definitely work with no problem because it follows the same structure as 6.1.2, so 7.0.10 might be a good version to target as the new default for 3.0.0 so that we can get something out quicker.

I’ll test with JavaCC 8.0.1 to see if the plugin can handle the multiple artifacts needed for the generator plugin structure. If additional updates are needed for JavaCC 8 support, that would be a backward-compatible upgrade that could be released as 3.1.0, but if the changes are minor enough then I’ll try to get them in for 3.0.0.

I’ll create issues for both of these so that we can track the progress.

zosrothko commented 3 years ago

If you don't mind, I would prefer org.javacc.gradle.javacc-gradle-plugin since there is already an org.javacc.maven prefix for the maven plugin.

Rest of you proposal is fine

kpitt commented 3 years ago

I don’t see an org.javacc.maven group on Maven Central, but I do see org.javacc.plugin:javacc-maven-plugin. Is that what you meant?

https://github.com/javacc/javacc-maven-plugin/blob/master/pom.xml

kpitt commented 3 years ago

Added issue #52 for changing the plugin ids, and issue #53 for JavaCC 8 support.

zosrothko commented 3 years ago

org.javacc.plugin:javacc-maven-plugin Yeap, that(s what I wanted to mean!