Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc pom/test/resource improvements #1142

Closed
wants to merge 11 commits into from

Conversation

ryan-williams
Copy link
Member

these happened while i was doing #1139; individual commits should be reasonably self-explanatory

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1440/

Build result: FAILURE

GitHub pull request #1142 of commit 3e02e9b automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1142/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains 3722878646d30b56dcaa16c9624bc0467db0486c # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1142/merge^{commit} # timeout=10Checking out Revision 3722878646d30b56dcaa16c9624bc0467db0486c (origin/pr/1142/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 3722878646d30b56dcaa16c9624bc0467db0486cFirst time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@ryan-williams
Copy link
Member Author

Some problem with where test-coverage reports are getting written after I changed directory configs from surefire-reports to scalatest-reports; I assumed that was find to just do everywhere but maybe something not in this repo has surefire-reports hard-coded? Looking…

@ryan-williams
Copy link
Member Author

The end of the log seems to get through the end of jenkins-test script successfully, so I can't tell what it's trying to do when the failure occurs:

+ echo 'All the tests passed'
All the tests passed
echo
+ echo

Recording test results
ERROR: Step ?Publish JUnit test result report? failed: No test report files were found. Configuration error?
Publishing Scoverage XML and HTML report...
Setting commit status on GitHub for https://github.com/bigdatagenomics/adam/commit/3722878646d30b56dcaa16c9624bc0467db0486c
Finished: FAILURE

@fnothaft
Copy link
Member

@ryan-williams IIRC, Jenkins has the test report paths hardcoded. Let me take a look at the config this AM!

@heuermh
Copy link
Member

heuermh commented Aug 31, 2016

Looks reasonable to me.

With scala.version.prefix will need to confirm the move_to... scripts still work correctly.

@fnothaft
Copy link
Member

Jenkins, test this please.

@@ -0,0 +1,3 @@
# We blanket-ignore .bam files in the top-level of the repo, but want to undo that here.
!*.bam
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nifty! Didn't know you could do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea this caused me some problems when I tried to git mv adam-core/* ./ to put all of adam-core in the top directory (after git rming the other modules); sorted.bam* did not come with that move since they were .gitignored (despite having been git added to the repo at some point, maybe before they became .gitignored?), so I was just missing them in my builds, which took me a while to figure out.

In any case, yea .gitignore "whitelists" can be useful in addition to the way it's usually used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah originally we had a prohibition on adding binary files, but we've relaxed that over time. Thanks for patching this!

@fnothaft
Copy link
Member

This 99% LGTM! The only nit that I have is that Scala version string changes should be OK but you need to change the scala versions at https://github.com/bigdatagenomics/adam/blob/master/scripts/move_to_scala_2.11.sh#L5 and https://github.com/bigdatagenomics/adam/blob/master/scripts/move_to_scala_2.10.sh#L5. Right now, the Scala 2.11 builds are failing, but I think fixing the two files above will resolve that. Thanks for going through and cleaning these bits up, @ryan-williams!

@fnothaft
Copy link
Member

Also, I have done the surefire_reports --> scalatest_reports path change in Jenkins, so that should be good to go.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1441/

Build result: FAILURE

GitHub pull request #1142 of commit 3e02e9b automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1142/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains 3722878646d30b56dcaa16c9624bc0467db0486c # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1142/merge^{commit} # timeout=10Checking out Revision 3722878646d30b56dcaa16c9624bc0467db0486c (origin/pr/1142/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 3722878646d30b56dcaa16c9624bc0467db0486cFirst time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@ryan-williams
Copy link
Member Author

OK, just pushed an attempt at updating the move_to scripts to use 2.11.8 and 2.10.6

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1442/

Build result: FAILURE

