[PATCH] Fix for EXE_SUFFIX being set for WSL having no effect

[Jonathan Gibbons]

Andrew,

This is the place to discuss what we might or might not need in jtreg, 
but this is not really the place to discuss style issues within the 
content of tests.

You are touching on a number of issues here.

1. With respect to the provision and use of a `-wsl` option, the 
suggestion to do without it, if possible, was intended for the 
convenience of the person running jtreg.  If you're running jtreg from 
the makefiles, it's not a big deal for the makefile to use the option, 
but not everyone runs tests that way; many folk have private scripts to 
run jtreg, which can be especially convenient when developing and 
debugging new tests.   It is reasonable for jtreg to provide the option, 
but it would be a bonus if jtreg was clever enough to determine when it 
was not necessary (e.g. running on Windows, with WSL installed, and with 
no Cygwin installed.)

2. With respect to the content of tests, for the purposes of this 
exercise of getting tests to run with WSL, we should be doing the 
minimum necessary to get the tests to function correctly when run in a 
WSL world. Based on earlier emails in this thread, it seemed like it was 
going to be enough to introduce and use `${EXE_SUFFIX}` as needed, which 
is why it was worth adding that into jtreg.

3. IMO, doing any non-trivial refactoring or rewriting of shell tests is 
out of scope for this work, and out of scope for this list. For 
historical reasons different teams have developed different styles for 
their tests, and anything beyond minimal fixup (like using 
${EXE_SUFFIX}) needs to be discussed with the appropriate team. If you 
need help identifying teams and corresponding email lists, just ask. 
And, even for minimal fixup, individual teams will want to be involved 
in the review process for any changes to their tests.

4. An equally if not more important reason not to do work beyond the 
minimum for shell tests is that, in general, there is an ongoing trend 
away from using shell tests at all, converting any such tests into Java 
code. The best way to make a shell test better is to convert it to 
Java!  See 
https://openjdk.java.net/jtreg/faq.html#can-i-and-should-i-write-shell-tests 
But again, such conversion work should be decoupled from WSL work and 
needs to be coordinated with the relevant component team.

-- Jon


On 01/18/2019 01:34 PM, Andrew Luo wrote:
> Sounds good.  My plan is to first try to address most of the failures in tier 1 rather than doing all tests at once.  There aren't that many shell tests in tier 1 so it won't be a lot of work wasted if other teams prefer a different approach.  Anyways, I want to have something that I know works before I propose it...
>
> Regarding the previous topic of whether or not we need the -wsl flag - I noticed in some places we are actually checking the type of JVM, in particular in test/hotspot/jtreg/test_env.sh:
>
> ${TESTJAVA}${FS}bin${FS}java${EXE_SUFFIX} ${TESTOPTS} -Xinternalversion | sed -e 's/[(][^)]*[)]//g' -e 's/ by "[^"]*"//g' > vm_version.out 2>&1
> echo "INT_VERSION=`cat vm_version.out 2>&1`"
>
> ...
>
> VM_OS="unknown"
> grep "aix" vm_version.out > ${NULL}
> if [ $? = 0 ]
> then
>    VM_OS="aix"
> fi
> grep "bsd" vm_version.out > ${NULL}
> if [ $? = 0 ]
> then
>    VM_OS="bsd"
> fi
> grep "linux" vm_version.out > ${NULL}
> if [ $? = 0 ]
> then
>    VM_OS="linux"
> fi
> grep "solaris" vm_version.out > ${NULL}
> if [ $? = 0 ]
> then
>    VM_OS="solaris"
> fi
> grep "windows" vm_version.out > ${NULL}
> if [ $? = 0 ]
> then
>    VM_OS="windows"
> fi
>
> This is potentially a way shell scripts can determine themselves whether or not they are in WSL, or perhaps if we want to remove the need for the -wsl flag to jtreg, we could implement some similar logic.  Just a thought though.  (I do also wonder why someone implemented this - for cross-compilation? As far as I'm aware, the OS running == JDK target OS in all cases except WSL)
>
> On a (somewhat related note), another thing I noticed is that some scripts have logic such as:
>
> OS=`uname -s`;
> # Set classpath separator
> case "$OS" in
>          Windows* | CYGWIN* )
> 	SEP=";"
>          ;;
>
> 	* )
> 	SEP=":"
> esac
>
> However, the classpath separator would depend on the platform of the JVM under test (; for a Windows JVM under test, : for a Linux JVM under test).  I could copy the above logic to detect the JVM under test in the script, but this seems like overkill just to check whether your target JVM is Windows or Linux.  So perhaps we could add another environment variable, say WSL_TARGET, which would either be set to "windows" for a Windows WSL target, or empty for Linux WSL target/non-WSL.  That way the above logic could be rewritten:
>
> OS=`uname -s`;
> # Set classpath separator
> case "$OS" in
>          Windows* | CYGWIN* )
> 	SEP=";"
>          ;;
>
> 	* )
> 	if [ "x${WSL_TARGET}" = "xwindows" ]; then
> 		SEP=";"
> 	else
> 		SEP=":"
> 	fi
> esac
>
> Of course, this could be implemented using VM_OS using the code above, but it seems to me to be pretty clumsy to have to invoke the JVM in every test just to determine whether you're target JVM is Windows or Linux, given that jtreg knows this information anyways and could just tell the scripts, via an environment variable.  Let me know your thoughts/what you prefer.
>
> Thanks,
>
> -Andrew
>