Generated by 🚫 Danger

Thanks for your PR!
I'm not so familiar with scala, so I try to describe, which problem this PR is solving in my own words:

Currently (before this PR), PMD builds against org.scalameta:scalameta_2.13 which in turn is built against scala 2.13. Due to incompatibilities between scala 2.12 and scala 2.13, one cannot use pmd-scala in a scala 2.12 project - one would need to update to scala 2.13. According to the travis builds of sbt-cpd, it uses scala 2.10 and scala 2.12.

One incompatibility is the Iterator::iterator call, you already fixed. It seems, that with this fix, the pmd-scala module can be built both against scala 2.12 and scala 2.13.

It seems, we updated the scala version with PMD 6.18.0, so pmd-scala <= 6.17.0 supports scala 2.12 and pmd-scala >= 6.18.0 requires scala 2.13

So, would if be enough maybe, if we fix the Iterator::iterator call and you could override the dependencies in sbt-cpd to use scala 2.12? That would not do a cross-compile, though. It would require binary compatibility. Would that work? This might be already all you need.

When adding cross-compiled modules, I wonder, how many versions of scala we need to support? Is scala 2.12 and 2.13 enough or do we also need to support 2.11 and 2.10 and later 2.14. (2.14 probably won't come, see Scala 2 Roadmap Update. So, the number of modules would be limited - which is a relief.

Adding support for cross-compiled modules in the way you did, has some problems:

• we change the artifactId of pmd-scala, which means, with the next version, there will be no pmd-scala module anymore, but pmd-scala_2.12 and pmd-scala_2.13 - that is incompatible. Ideally, we still provide pmd-scala and in addition to this (if needed) pmd-scala_2.13.
• it requires multiple builds. That makes the release process potentially fragile: pmd-scala_2.12 and pmd-scala_2.13 are created in two separate maven runs, each of them deploy into a separate staging repository, which could potentially lead to the problem, that only one of the two appears in maven-central.
• it requires multiple builds - this needs to be kept always in mind. You already update the build scripts, but this might be easily forgotten. It also makes it a bit more difficult to reproduce the builds.
• If we change the supported scala version, we need to change now the build scripts, rather than the source code.

So, if we need to support such "real" cross-compiled builds, I'd rather look into a solution, that integrates this via additional maven modules (like it is done here: https://github.com/random-maven/scalor-maven-plugin/tree/master/src/it/test-cross). The sources are shared via sym-link but each modules builds against a specific scala version. Using this project layout, a single maven build is enough to produce all artifacts.

Next steps:

• Figure out, whether we need real cross-compiled builds or whether scala 2.12 is binary compatible with scala 2.13 (and 2.11 and 2.10)
  • Then either just fix the Iterator::iterator call
  • Or create the module structure

Thanks for your PR!
I'm not so familiar with scala, so I try to describe, which problem this PR is solving in my own words:

Currently (before this PR), PMD builds against org.scalameta:scalameta_2.13 which in turn is built against scala 2.13. Due to incompatibilities between scala 2.12 and scala 2.13, one cannot use pmd-scala in a scala 2.12 project - one would need to update to scala 2.13. According to the travis builds of sbt-cpd, it uses scala 2.10 and scala 2.12.

Correct, I would just add that sbt-cpd is still using version 5.8.1, and it is doing cross compilation for 2.10 and 2.12 (and not for 2.11) because it is an sbt plugin. sbt plugins for version 1.x need to be compiled for scala 2.12 and for sbt 0.13.x it needs to be compiled for scala 2.10. However I would say most of the community is al ready using sbt 1.x

One incompatibility is the Iterator::iterator call, you already fixed. It seems, that with this fix, the pmd-scala module can be built both against scala 2.12 and scala 2.13.

Yes this makes the the pmd-scala java code source-compatible with scala 2.12 and 2.13, but does not make it binary compatible between scala 2.12 and 2.13 (because we have a dependency on scalameta we have a dependency also on the scala std library which is not binary compatible...)

It seems, we updated the scala version with PMD 6.18.0, so pmd-scala <= 6.17.0 supports scala 2.12 and pmd-scala >= 6.18.0 requires scala 2.13

So, would if be enough maybe, if we fix the Iterator::iterator call and you could override the dependencies in sbt-cpd to use scala 2.12? That would not do a cross-compile, though. It would require binary compatibility. Would that work? This might be already all you need.

I would say this will not work, but OTOH this seems to work for the current version of sbt-cpd. The current version of sbt-cpd depends on pmd-scala % 5.8.1 which depends on scala-library % 2.10, but I guess when it is cross compiled in 2.12 the scala-library dependency is bumped to 2.12 and it seems to work...

When adding cross-compiled modules, I wonder, how many versions of scala we need to support? Is scala 2.12 and 2.13 enough or do we also need to support 2.11 and 2.10 and later 2.14. (2.14 probably won't come, see Scala 2 Roadmap Update. So, the number of modules would be limited - which is a relief.

The consensus in the scala comunity is to support the current scala version (2.13) and the previous one (2.12). However we also need to consider that pmd will mostly be used from an sbt plugin and sbt plugins must target scala 2.12. So maybe would be enough to just support scala 2.12? However that would be a breaking change since pmd-scala already targets 2.13...

Adding support for cross-compiled modules in the way you did, has some problems:

* we change the artifactId of pmd-scala, which means, with the next version, there will be no pmd-scala module anymore, but pmd-scala_2.12 and pmd-scala_2.13 - that is incompatible. Ideally, we still provide pmd-scala and in addition to this (if needed) pmd-scala_2.13.

I would really not do this, since it does not follow the scala conventions...

* it requires multiple builds. That makes the release process potentially fragile: pmd-scala_2.12 and pmd-scala_2.13 are created in two separate maven runs, each of them deploy into a separate staging repository, which could potentially lead to the problem, that only one of the two appears in maven-central.

* it requires multiple builds - this needs to be kept always in mind. You already update the build scripts, but this might be easily forgotten. It also makes it a bit more difficult to reproduce the builds.

* If we change the supported scala version, we need to change now the build scripts, rather than the source code.

So, if we need to support such "real" cross-compiled builds, I'd rather look into a solution, that integrates this via additional maven modules (like it is done here: random-maven/scalor-maven-plugin:src/it/test-cross@master). The sources are shared via sym-link but each modules builds against a specific scala version. Using this project layout, a single maven build is enough to produce all artifacts.

I completely understand your points, and I will try what you propose with scalor-maven-plugin. What about pmd-dist? I will depend on pmd-scala_2.12 or pmd-scala_2.13?

Next steps:

* Figure out, whether we need real cross-compiled builds or whether scala 2.12 is binary compatible with scala 2.13 (and 2.11 and 2.10)
  
  * Then either just fix the `Iterator::iterator` call
  * Or create the module structure

One incompatibility is the Iterator::iterator call, you already fixed. It seems, that with this fix, the pmd-scala module can be built both against scala 2.12 and scala 2.13.

Yes this makes the the pmd-scala java code source-compatible with scala 2.12 and 2.13, but does not make it binary compatible between scala 2.12 and 2.13

Yes, I feared that - it might be accidentally compatible, but it is not guaranteed to work (the similar case is happening with java's bootclasspath...).

The consensus in the scala comunity is to support the current scala version (2.13) and the previous one (2.12). However we also need to consider that pmd will mostly be used from an sbt plugin and sbt plugins must target scala 2.12. So maybe would be enough to just support scala 2.12? However that would be a breaking change since pmd-scala already targets 2.13...

Thanks for clarifying which versions should be supported - it seems, the PMD then should also support both version 2.12 and 2.13.

Adding support for cross-compiled modules in the way you did, has some problems:

* we change the artifactId of pmd-scala, which means, with the next version, there will be no pmd-scala module anymore, but pmd-scala_2.12 and pmd-scala_2.13 - that is incompatible. Ideally, we still provide pmd-scala and in addition to this (if needed) pmd-scala_2.13.

I would really not do this, since it does not follow the scala conventions...

Ok, so I'd suggest the following modules:

1. pmd-scala - which is basically a empty module (packaging pom) and just has a dependency to pmd-scala_2.13
2. pmd-scala_2.13
3. pmd-scala_2.12

So, if we need to support such "real" cross-compiled builds, I'd rather look into a solution, that integrates this via additional maven modules (like it is done here: random-maven/scalor-maven-plugin:src/it/test-cross@master). The sources are shared via sym-link but each modules builds against a specific scala version. Using this project layout, a single maven build is enough to produce all artifacts.

I completely understand your points, and I will try what you propose with scalor-maven-plugin. What about pmd-dist? I will depend on pmd-scala_2.12 or pmd-scala_2.13?

That's a good question. It can only depend on one version. pmd-dist is used to create the CLI version of PMD and is pretty self-contained. I guess, we can leave it at 2.13 then?

I was googling yesterday a bit about the symlink solution: while this is a no-brainer for linux/macosx, I'm not sure, whether PMD would still be able to be built under Windows.

Maybe an alternative to the symlink solution could be, to override the default sourceDirectoy, e.g.

  <build>
    <sourceDirectory>${project.basedir}/../pmd-scala/src/main/java</sourceDirectory>

That is a bit unusual (maybe the IDEs are confused then)... something to test :)

The directory structure I propose looks like this:

<main pmd dir>
  |- pmd-scala-modules
      |- pmd-scala
      |       |- pom.xml: artifactId=pmd-scala, packaging=pom, dependency=pmd-scala_2.13
      |       |- src/main/java (since packaging=pom, these sources are not used)
      |- pmd-scala_2.13
      |       |- pom.xml: artifactId=pmd-scala_2.13, packaging=jar, build.sourceDirectory=../pmd-scala/src/main/java
      |- pmd-scala_2.12
              |- pom.xml: artifactId=pmd-scala_2.12, packaging=jar, build.sourceDirectory=../pmd-scala/src/main/java

The modules list in the main parent pom of PMD:

<module>pmd-scala-modules/pmd-scala</module>
<module>pmd-scala-modules/pmd-scala_2.13</module>
<module>pmd-scala-modules/pmd-scala_2.12</module>

I've not tried it out, whether that would work....

Hi @adangel

I tried to do the structure you proposed without symlinks trick, but even after reconfiguring all the sourceDirectories and resourceDirectories, the tests were failing with:

[ERROR] Tests run: 2, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.017 s <<< FAILURE! - in net.sourceforge.pmd.lang.scala.ast.ScalaParserTests
[ERROR] testPackageObject  Time elapsed: 0.015 s  <<< FAILURE!
java.lang.AssertionError: Source file /home/joao/git/pmd/pmd-scala-modules/pmd-scala_2.12/src/test/resources/net/sourceforge/pmd/lang/scala/ast/testdata/package.scala is missing
	at net.sourceforge.pmd.lang.scala.ast.ScalaParserTests.testPackageObject(ScalaParserTests.kt:21)

[ERROR] testSomeScalaFeatures  Time elapsed: 0 s  <<< FAILURE!
java.lang.AssertionError: Source file /home/joao/git/pmd/pmd-scala-modules/pmd-scala_2.12/src/test/resources/net/sourceforge/pmd/lang/scala/ast/testdata/List.scala is missing
	at net.sourceforge.pmd.lang.scala.ast.ScalaParserTests.testSomeScalaFeatures(ScalaParserTests.kt:18)

So I gave up and used symlinks instead and things seem to work...

@jtjeferreira Thanks for the update.

I'm working on it now and do the following changes:

• I've tried out the symlinks under Windows and as expected - it just doesn't work without fiddling (first git config --local core.symlinks true, then read carefully https://github.com/git-for-windows/git/wiki/Symbolic-Links#allowing-non-administrators-to-create-symbolic-links and assign your user the permission to actually create symbolic links. This setting is only effective after logout/login. Then checkout the branch with the symlinks again, now they should be created correctly. (I guess travis runs anyway with admin rights or they took care of that permission already)
• I still prefer a solution without symlinks. The test failures come from a custom surefire configuration from us:

  pmd/pom.xml

  Line 259 in bc4a1d6

  <mvn.project.src.test.resources>${project.basedir}/src/test/resources</mvn.project.src.test.resources>

  • this can be solved by using ${project.build.testResources[0].directory}. Then the configuration under project/build/testResources is respected in individual projects.
• I think, it would be good to retain the project folder pmd-scala for a while before we delete it completely - so I keep that with a dependency to pmd-scala_2.13 . That way, if someone depends on pmd-scala it should transitively pull pmd-scala_2.13 and things should continue to work.
• I'd rename the shared source folder to pmd-scala-modules/pmd-scala-common
• I've seen that there are a couple of duplications in the two poms for pmd-scala_2.12 and pmd-scala_2.13. I'll try to move that into a common parent pom (which is pmd-scala-common). E.g. it makes sense to keep the version of scalameta the same.