GitHub pull request #1142 of commit 372b573 automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1142/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains ee02423 # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1142/merge^{commit} # timeout=10Checking out Revision ee02423 (origin/pr/1142/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f ee024234052b2bd21ebcbadd15ff96785320fdccFirst time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@@ -268,7 +268,8 @@
</execution>
</executions>
<configuration>
<scalaVersion>2.10.4</scalaVersion>
<scalaVersion>${scala.version}</scalaVersion>
<recompileMode>incremental</recompileMode>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd removed incremental in the past. I'd need to dig through the commit history to find the exact reason, but it has something to do with causing transient build issues. The Jenkins build just failed with an issue related to such. Alternatively, we could do -DrecompileMode=all in scripts/jenkins-test as suggested by davidB/scala-maven-plugin#185. I'm open to either. It'd be nice to have faster Scala compile times in the general case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting; so incremental compilation in Scala 2.11 either doesn't work correctly at all, or only works in SBT?

Maybe a better option would be to enable a different Maven profile in the presence of a jenkins environment variable?

That way we would get incremental compiles locally, which would be nice, unless it's so broken we don't think we want that.

@ryan-williams
Copy link
Member Author

Just following up here, the last build failed on 2.6.0 x 1.5.2 x 2.10 with:

[error] Required file not found: sbt-interface.jar
[error] See zinc -help for information about locating necessary files

and 2.6.0 x 1.5.2 x 2.11 with:

[ERROR] The build could not read 1 project -> [Help 1]
[ERROR]   
[ERROR]   The project org.bdgenomics.adam:adam-core_2.11:0.19.1-SNAPSHOT (/home/jenkins/workspace/ADAM-prb/HADOOP_VERSION/2.6.0/SCALAVER/2.11/SPARK_VERSION/1.5.2/label/centos/adam-core/pom.xml) has 1 error
[ERROR]     'dependencies.dependency.version' for org.apache.parquet:parquet-scala_2.10:jar is missing. @ line 155, column 17
[ERROR] 

I don't have obvious leads about either one.

@heuermh
Copy link
Member

heuermh commented Sep 1, 2016

The parquet-scala_2.10 dependency is strange as there is no 2.11 version.
http://search.maven.org/#search%7Cga%7C1%7Cparquet-scala

The move_to_scala_2.11.sh script has a workaround, maybe something went wrong
https://github.com/bigdatagenomics/adam/blob/master/scripts/move_to_scala_2.11.sh#L6

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1517/

Build result: FAILURE

GitHub pull request #1142 of commit ef8e35f automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1142/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains 711b45729873bba2a021a1feaf618a316d783327 # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1142/merge^{commit} # timeout=10Checking out Revision 711b45729873bba2a021a1feaf618a316d783327 (origin/pr/1142/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 711b45729873bba2a021a1feaf618a316d783327First time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1518/

Build result: FAILURE

GitHub pull request #1142 of commit 073cf9f automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1142/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains 5b75185a8a9b69552fced6383e13828bafe7bc07 # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1142/merge^{commit} # timeout=10Checking out Revision 5b75185a8a9b69552fced6383e13828bafe7bc07 (origin/pr/1142/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 5b75185a8a9b69552fced6383e13828bafe7bc07First time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@ryan-williams
Copy link
Member Author

I think the missing sbt-interface.jar has to do with zinc not being installed or configured properly on the Jenkins build machine. Not sure what to do about it, maybe I'll try to just turn off incremental-compilation, overall or just for Jenkins.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1519/

Build result: FAILURE

[...truncated 32 lines...]Triggering ADAM-prb ? 2.6.0,2.11,1.3.1,centosTriggering ADAM-prb ? 2.3.0,2.11,1.4.1,centosTriggering ADAM-prb ? 2.6.0,2.11,2.0.0,centosTriggering ADAM-prb ? 2.3.0,2.10,1.4.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.6.1,centosADAM-prb ? 2.3.0,2.11,1.5.2,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.10,1.5.2,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.10,2.0.0,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.4.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.10,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.10,2.0.0,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.4.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.10,1.3.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.10,1.3.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,1.3.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,2.0.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.11,1.3.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,1.4.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.11,2.0.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.4.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.11,1.6.1,centos completed with result SUCCESSNotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@ryan-williams
Copy link
Member Author

All the Spark 2.0.0 builds failed with:

