search

takari / polyglot-maven

Support alternative markup for Apache Maven POM files
Eclipse Public License 1.0
893 stars 101 forks source link

Re-Enabled Scala Dialect and made it Java 11 ready #234

Closed lefou closed 3 years ago

lefou commented 3 years ago

This PR re-enables the Scala Dialect again.

I fixed some code issue resulting from the combination of (old) Scala 2.11 API and additions in the Java 11 API, which introduced conflicts.

I had to disable 7 internal unit tests when testing under Java 11. The actual tested code - the plugin - still works regardless of the Java version used to compile (8 or 11) and run (8 or 11). I tested this manually for now, but adding some maven-invoker-plugin based tests is probably a good idea. (Please note, that I only disabled a very small portion of the existing tests for Java 11. The issue is the specific test setup rather than some functionality not working.)

I extended the CI setup to run against a matrix, which contain Java 8 and Java 11.

Also, I added a Readme document specific to the Scala Dialect which I created long time ago.

mosabua commented 3 years ago

Do we want to update Scala dependency as well?

jvanzyl commented 3 years ago

@lefou I'm not in a dire rush to release, so if you need a couple days to get the scala module working go for it. It still seems to be failing in CI.

lefou commented 3 years ago

Do we want to update Scala dependency as well?

Depends on what the current minimal requirements for Maven and JDK are. Scala 2.11.12 is already the newers in the 2.11 branch, and 2.12 requires Java 8, so this is probably too new for polyglot-maven?

mosabua commented 3 years ago

We can definitely update to Java 11 imho and therefore probably also newer Scala.

jvanzyl commented 3 years ago

I'll let @headius chime in because I still see this in JRuby: https://github.com/jruby/jruby/blob/master/pom.rb#L59

jvanzyl commented 3 years ago

I also see this in their CI build which is using Java8 (along with Java11): https://github.com/jruby/jruby/blob/master/.github/workflows/ci.yml#L10

lefou commented 3 years ago

As a rule of thumb, I would add the lowest Java version to CI, we still want to support (and also use it for creating the release JARs). And add more recent versions to monitor for issues. Polyglot-Maven claims to be compatible to Maven 3.3.1+ and Java 7+ (https://github.com/takari/polyglot-maven/#requirements), so we should also build with Java 7 or change the Requirements. Bumping Java and also Scala would be great, but I need to check if all dependencies are available for newer Scala versions.

Remark: I was only updating the Scala dialect, to keep it in the distribution, as it makes working with Maven so much more pleasant. But I'm not using it anymore (beside some older projects in maintenance mode). So my knowledge of polyglot-maven internals is a bit rusty. I need to dig what has changed and since when the tests failed. Any pointers would be appreciated.

lefou commented 3 years ago

I can confirm that polyglot-maven tests run fine with Java 8.

lefou commented 3 years ago

@jvanzyl and @mosabua I think I have now made the Scala Dialect re-usable again. I updated the PR description, so please read it for a summary of my changes.

I suggest to build future releases with the lowest Java version we want to support, which should be Java 8 IMHO. That way, we guarantee to not let newer Java API slip into our binary artifacts.