Remove Scalariform from process-sources phase

[nealsid]

We discussed on IRC that we shouldn't do this as part of the build.
You can run it like so:

$ mvn org.scalariform:scalariform-maven-plugin:format

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/265/

[massie]

Can you also update the CONTRIBUTING.md with info how to run it?

[nealsid]

Good call - done.

[massie]

This is odd. When I run mvn org.scalariform:scalariform-maven-plugin:format against the master branch it reformats all the code again?

[nealsid]

What are the particular differences? I was running into behavior similar to that but thought it was a peculiarity to my setup.

[massie]

It also seems like running it manually ignores all the configuration options in pom.xml?

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/268/

[fnothaft]

How are you running it manually? mvn process-sources?

[nealsid]

Interesting. Apparently we needed this special id to get Maven to use the configuration tags if it was run from the command line, and not as part of an existing phase.
http://stackoverflow.com/questions/3448648/how-do-i-run-a-specific-goal-with-a-particular-configuration-in-a-maven-plugin-w

[massie]

Things are looking better now. I'll send a snippet of code to drop in scripts/jenkins-test in just a sec.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/269/

[nealsid]

@fnothaft Previously it was part of the process-sources phase, but we discussed in IRC making it run manually. So the instructions I wrote used Maven's syntax for executing a specific plugin. However, due to some Maven peculiarity, this won't use the <configuration> parameters, unless it also has a <id>default-cli</id> tag in the <execution> section.

[fnothaft]

@nealsid ah, right. Thanks for the clarification!

[massie]

@nealsid Can you add this test for Jenkins?

diff --git a/scripts/jenkins-test b/scripts/jenkins-test
index 8da20c0..627d3e9 100755
--- a/scripts/jenkins-test
+++ b/scripts/jenkins-test
@@ -38,6 +38,15 @@ $ADAM flagstat $READS
 rm -rf $ADAM_TMP_DIR
 popd

+pushd "$PROJECT_ROOT"
+mvn org.scalariform:scalariform-maven-plugin:format
+if grep --quiet "dirty" <(git describe --dirty)
+then
+        echo "Please run scalariform using 'mvn org.scalariform:scalariform-maven-plugin:format'"
+        exit 1
+fi
+popd
+
 echo
 echo "All the tests passed"
 echo

[nealsid]

Added. Thanks, @massie

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/270/

[massie]

Jenkins, test this please.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/271/

[massie]

Jenkins, test this please.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/272/

[massie]

This looks good to me. I'll wait to merge until we hear from someone else.

@heuermh, This change will keep scalariform from running at compile time (it needs to be run explicitly from the commandline) and adds a Jenkins check to the pull request builder. Does that approach make sense to you or do you have recommendations for other ways to handle this?

[heuermh]

@massie Running on demand and with a Jenkins check sounds reasonable to me.

My gripe with the settings & changes made in commit b320f02 were just about the setting that adds whitespace in the middle of a line

case None      => printCommands()

Does anyone actually type code that way? I knew a guy once who formatted his variable names that way and well that is not a story for public github comments. :)

Thanks for letting me voice a opinion, even though I'm not actually a core ADAM developer.

[fnothaft]

@heuermh I generally prefer to format case/switch/match statements like that. I think you may find that this varies a bit across language backgrounds; I spent a lot of time as a hardware developer (using Verilog), and that inline whitespace is standard style. IMO, for match statements, it makes it a lot easier to tell the differences between the cases you are matching to. I don't feel terribly strongly one way or the other, but I don't think this formatting is terribly unusual.

[massie]

Thanks, Neal!