mvn clean -Dhadoop.version=2.3.0 -Dspark.version=2.0.0
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=1g; support was removed in 8.0
[INFO] Scanning for projects...
[ERROR] The build could not read 4 projects -> [Help 1]
[ERROR]   
[ERROR]   The project org.bdgenomics.adam:adam-core-spark2_2.10:0.19.1-SNAPSHOT (/home/jenkins/workspace/ADAM-prb/HADOOP_VERSION/2.3.0/SCALAVER/2.10/SPARK_VERSION/2.0.0/label/centos/adam-core/pom.xml) has 5 errors
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-misc-spark2_2.10:test-jar is missing. @ line 93, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-metrics-spark2_2.10:jar is missing. @ line 99, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-io-spark2_2.10:jar is missing. @ line 103, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-cli-spark2_2.10:jar is missing. @ line 107, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-intervalrdd-spark2_2.10:jar is missing. @ line 111, column 17
[ERROR]   
[ERROR]   The project org.bdgenomics.adam:adam-apis-spark2_2.10:0.19.1-SNAPSHOT (/home/jenkins/workspace/ADAM-prb/HADOOP_VERSION/2.3.0/SCALAVER/2.10/SPARK_VERSION/2.0.0/label/centos/adam-apis/pom.xml) has 3 errors
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-misc-spark2_2.10:test-jar is missing. @ line 105, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.adam:adam-core-spark2_2.10:jar is missing. @ line 115, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.adam:adam-core-spark2_2.10:test-jar is missing. @ line 119, column 17
[ERROR]   
[ERROR]   The project org.bdgenomics.adam:adam-cli-spark2_2.10:0.19.1-SNAPSHOT (/home/jenkins/workspace/ADAM-prb/HADOOP_VERSION/2.3.0/SCALAVER/2.10/SPARK_VERSION/2.0.0/label/centos/adam-cli/pom.xml) has 9 errors
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-misc-spark2_2.10:test-jar is missing. @ line 116, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-misc-spark2_2.10:jar is missing. @ line 122, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-io-spark2_2.10:jar is missing. @ line 126, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-cli-spark2_2.10:jar is missing. @ line 130, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.utils:utils-metrics-spark2_2.10:jar is missing. @ line 134, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.adam:adam-core-spark2_2.10:jar is missing. @ line 142, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.adam:adam-core-spark2_2.10:test-jar is missing. @ line 146, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.adam:adam-apis-spark2_2.10:jar is missing. @ line 152, column 17
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.adam:adam-apis-spark2_2.10:test-jar is missing. @ line 156, column 17
[ERROR]   
[ERROR]   The project org.bdgenomics.adam:adam-assembly-spark2_2.10:0.19.1-SNAPSHOT (/home/jenkins/workspace/ADAM-prb/HADOOP_VERSION/2.3.0/SCALAVER/2.10/SPARK_VERSION/2.0.0/label/centos/adam-assembly/pom.xml) has 1 error
[ERROR]     'dependencies.dependency.version' for org.bdgenomics.adam:adam-cli-spark2_2.10:jar is missing. @ line 115, column 17

Any ideas?

@heuermh
Copy link
Member

heuermh commented Oct 4, 2016

The version for those dependencies should be 0.2.8, which is on Maven Central. Are those variable substitutions coming from a profile? We had trouble with that before.

@@ -476,7 +476,8 @@
</dependency>
<dependency>
<groupId>org.apache.parquet</groupId>
<artifactId>parquet-scala_${scala.version.prefix}</artifactId>
<!-- This library has no Scala 2.10 version, but using the 2.10 version seems to work. -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is correct

@@ -282,9 +282,9 @@
</jvmArgs>
<javacArgs>
<javacArg>-source</javacArg>
<javacArg>${java.version}</javacArg>
<javacArg>1.7</javacArg>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we require jdk8 because of the transitive dependency on htsjdk through Hadoop-BAM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, needs to be changed. Although, is there a reason to move away from the ${java.version} property here, if we're generally moving back towards using the parameters throughout the POM? I would prefer going back to ${java.version} on this line.

