-
Notifications
You must be signed in to change notification settings - Fork 309
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
[ADAM-1008] Modify jenkins-test script to support Java 8 build. #1026
Conversation
LGTM++ |
Test FAILed. Build result: FAILURE[...truncated 24 lines...]Triggering ADAM-prb ? 2.3.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.4.1,centosTriggering ADAM-prb ? 2.3.0,2.10,1.4.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.6.1,centosTriggering ADAM-prb ? 2.6.0,2.11,1.3.1,centosTriggering ADAM-prb ? 2.3.0,2.10,1.3.1,centosTriggering ADAM-prb ? 2.3.0,2.11,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.10,1.3.1,centosTriggering ADAM-prb ? 2.3.0,2.10,1.5.2,centosADAM-prb ? 2.6.0,2.10,1.4.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.3.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.6.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.4.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.4.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.4.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.6.1,centos completed with result FAILUREADAM-prb ? 2.6.0,2.11,1.3.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.3.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.11,1.5.2,centos completed with result FAILUREADAM-prb ? 2.6.0,2.10,1.3.1,centos completed with result FAILUREADAM-prb ? 2.3.0,2.10,1.5.2,centos completed with result FAILURENotifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'Test FAILed. |
77df0bf
to
a9cc8f3
Compare
Test PASSed. |
Are folks OK with me merging this/can I get a merge? |
you can merge it only if you don't forget to change the jenkins build. On Thu, Apr 28, 2016 at 2:28 PM, Frank Austin Nothaft <
|
@shaneknapp oh, of course! I don't want to change the Jenkins build though until this is right about to merge (i.e., I will change the Jenkins build immediately before merging this). |
i took a closer look and have some comments... mostly bash "style" stuff On Thu, Apr 28, 2016 at 3:58 PM, Frank Austin Nothaft <
|
@shaneknapp that'd be great. Do you want to drop them inline on the PR? Alternatively, if you want to send a commit against my branch, that'd be great too. |
yeah, there's a bunch that needs to be done... i'll start at it for a bit and then post some thoughts and inline comments. |
@@ -2,81 +2,107 @@ | |||
|
|||
set -e -x | |||
|
|||
export JAVA_HOME=/usr/java/jdk1.8.0_60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put all of your variable decs up here, rather than spreading them throughout the script. and only use export for variables that need to be visible to subprocs (JAVA_HOME, PATH, etc).
ill just say that you have a lot of extraneous quotes, and inconsistent variable declaration. a good, easy to read standard looks like this: FOO="${BAR}.baz.zomgwtf" for STDOUT declared vars: FOO=$(command) curly braces around vars are optional, depending on legibility. bonus points for 'set -x -e'. :) clean up the variable decs and quotes, and i'll take a second look. |
+1 from me, after cleanup |
5547830
to
782075b
Compare
Updated to resolve review comments. Also, added commit to address #827. |
Test FAILed. Build result: FAILUREGitHub pull request #1026 of commit 782075b 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/1026/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains ec177bd # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1026/merge^{commit} # timeout=10Checking out Revision ec177bd (origin/pr/1026/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f ec177bda52cc845b436dfdbbd43facfbf6e41cbeFirst 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. |
Resolves bigdatagenomics#1008. As per discussion with @shaneknapp, it is preferable to support builds across Java versions by manually setting JAVA_HOME in a shell script run inside of Jenkins. This is due to certain configuration limitations inside of Jenkins. Currently, we are doing this via a script tracked inside of Jenkins. I would rather roll all of that config into the ./scripts/jenkins-test script so that we have revision control, etc.
Test PASSed. |
looks great! |
LGTM |
Thank you, @fnothaft! |
Resolves #1008. As per discussion with @shaneknapp, it is preferable to support builds across Java versions by manually setting JAVA_HOME in a shell script run inside of Jenkins. This is due to certain configuration limitations inside of Jenkins. Currently, we are doing this via a script tracked inside of Jenkins. I would rather roll all of that config into the ./scripts/jenkins-test script so that we have revision control, etc.