Added quoting around compile properties #577

judovana · 2024-06-24T18:03:23Z

Hello!

We are hitting:

**************************************************************************

System.getProperty('java.vm.name')=OpenJDK 64-Bit Server VM

System.getProperty('java.vendor')=Undefined Vendor

Warning: cannot determine vendor, use System.getProperty('java.vendor')=Undefined Vendor directly.

System.getProperty('os.name')=Linux

System.getProperty('os.arch')=aarch64

System.getProperty('java.fullversion')=null

System.getProperty('sun.arch.data.model')=64

make -f clean.mk cleanBuild
make[1]: Entering directory '/home/tester/aqa-crypto-tests/aqa-tests/TKG'
rm -f -r /home/tester/aqa-crypto-tests/aqa-tests/TKG/../../jvmtest
make[1]: Leaving directory '/home/tester/aqa-crypto-tests/aqa-tests/TKG'
make -f compile.mk compile
make[1]: Entering directory '/home/tester/aqa-crypto-tests/aqa-tests/TKG'
rm -f -r /home/tester/aqa-crypto-tests/aqa-tests/TKG/../TKG/output_compilation; \
mkdir -p /home/tester/aqa-crypto-tests/aqa-tests/TKG/../TKG/output_compilation; \
(ant -f scripts/build_test.xml -DTEST_ROOT=/home/tester/aqa-crypto-tests/aqa-tests/TKG/.. -DBUILD_ROOT=/home/tester/aqa-crypto-tests/aqa-tests/TKG/../../jvmtest -DJDK_VERSION=21 -DJDK_IMPL=hotspot -DJDK_VENDOR=Undefined Vendor -DJCL_VERSION=latest -DBUILD_LIST=functional/security/Crypto -DRESOURCES_DIR=/home/tester/aqa-crypto-tests/aqa-tests/TKG/../../jvmtest/TestConfig/resources -DSPEC=linux_aarch64 -DTEST_JDK_HOME=/usr/lib/jvm/java-21-openjdk-jdk.21.0.4.5-0.ojdk21~u~upstream.hotspot.fastdebug.sdk.el8.aarch64.tarxz -DJVM_VERSION=openjdk21 -DLIB_DIR=/tmp/tmp.CgKlo9A1Qu  2>&1; echo $? ) | tee "/home/tester/aqa-crypto-tests/aqa-tests/TKG/../TKG/output_compilation/compilation.log"; \
if [ -z "$(tail -1 /home/tester/aqa-crypto-tests/aqa-tests/TKG/../TKG/output_compilation/compilation.log | grep 0)" ]; then perl scripts/moveDmp.pl --compileLogPath="/home/tester/aqa-crypto-tests/aqa-tests/TKG/../TKG/output_compilation/compilation.log" --testRoot="/home/tester/aqa-crypto-tests/aqa-tests/TKG/.."; false; else rm -f -r "/home/tester/aqa-crypto-tests/aqa-tests/TKG/../TKG/output_compilation"; fi
Picked up JAVA_TOOL_OPTIONS: -Djava.security.manager=allow
Buildfile: /home/tester/aqa-crypto-tests/aqa-tests/TKG/scripts/build_test.xml
     [echo]  build.list is TestConfig/build.xml,functional/security/Crypto/build.xml 

BUILD FAILED
Target "Vendor" does not exist in the project "Compile Java Test Projects". 

Total time: 0 seconds
1
make[1]: Leaving directory '/home/tester/aqa-crypto-tests/aqa-tests/TKG'
make[1]: *** [compile.mk:45: compile] Error 1
make: *** [makefile:87: compile] Error 2

Which suggests that under some circumstances vendor with spaces is not handled properly. THis PR may be very wrong, the call from aqa-tests may be guilty., BUt I have hard times to manually trigger it, however:

In case of rpms vendor is: "Red Hat, Inc."
Seems like some logic simplifies it to "redhat" and it is then passed as that:
ant -f scripts/build_test.xml ... -DJDK_VENDOR=redhat ...

In case undef builds seems to be: "Undefined Vendor"
which it says is unrecognized and passed literally:
ant -f scripts/build_test.xml ... -DJDK_VENDOR=Undefined Vendor ...

judovana · 2024-06-25T11:08:01Z

We were able to workaround it by

  export JDK_VENDOR="redhat"
  export AUTO_DETECT="false"

But I stil think it should be quoted, thoughts?

smlambert · 2024-06-25T13:42:43Z

Happy to bring this change in (with some additional testing expected), fixes a "closed as won't fix" issue #502

sophia-guo · 2024-06-25T16:45:35Z

compile.mk

@@ -38,7 +38,7 @@ TEST_FLAG_PARAM := -DTEST_FLAG=$(TEST_FLAG)
 else
 TEST_FLAG_PARAM :=
 endif