Copy link
Member Author

@ryan-williams ryan-williams Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I got cornered into doing this for a reason that I didn't have paged in and meant to go back and try to reconstruct what that was before responding.

I haven't quite done that yet, but I'm pretty sure it was related to my making the build use zinc; for some reason 1.8 was not acceptable in that setting, and I had to hard-code 1.7 here. I'll try to dig back and get the full state on it for posterity, but in the meantime I was going to give up on zinc in this PR anyway, since I didn't know how to get around the "sbt-interface.jar not found" problem on Jenkins discussed elsewhere on this PR (I believe due to zinc not being installed on Jenkins in some way), so I'll revert this bit momentarily. Thanks!

<!-- informative only, used in about template substitution -->
<scala.version>2.10</scala.version>
<scala.version>2.10.6</scala.version>
<scala.version.prefix>2.10</scala.version.prefix>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting; what is the contract with that file? Every Maven property must be declared there? Why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That file is populated by the templating-maven-plugin and used to generate the cli --version message

$ ./bin/adam-submit --version
Using ADAM_MAIN=org.bdgenomics.adam.cli.ADAMMain
Using SPARK_SUBMIT=/usr/local/bin/spark-submit

       e         888~-_          e             e    e
      d8b        888   \        d8b           d8b  d8b
     /Y88b       888    |      /Y88b         d888bdY88b
    /  Y88b      888    |     /  Y88b       / Y88Y Y888b
   /____Y88b     888   /     /____Y88b     /   YY   Y888b
  /      Y88b    888_-~     /      Y88b   /          Y888b

ADAM version: 0.19.1-SNAPSHOT
Commit: ca4d9649bac2523f9951c24f2e184a1b37f9ba06 Build: 2016-10-04
Built for: Scala 2.10 and Hadoop 2.6.0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I looked around this a bit but don't see what change I need to make; help?

The templating-maven-plugin docs say it's useful "if you need for example to have things in that code replaced with some properties values."

${scala.version} is already exposed there (though not used anywhere in the project afaict) and I have no use for exposing ${scala.version.prefix} there, if that's what you mean?

Copy link
Member

@heuermh heuermh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All I meant was that before ${scala.version} was used to populate that field on About.java, so that it could be displayed in the --version string, thus the comment at the previous line 22.

When you used ${scala.version} elsewhere defined as 2.10.6, then the field in About.java should have been updated to use ${scala.version.prefix} instead.

But as mentioned below, I don't think we'll be able to use these properties for substitution in artifact names, so I guess the point is moot.

Copy link
Member

@fnothaft fnothaft Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK (preferable, really) to have About print the full Scala version, as opposed to the prefix. I don't think we need to make a change to About.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that makes sense @heuermh, thanks for the explanation. Sounds like in the end we can just leave as is though, lmk if otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with me

</dependency>
<dependency>
<groupId>org.bdgenomics.adam</groupId>
<artifactId>adam-core_2.10</artifactId>
<artifactId>adam-core_${scala.version.prefix}</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly certain there's a reason why we can't do this, I know because I tried to do it once. Of course, I don't remember why it didn't work . . .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK to use the properties in the artifactId for dependencies, it doesn't work to use properties in the artifactId for the POM itself. I know we've gone back and forth on this a lot, but I think it's actually preferable to do what @ryan-williams is doing here. I'll make a comment at the end with my reasoning.

@ryan-williams
Copy link
Member Author

The version for those dependencies should be 0.2.8, which is on Maven Central. Are those variable substitutions coming from a profile? We had trouble with that before.

Those errors are modules' <dependency>s not declaring <version>s (because they should pick them up from the parent POM), but something about that resolution breaking down and Maven thinking they are just missing a <version> specification.

Copy link
Member

@fnothaft fnothaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need the Java version changes in POM and scripts/jenkins-test, otherwise LGTM!

