SPARK-2651: Add maven scalastyle plugin

[rahulsinghaliitd]

Can be run as: "mvn scalastyle:check"

[AmplabJenkins]

Can one of the admins verify this patch?

[pwendell]

is the goal here to make this run when someone runs mvn package? It didn't seem to do that when I ran it (or maybe it runs them all at the end, I just looked after core was compiled?)

[rahulsinghaliitd]

Yes, mvn package is supposed to run these checks. I am surprised to hear that it didn't work for you. I had updated the PR yesterday, do by chance happen to have the older version (it was missing the line <phase>package</phase>)? Here is the snippet from my machine:

[INFO] --- maven-jar-plugin:2.4:jar (default-jar) @ spark-core_2.10 ---
[INFO] Building jar: /mnt/devel/rahuls/code/apache-spark/core/target/spark-core_2.10-1.1.0-SNAPSHOT.jar
[INFO]
[INFO] --- maven-site-plugin:3.3:attach-descriptor (attach-descriptor) @ spark-core_2.10 ---
[INFO]
[INFO] --- maven-source-plugin:2.2.1:jar-no-fork (create-source-jar) @ spark-core_2.10 ---
[INFO] Building jar: /mnt/devel/rahuls/code/apache-spark/core/target/spark-core_2.10-1.1.0-SNAPSHOT-sources.jar
[INFO]
[INFO] --- scalastyle-maven-plugin:0.4.0:check (default) @ spark-core_2.10 ---
Saving to outputFile=/mnt/devel/rahuls/code/apache-spark/core/scalastyle-output.xml
Processed 361 file(s)
Found 0 errors
Found 0 warnings
Found 0 infos
Finished in 8234 ms

Another way to run these checks is through mvn scalastyle:check but that requires the inter-module dependencies are satisfied via M2 repo.

[pwendell]

Thanks for adding this to maven - just had one question based on running this locally.

[pwendell]

After some thought, I'd prefer not to force stylce checking when the package target is run. Many distributors fork the Spark build and modify things, I don't want to impose on them having to meet our internal style guidelines or they get a build failure.

I think it's fine to just support mvn install; mvn scalastyle:check. Another alternative is, we could add a profile -Pscalastyle that enables checking when package is run.

[srowen]

Packaging wouldn't generally mean changing code checked by scalastyle, I think. I wouldn't mind it being enabled on package; it feels like something to run with the test phase even. It's a good step forward to be able to invoke it at all.

[pwendell]

@srowen your perspective could be helpful here. My concern was that say Cloudera has some custom patches or back ports in a Spark package. Would they find it annoying if the build fails if they do not meet the upstream style guidelines in their own packages?

[srowen]

Cloudera only back-ports and doesn't add extra code. Back-porting style-compliant code should result in style-compliant code. Of course a back-port sometimes requires a little surgery to transplant back in time if it doesn't merge cleanly, but it's not difficult to make it style compliant.

[pwendell]

okay then - perhaps it's fine to stick with this. We can always make it less stringent if people complain. My main concern was just frustrating downstream packagers - thanks for chiming in on this one.

[rahulsinghaliitd]

The default maven phase for this plugin was verify which would have been ideal since it is not run as part of package but is part of install. But I wanted these checks to be enabled through make-distribution.sh script but that only executes the package phase.

Although I agree with the current decision to revisit this on any complaints from downstream.