-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
issue #41 issue #45 Enable tests on windows #46
Conversation
Jenkins build current have two other issues:
All is good. Ask for deliver those fixes first and open specific issues and investigate. |
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.
LGTM
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.
A few Q's / comments
JAVA_BIN='$WORKSPACE/openjdkbinary/j2sdk-image/jre/bin' | ||
JCL_VERSION='current' | ||
OPENJDK_TEST='$WORKSPACE/openjdk-test' | ||
JAVA_BIN='C:/Users/jenkins/workspace/openjdk_test_x86-64_windows/openjdkbinary/j2sdk-image/jre/bin' |
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.
worth putting C:/Users/jenkins/workspace/openjdk_test_x86-64_windows
into a var?
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.
See issue #41 for jenkins there is an environment variable for this value WORKSPACE. However it doesn't work for jenkins pipeline environment section on windows. So this is temporary solution. Eventually we'd like using WORKSPACE variable.
*CYGWIN*) | ||
wget https://github.com/AdoptOpenJDK/openjdk-releases/releases/download/jdk8u152-b04/OpenJDK8_x64_Win_jdk8u152-b04.zip | ||
unzip OpenJDK8_x64_Win_jdk8u152-b04.zip | ||
;; |
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.
Hard coded binaries? Should be parameterized?
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.
This will be enhanced depending on how we would like to run the tests(e.g. nightly only run latest? developer run specific version? What is the available gitHub APIs...). Currently we'd like to enable the build first as it seems build on windows have more issues than on Linux.
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.
OK, as long as we have an Issue or a TODO somewhere :-)
get.sh
Outdated
download_url=`curl https://api.github.com/repos/AdoptOpenJDK/openjdk-releases/releases/latest | jq -r '.assets[] | select(.browser_download_url | contains("x64_Linux_jdk")) | {url: .browser_download_url} | select (.url | contains("tar.gz")) | .[]'` | ||
wget "${download_url}" | ||
#wget https://github.com/AdoptOpenJDK/openjdk-releases/releases/download/jdk8u152-b01/OpenJDK8_x64_Linux_jdk8u152-b01.tar.gz | ||
jar_file_url=$download_url |
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.
commented out code?
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.
Removed.
get.sh
Outdated
jar_file_name=${jar_file_url##*/} | ||
tar -zxvf $jar_file_name | ||
#tar -zxvf OpenJDK8_x64_Linux_jdk8u152-b01.tar.gz ;; | ||
esac |
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.
commented out code?
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.
Removed.
Signed-off-by: Sophia Guo <[email protected]>
Given that variables OPENJDK_TEST and JAVA_BIN are only needed for specific stages move them from global settings to withEnv blocks. This way we won't need environment variable WORKSPACE in environment section on windows platform, which is a bug and not know when/will it be fixed. PR updated. @karianna @smlambert please take another review. Thanks. |
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.
LGTM
Signed-off-by: Sophia Guo [email protected]