-COMPILE_CMD=ant -f scripts$(D)build_test.xml -DTEST_ROOT=$(TEST_ROOT) -DBUILD_ROOT=$(BUILD_ROOT) -DJDK_VERSION=$(JDK_VERSION) -DJDK_IMPL=$(JDK_IMPL) -DJDK_VENDOR=$(JDK_VENDOR) -DJCL_VERSION=$(JCL_VERSION) -DBUILD_LIST=${COMPILE_BUILD_LIST} -DRESOURCES_DIR=${RESOURCES_DIR} -DSPEC=${SPEC} -DTEST_JDK_HOME=${TEST_JDK_HOME} -DJVM_VERSION=$(JVM_VERSION) -DLIB_DIR=$(LIB_DIR) ${TEST_FLAG_PARAM}
+COMPILE_CMD=ant -f scripts$(D)build_test.xml -DTEST_ROOT="$(TEST_ROOT)" -DBUILD_ROOT="$(BUILD_ROOT)" -DJDK_VERSION="$(JDK_VERSION)" -DJDK_IMPL="$(JDK_IMPL)" -DJDK_VENDOR="$(JDK_VENDOR)" -DJCL_VERSION="$(JCL_VERSION)" -DBUILD_LIST="${COMPILE_BUILD_LIST}" -DRESOURCES_DIR="${RESOURCES_DIR}" -DSPEC="${SPEC}" -DTEST_JDK_HOME="${TEST_JDK_HOME}" -DJVM_VERSION="$(JVM_VERSION)" -DLIB_DIR="$(LIB_DIR)" ${TEST_FLAG_PARAM}


Could we only quote JDK_VENDOR or non directory one unless this can be tested in windows platforms (jenkins and local)?

Also, we may need to use $(Q) to replace " and test it works on all platforms.

judovana · 2024-06-26T06:25:13Z

Sure. TY! Will elebaorate

judovana · 2024-06-26T06:26:51Z

Jsut a question - I woudl thought, that the directories one would be the most important, due to possible space in path... But seeing the #502 I smell troubles :)

sophia-guo · 2024-06-26T20:46:54Z

yes, feel like directory ones would be the most possible ones with spaces in path, especially for windows and mac. However we haven't got problems by now. The reason we might need to be cautious is quotes might be added later somewhere else. Then there will be extra quotes. Though if tests on windows and mac are fine then we are good to do that.

judovana · 2024-07-10T14:26:28Z

Hello! Sorry for delay. I had did all suggested changes. Once this beak in, I will submit PR, which will be quoting all, as it seesm to be correct way to do so. All local testing on linux passed

judovana · 2024-07-10T14:31:50Z

$ git checkout vendorWithSpaces && JDK_VENDOR="part1 part2" AUTO_DETECT=false TEST_JDK_HOME=/usr/lib/jvm/java-11-openjdk make 
Already on 'vendorWithSpaces'
Your branch is up to date with 'origin/vendorWithSpaces'.
JAVA_HOME is set to /usr/lib/jvm/java-11-openjdk
UNAME_OS is Linux
LIB_DIR is set to /home/jvanek/git/TKG/../TKG/lib
rm -f -r /home/jvanek/git/TKG/../TKG/output_compilation; \
mkdir -p /home/jvanek/git/TKG/../TKG/output_compilation; \
(ant -f ./scripts/build_tools.xml -DTEST_JDK_HOME=/usr/lib/jvm/java-11-openjdk -DTEST_ROOT=/home/jvanek/git/TKG/.. -DLIB_DIR=/home/jvanek/git/TKG/../TKG/lib 2>&1; echo $? ) | tee "/home/jvanek/git/TKG/../TKG/output_compilation/compilation.log"; \
if [ -z "$(tail -1 /home/jvanek/git/TKG/../TKG/output_compilation/compilation.log | grep 0)" ]; then perl scripts/moveDmp.pl --compileLogPath="/home/jvanek/git/TKG/../TKG/output_compilation/compilation.log" --testRoot="/home/jvanek/git/TKG/.."; false; else rm -f -r "/home/jvanek/git/TKG/../TKG/output_compilation"; fi
...
System.getProperty('java.vendor')=Red Hat, Inc.
...
mkdir -p /home/jvanek/git/TKG/../TKG/output_compilation; \
(ant -f scripts/build_test.xml -DTEST_ROOT=/home/jvanek/git/TKG/.. -DBUILD_ROOT=/home/jvanek/git/TKG/../../jvmtest -DJDK_VERSION=11 -DJDK_IMPL=hotspot "-DJDK_VENDOR=part1 part2" -DJCL_VERSION=latest -DBUILD_LIST= -DRESOURCES_DIR=/home/jvanek/git/TKG/../../jvmtest/TestConfig/resources -DSPEC=linux_x86-64 -DTEST_JDK_HOME=/usr/lib/jvm/java-11-openjdk -DJVM_VERSION=openjdk11 -DLIB_DIR=/home/jvanek/git/TKG/../TKG/lib  2>&1; echo $? ) | tee "/home/jvanek/git/TKG/../TKG/output_compilation/compilation.log"; \
if [ -z "$(tail -1 /home/jvanek/git/TKG/../TKG/output_compilation/compilation.log | grep 0)" ]; then perl scripts/moveDmp.pl --compileLogPath="/home/jvanek/git/TKG/../TKG/output_compilation/compilation.log" --testRoot="/home/jvanek/git/TKG/.."; false; else rm -f -r "/home/jvanek/git/TKG/../TKG/output_compilation"; fi
Buildfile: /home/jvanek/git/TKG/scripts/build_test.xml
     [echo]  build.list is */build.xml 