</dependency>
<dependency>
<groupId>org.bdgenomics.adam</groupId>
<artifactId>adam-core_2.10</artifactId>
<artifactId>adam-core_${scala.version.prefix}</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK to use the properties in the artifactId for dependencies, it doesn't work to use properties in the artifactId for the POM itself. I know we've gone back and forth on this a lot, but I think it's actually preferable to do what @ryan-williams is doing here. I'll make a comment at the end with my reasoning.

@@ -282,9 +282,9 @@
</jvmArgs>
<javacArgs>
<javacArg>-source</javacArg>
<javacArg>${java.version}</javacArg>
<javacArg>1.7</javacArg>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, needs to be changed. Although, is there a reason to move away from the ${java.version} property here, if we're generally moving back towards using the parameters throughout the POM? I would prefer going back to ${java.version} on this line.

-e "s:sun.io.serialization.extendedDebugInfo=true:sun.io.serialization.extendedDebugInfo=true -Djava.io.tmpdir=${ADAM_MVN_TMP_DIR}:g" \
{} \;
find . -name "*.bak" -exec rm {} \;

# variable declarations
export JAVA_HOME=/usr/java/jdk1.8.0_60
if [ -z "$JAVA_HOME" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit ugly, but we always need to overwrite $JAVA_HOME here due to Jenkins doing "shenanigans" with the Java version. The short story is that Jenkins will always set $JAVA_HOME to a bad value, and we are powerless to stop it. There's a much longer and uninteresting backstory. Can you revert this back, or add some other logic to disable overwriting $JAVA_HOME?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I was too quick on the trigger here. We can more cleanly handle setting the JAVA_HOME value in the Jenkins command that wraps the jenkins-test invocation. Perhaps it'd be cleanest to remove this logic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I'll remove it here then altogether, 1m.

@ryan-williams
Copy link
Member Author

ryan-williams commented Oct 5, 2016

OK, I reverted the java.version bit and removed the jenkins-test JAVA_HOME-handling; I don't know why the build would avoid the failure above this time around, so am sort of expecting that to happen again, and suspect that it is related to the scala.version.prefix usage throughout the POMs, so let's see what happens there.

Also I'm not sure about Github's code-review flow, but it is telling me I need to make changes to the java.version thing even though I already did (and force-pushed over the old version):

We'll see if that resolves itself given some time? Or maybe force-pushing just breaks everything.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1521/

Build result: FAILURE

[...truncated 32 lines...]Triggering ADAM-prb ? 2.6.0,2.11,1.3.1,centosTriggering ADAM-prb ? 2.3.0,2.11,1.4.1,centosTriggering ADAM-prb ? 2.6.0,2.11,2.0.0,centosTriggering ADAM-prb ? 2.3.0,2.10,1.4.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.6.1,centosADAM-prb ? 2.3.0,2.11,1.5.2,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.10,1.5.2,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.10,2.0.0,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.4.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.10,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.10,2.0.0,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.4.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.10,1.3.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.10,1.3.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,1.3.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,2.0.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.6.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.11,1.3.1,centos completed with result SUCCESSADAM-prb ? 2.3.0,2.11,1.4.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.11,2.0.0,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.4.1,centos completed with result SUCCESSADAM-prb ? 2.6.0,2.11,1.6.1,centos completed with result SUCCESSNotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1522/
Test PASSed.

@ryan-williams
Copy link
Member Author

Cool, so it turns out I just hadn't updated the sed regexs in move_to_spark2 to properly handle lines that now have _${scala.version.prefix} instead of _2.10.

I am not sure what to do about this "fnothaft requested changes" error I'm still seeing.

@fnothaft
Copy link
Member

fnothaft commented Oct 7, 2016

LGTM! Unless anyone has objections, I will squash this down and merge in the AM.

@fnothaft
Copy link
Member

fnothaft commented Oct 7, 2016

Thanks @ryan-williams! I've merged this manually as ad50f7d, e8b0fcd, cd488df, d242a26, a0730f3, and 89942fe. As always, thank you for taking the time to make a nice cleanup pass! It is greatly appreciated.

@fnothaft fnothaft closed this Oct 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants