[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set
jdk7 as JAVA_HOME for branches-1 precommit and nightly.
URL: https://github.com/apache/hbase/pull/621#discussion_r324853657
##
File path: dev-support/Jenkinsfile
##
@@ -131,9 +131,11 @@ curl -L -o personality.sh "${env.PROJECT_PERSONALITY}"
// TODO does hadoopcheck need to be jdk specific?
// Should be things that work with multijdk
TESTS = 'all,-unit,-findbugs'
-// on branches that don't support jdk7, this will already be
JAVA_HOME, so we'll end up not
-// doing multijdk there.
-MULTIJDK = '/usr/lib/jvm/java-8-openjdk-amd64'
+// all jdks tested on this stage for this branch
+// note that JAVA_HOME still needs to be set for tests
+// that don't support multijdk. JAVA_HOME will also
+// determine which jvm will be tested last.
+MULTIJDK = '/usr/lib/jvm/zulu-8-amd64,/usr/lib/jvm/zulu-7-amd64'
Review comment:
nit: I expected to see these ordered ascending by version number.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set
jdk7 as JAVA_HOME for branches-1 precommit and nightly.
URL: https://github.com/apache/hbase/pull/621#discussion_r324854698
##
File path: dev-support/hbase_nightly_yetus.sh
##
@@ -40,6 +40,10 @@ if [[ -n "${MULTIJDK}" ]]; then
YETUS_ARGS=("--multijdkdirs=${MULTIJDK}" "${YETUS_ARGS[@]}")
fi
+# If we're doing docker, make sure we don't accidentally pollute the image
with a host java path
Review comment:
So we do run nightly on not-Jenkins? Or you mean for developers doing local
testing...
What's the issue with it running slowly on OSX? Is that an upstream bug
tracked somewhere?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly. URL: https://github.com/apache/hbase/pull/621#discussion_r324853088 ## File path: dev-support/docker/Dockerfile ## @@ -97,9 +94,7 @@ RUN apt-get -q install --no-install-recommends -y zulu-8 zulu-7 RUN update-alternatives --config java RUN update-alternatives --config javac RUN mv /usr/lib/jvm/java-7-openjdk-amd64 /usr/lib/jvm/moved.java-7-openjdk-amd64 -RUN mv /usr/lib/jvm/zulu-7-amd64 /usr/lib/jvm/java-7-openjdk-amd64 -RUN mv /usr/lib/jvm/zulu-8-amd64 /usr/lib/jvm/java-8-openjdk-amd64 - +ENV JAVA_HOME /usr/lib/jvm/zulu-7-amd64 Review comment: Perhaps a side-thought, but it seems like a concept of a `yetusrc` file would be useful -- a set of constants/declarations that can be committed to each branch. Or maybe that's not helpful because these values vary according to conditional logic as expressed in the `Jenkinsfile`? My somewhat selfish objective would be to have the `Jenkinsfile` definitions as small as possible, such that one could run as much of the pre-commit pipeline as possible from their local workstation, without magical Jenkins context. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly. URL: https://github.com/apache/hbase/pull/621#discussion_r324851766 ## File path: dev-support/hbase-personality.sh ## @@ -45,6 +45,23 @@ if ! declare -f "yetus_info" >/dev/null; then fi +# work around yetus overwriting JAVA_HOME from our docker image +function docker_do_env_adds Review comment: > Without this Yetus tells docker to unset any JAVA_HOME defined in the image if the host doesn't have JAVA_HOME set, which doesn't make sense to me. This sounds like to a bug to me as well. As far as I can see, the Yetus runtime from the build host should be completely agnostic to the runtime within the docker context -- no need for setting `JAVA_HOME` any more than there's a need for, for example, a `go` executable. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set
jdk7 as JAVA_HOME for branches-1 precommit and nightly.
URL: https://github.com/apache/hbase/pull/621#discussion_r324266066
##
File path: dev-support/hbase_nightly_yetus.sh
##
@@ -40,6 +40,10 @@ if [[ -n "${MULTIJDK}" ]]; then
YETUS_ARGS=("--multijdkdirs=${MULTIJDK}" "${YETUS_ARGS[@]}")
fi
+# If we're doing docker, make sure we don't accidentally pollute the image
with a host java path
+if [ -n "${JAVA_HOME}" ]; then
+ unset JAVA_HOME
+fi
if [[ -n "${SET_JAVA_HOME}" ]]; then
YETUS_ARGS=("--java-home=${SET_JAVA_HOME}" "${YETUS_ARGS[@]}")
Review comment:
Per earlier comments, I think we should always explicitly call `--java-home`
and never leave it up to sniffing around.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly. URL: https://github.com/apache/hbase/pull/621#discussion_r324264248 ## File path: dev-support/hbase-personality.sh ## @@ -45,6 +45,23 @@ if ! declare -f "yetus_info" >/dev/null; then fi +# work around yetus overwriting JAVA_HOME from our docker image +function docker_do_env_adds Review comment: I think this function can be skipped -- instead, pass `--java-home` as an invocation argument to `test-patch`. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set
jdk7 as JAVA_HOME for branches-1 precommit and nightly.
URL: https://github.com/apache/hbase/pull/621#discussion_r324265255
##
File path: dev-support/Jenkinsfile
##
@@ -131,9 +131,8 @@ curl -L -o personality.sh "${env.PROJECT_PERSONALITY}"
// TODO does hadoopcheck need to be jdk specific?
// Should be things that work with multijdk
TESTS = 'all,-unit,-findbugs'
-// on branches that don't support jdk7, this will already be
JAVA_HOME, so we'll end up not
-// doing multijdk there.
-MULTIJDK = '/usr/lib/jvm/java-8-openjdk-amd64'
+// this is only elevant on branches-1. on branches with default
jdk8, the relevant jdk will be JAVA_HOME
+MULTIJDK = '/usr/lib/jvm/zulu-8-amd64'
Review comment:
This still trips me up. I'd rather see both (all) JDK's explicitly listed
out in the `--multijdkdirs` argument. It's explicitly clear, vs. "use this one
plus whatever I happens to resolve into `JAVA_HOME` later."
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly. URL: https://github.com/apache/hbase/pull/621#discussion_r324264609 ## File path: dev-support/docker/Dockerfile ## @@ -97,9 +94,7 @@ RUN apt-get -q install --no-install-recommends -y zulu-8 zulu-7 RUN update-alternatives --config java RUN update-alternatives --config javac RUN mv /usr/lib/jvm/java-7-openjdk-amd64 /usr/lib/jvm/moved.java-7-openjdk-amd64 -RUN mv /usr/lib/jvm/zulu-7-amd64 /usr/lib/jvm/java-7-openjdk-amd64 -RUN mv /usr/lib/jvm/zulu-8-amd64 /usr/lib/jvm/java-8-openjdk-amd64 - +ENV JAVA_HOME /usr/lib/jvm/zulu-7-amd64 Review comment: Rather than manage the variable in Docker, why not pass it in as `--java-home` ? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set
jdk7 as JAVA_HOME for branches-1 precommit and nightly.
URL: https://github.com/apache/hbase/pull/621#discussion_r324265807
##
File path: dev-support/hbase_nightly_yetus.sh
##
@@ -40,6 +40,10 @@ if [[ -n "${MULTIJDK}" ]]; then
YETUS_ARGS=("--multijdkdirs=${MULTIJDK}" "${YETUS_ARGS[@]}")
fi
+# If we're doing docker, make sure we don't accidentally pollute the image
with a host java path
Review comment:
Does this ever not run in Docker mode? Can we settle on only running in
Docker mode? It keeps things simpler and tidy, IMHO. The possibility of host
environment values leaking in makes for bizarre bugs.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set jdk7 as JAVA_HOME for branches-1 precommit and nightly.
ndimiduk commented on a change in pull request #621: HBASE-22955 Properly set
jdk7 as JAVA_HOME for branches-1 precommit and nightly.
URL: https://github.com/apache/hbase/pull/621#discussion_r324264734
##
File path: dev-support/Jenkinsfile_GitHub
##
@@ -110,6 +110,10 @@ pipeline {
if [[ "true" = "${DEBUG}" ]]; then
YETUS_ARGS+=("--debug")
fi
+# If we're doing docker, make sure we don't
accidentally pollute the image with a host java path
Review comment:
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