-create_test_directory:
    [mkdir] Created dir: /home/jvanek/jvmtest

stage_test_material:
     [copy] Copying 134 files to /home/jvanek/jvmtest

build:

-pre-init:

....

git checkout master  && JDK_VENDOR="part1 part2" AUTO_DETECT=false TEST_JDK_HOME=/usr/lib/jvm/java-11-openjdk make 
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
JAVA_HOME is set to /usr/lib/jvm/java-11-openjdk
UNAME_OS is Linux
LIB_DIR is set to /home/jvanek/git/TKG/../TKG/lib
...
System.getProperty('java.vendor')=Red Hat, Inc.
...
mkdir -p /home/jvanek/git/TKG/../TKG/output_compilation; \
(ant -f scripts/build_test.xml -DTEST_ROOT=/home/jvanek/git/TKG/.. -DBUILD_ROOT=/home/jvanek/git/TKG/../../jvmtest -DJDK_VERSION=11 -DJDK_IMPL=hotspot -DJDK_VENDOR=part1 part2 -DJCL_VERSION=latest -DBUILD_LIST= -DRESOURCES_DIR=/home/jvanek/git/TKG/../../jvmtest/TestConfig/resources -DSPEC=linux_x86-64 -DTEST_JDK_HOME=/usr/lib/jvm/java-11-openjdk -DJVM_VERSION=openjdk11 -DLIB_DIR=/home/jvanek/git/TKG/../TKG/lib  2>&1; echo $? ) | tee "/home/jvanek/git/TKG/../TKG/output_compilation/compilation.log"; \
if [ -z "$(tail -1 /home/jvanek/git/TKG/../TKG/output_compilation/compilation.log | grep 0)" ]; then perl scripts/moveDmp.pl --compileLogPath="/home/jvanek/git/TKG/../TKG/output_compilation/compilation.log" --testRoot="/home/jvanek/git/TKG/.."; false; else rm -f -r "/home/jvanek/git/TKG/../TKG/output_compilation"; fi
Buildfile: /home/jvanek/git/TKG/scripts/build_test.xml
     [echo]  build.list is */build.xml 

BUILD FAILED
Target "part2" does not exist in the project "Compile Java Test Projects". 

Total time: 0 seconds
1
make[1]: *** [compile.mk:45: compile] Error 1
make[1]: Leaving directory '/home/jvanek/git/TKG'
make: *** [makefile:87: compile] Error 2

judovana · 2024-07-11T18:34:47Z

@sophia-guo @smlambert @LongyuZhang Just in case it is not clear, the #577 (comment) is showing the improvement this PR does :) (I opened it today and it is not clear..but it really shows that.)

LongyuZhang

LGTM.

judovana · 2024-07-12T12:19:23Z

tyvm!

Added quoting around compile properties

4f45845

smlambert requested review from sophia-guo and LongyuZhang June 25, 2024 16:07

sophia-guo reviewed Jun 25, 2024

View reviewed changes

judovana added 3 commits July 10, 2024 16:13

Quoting jsut the vendor

49f5500

Quoting whole argument (seems more correct)

b3c6b07

Replaced hardcoded " by $(Q)

626b673

judovana requested a review from sophia-guo July 10, 2024 14:32

smlambert approved these changes Jul 11, 2024

View reviewed changes

LongyuZhang approved these changes Jul 12, 2024

View reviewed changes

LongyuZhang requested a review from llxia July 12, 2024 11:30

smlambert merged commit ba9b3b1 into adoptium:master Jul 12, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added quoting around compile properties #577

Added quoting around compile properties #577

judovana commented Jun 24, 2024

judovana commented Jun 25, 2024

smlambert commented Jun 25, 2024

sophia-guo Jun 25, 2024 •

edited

Loading

LongyuZhang Jun 25, 2024

judovana commented Jun 26, 2024

judovana commented Jun 26, 2024

sophia-guo commented Jun 26, 2024

judovana commented Jul 10, 2024

judovana commented Jul 10, 2024

judovana commented Jul 11, 2024

LongyuZhang left a comment

judovana commented Jul 12, 2024

Added quoting around compile properties #577

Added quoting around compile properties #577

Conversation

judovana commented Jun 24, 2024

judovana commented Jun 25, 2024

smlambert commented Jun 25, 2024

sophia-guo Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

LongyuZhang Jun 25, 2024

Choose a reason for hiding this comment

judovana commented Jun 26, 2024

judovana commented Jun 26, 2024

sophia-guo commented Jun 26, 2024

judovana commented Jul 10, 2024

judovana commented Jul 10, 2024

judovana commented Jul 11, 2024

LongyuZhang left a comment

Choose a reason for hiding this comment

judovana commented Jul 12, 2024

sophia-guo Jun 25, 2024 •

edited

Loading