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

Output generic TEST_JDK_HOME in job.properties #3476

Merged
merged 11 commits into from
Apr 5, 2022

Conversation

JeromeJu
Copy link
Contributor

Always output TEST_JDK_HOME into job.properties at get.sh, therefore in JenkinsfileBase the TEST_JDK_HOME would always be read regardless the condition checks.

Fixes: #3128

Sign off by: Jerome Ju jeromeqju@gmail.com

buildenv/jenkins/JenkinsfileBase Outdated Show resolved Hide resolved
buildenv/jenkins/JenkinsfileBase Outdated Show resolved Hide resolved
get.sh Outdated Show resolved Hide resolved
get.sh Outdated Show resolved Hide resolved
@JeromeJu JeromeJu marked this pull request as ready for review March 21, 2022 15:50
buildenv/jenkins/JenkinsfileBase Outdated Show resolved Hide resolved
buildenv/jenkins/JenkinsfileBase Outdated Show resolved Hide resolved
buildenv/jenkins/JenkinsfileBase Show resolved Hide resolved
@JeromeJu JeromeJu requested a review from llxia March 21, 2022 16:35
@JeromeJu
Copy link
Contributor Author

Thanks Lan for the pointers!

@llxia
Copy link
Contributor

llxia commented Mar 21, 2022

@JeromeJu Thanks for the update. We will need to test on xlinux, wins, mac and zos. I will take care of zos. Could you provide Grinder links for the rest of 3 platforms?

@JeromeJu
Copy link
Contributor Author

JeromeJu commented Mar 22, 2022

Built Success
mac: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/3866/
image

linux: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/3865/
image

Failed with hudson.AbortException
win_x86: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/3867/
image

^^^ Looking into this...

22:21:35       [echo] Ant version is Apache Ant(TM) version 1.10.5 compiled on July 10 2018
22:21:35       [echo] ============COMPILER SETTINGS============
22:21:35       [echo] ===fork:                         yes
22:21:35       [echo] ===debug:                        on
22:21:35      [javac] Compiling 25 source files to E:\workspace\Grinder\aqa-tests\TKG\bin
22:21:35  
22:21:35  BUILD FAILED
22:21:35  E:\workspace\Grinder\aqa-tests\TKG\scripts\build_tools.xml:58: The following error occurred while executing this line:
22:21:35  E:\workspace\Grinder\aqa-tests\TKG\scripts\build_tools.xml:37: Error running \cygdrive\e\workspace\Grinder\aqa-tests\..\openjdkbinary\j2sdk-image\bin\javac compiler
22:21:35  
22:21:35  Total time: 0 seconds
22:21:35  1
22:21:35  make: *** [makefile:94: compileTools] Error 1

@llxia
Copy link
Contributor

llxia commented Mar 22, 2022

zos: /Grinder/21919

image

@JeromeJu
Copy link
Contributor Author

I noticed that for the windows_x86,

	    <target name="compile" depends="init,getDependentLibs" description="Using java ${JDK_VERSION} to compile the source  ">
		    <echo>Ant version is ${ant.version}</echo>
		    <echo>============COMPILER SETTINGS============</echo>
		    <echo>===fork:                         yes</echo>
		    <echo>===debug:                        on</echo>
>>>		    <javac srcdir="${src}" destdir="${build}" debug="true" fork="true" executable="${TEST_JDK_HOME}/bin/javac" includeAntRuntime="false" encoding="ISO-8859-1">
			    <classpath>
				    <pathelement location="${json.jar}" />
			    </classpath>
		    </javac>
	    </target>

@llxia
Copy link
Contributor

llxia commented Mar 22, 2022

For windows, could you try with JDK11?

@JeromeJu
Copy link
Contributor Author

JeromeJu commented Mar 24, 2022

I've added the conversion to windows path from cygpath at https://github.com/JeromeJu/aqa-tests/blob/3128_TEST_JDK_HOME/get.sh#L602-L604.
Now it does ouput the correct windows path into job.properties by cat in another test_3128 branch in https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/3910/console.

However, I could not figure out why when the JenkinsfileBase reads it, the slashes are all gone as follow:

[](https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/3911/console#)[](https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/3911/console#)19:28:46  readProperties file: ./aqa-tests/job.properties
[Pipeline] readProperties
[Pipeline] echo
19:28:47  Set TEST_JDK_HOME to C:UsersJenkinsworkspaceGrinderopenjdkbinaryj2sdk-image

@llxia
Copy link
Contributor

llxia commented Mar 28, 2022

Could you try to add double quotes around TEST_JDK_HOME?

@JeromeJu
Copy link
Contributor Author

Hi @llxia, I figured it's the redirect issue of backslash in the converted windows paths. I used sed to circumvent that.

Grinder links:
mac: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/4215/
linux: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/4214/
win: https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/4213/

Could you plz help rerun the zos as I could not find the params according to your previous job number?

Thanks!

@JeromeJu
Copy link
Contributor Author

get.sh Outdated Show resolved Hide resolved
@JeromeJu
Copy link
Contributor Author

JeromeJu commented Apr 1, 2022

@llxia
Copy link
Contributor

llxia commented Apr 4, 2022

zos: Grinder/22280

Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

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

LGTM

@smlambert smlambert merged commit b18a3e4 into adoptium:master Apr 5, 2022
@@ -599,6 +599,12 @@ testJavaVersion()
echo "=JAVA VERSION OUTPUT BEGIN="
${_java} -version
echo "=JAVA VERSION OUTPUT END="
if [ "$os" = "windows" ]; then
TEST_JDK_HOME=$(cygpath -w "${TEST_JDK_HOME}")
echo -e "TEST_JDK_HOME=${TEST_JDK_HOME}" | sed 's/\\/\\\\/g' > ${TESTDIR}/job.properties
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you want -e here. Suppose ${TEST_JDK_HOME} is C:\test\foo\bar, -e will map \t to a tab which isn't likely to work.

@pshipton
Copy link
Contributor

pshipton commented Apr 5, 2022

All of a sudden the OpenJ9 testing is failing to run javac, at least on Windows, and this is what changed.
See eclipse-openj9/openj9#14857

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.

TEST_JDK_HOME should be detected in get.sh
5 participants