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

Fri Jan 18 23:56:11 UTC 2019

Andrew,

I guess I don't yet understand why we would need WSL_TARGET.

If a shell script has been started by jtreg, can you provide a table 
describing the observable operational differences (e.g. `os.*` system 
properties, `uname` output etc) in the following environments:

* Real Linux
* Windows with Cygwin
* Windows with WSL

In addition, assuming the existence of EXE_SUFFIX, can you explain why 
some tests may still not work?  I think you were trying to explain that 
earlier, but it seemed to be mixed up with a discussion of the jtreg 
-wsl option, which ought to be a separable issue.   For tests that use 
the "case ${OS}" convention, what is the simplest way to modify the code 
pattern to cope with WSL? Likewise, setting efficiency aside, what is 
the simplest way to extend the code pattern in 
test/hotspot/jtreg/test_env.sh for WSL? I guess I'm looking for 
indications that WSL either behaves the same as an existing case (e.g. 
it's like Linux) or it can be handled as a new case in its own right, 
(e.g. like Cygwin).

-- Jon

On 01/18/2019 03:33 PM, Andrew Luo wrote:
> I generally agree with the points you made.  I wasn't meaning to discuss the test here and propose specific changes to them (that will of course need the relevant team to review), just was giving a few examples of places we could use WSL_TARGET.
>
> With regards to converting shell tests to Java code, I do realize that shell tests are, generally speaking, deprecated, however, I don't know how long it will take to fully remove them (in my experience at various companies, we've had "deprecated" features/code retained for sometimes as long as 5-10 years).  But if it's not happening anytime soon, I think it's important to have tests work on WSL if we want WSL to be a fully-supported platform for building OpenJDK.
>
> Anyways, I can make the jtreg WSL_TARGET changes as well as the shell scripts changes locally, and then seek review from the involved teams, but I wanted to make sure that it's okay with you guys to add WSL_TARGET to jtreg in the first place, if that indeed does solve our issues.
>
> Thanks,
>
> -Andrew
>
> -----Original Message-----
> From: Jonathan Gibbons <jonathan.gibbons at oracle.com>
> Sent: Friday, January 18, 2019 3:15 PM
> To: Andrew Luo <andrewluotechnologies at outlook.com>; code-tools-dev at openjdk.java.net
> Subject: Re: [PATCH] Fix for EXE_SUFFIX being set for WSL having no effect
>
> 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
>>