[PATCH] Enable jtreg tests to run on WSL (Windows Subsystem for Linux)

[Jonathan Gibbons]

Andrew,

There are some initial problems with your patch.

1.  In ShellAction.joinWSLEnv ... it is still a self-imposed requirement 
that we can build jtreg with JDK 7 (to help support testing older 
releases of JDK.) In joinWSLEnv, you use String.join, which is @since 8, 
and so not available on 7.   For now, I've written a replacement, but in 
doing do. I wonder if there is a bug in your original.  In particular, 
at the end of the method, why do you add `suffix` if 
`result.isEmpty())`.  I would have expected this to be a test if the 
result was _not_ empty.

Anyway, here is my proposed replacement for the method:

        private String joinWSLENV(List<String> list, String suffix) {
             StringBuilder sb =new StringBuilder();
             String sep ="";
             for (String item : list) {
                 if (item.trim().isEmpty() || item.toLowerCase().equals("path")) {
                     continue;
                 }

                 sb.append(sep).append(item).append(suffix);
                 sep =":";
             }

             return sb.toString();
        }

The code appends the ":" separator after the first item, and always 
appends the suffix.   Is that what you believe it should do?


2. You added a new option, but did not add an entry into the resource 
file. This causes the jtreg build to fail one of its self-tests.  I've 
created a placeholder entry, but it is at the semantic level I hate: 
"getFo(): gets a foo".  If you can provide a more informative string, 
that would be better.

$ hg diff src/share/classes/com/sun/javatest/regtest/tool/i18n.properties
diff -r b6fabf6bf942 
src/share/classes/com/sun/javatest/regtest/tool/i18n.properties
--- a/src/share/classes/com/sun/javatest/regtest/tool/i18n.properties 
Thu Dec 20 14:43:41 2018 -0800
+++ b/src/share/classes/com/sun/javatest/regtest/tool/i18n.properties 
Mon Jan 07 13:38:40 2019 -0800
@@ -257,6 +257,7 @@
      The name may contain '*' to match any sequence of characters. \
      For example, result.* or *.err.
  help.main.retain.arg=<pass,fail,error,all,file-pattern>,...
+help.main.wslWindowsTarget.desc=(Windows only) Run with WSL targeting 
Windows
  help.main.dir.desc=Specify a base directory for test files and directories
  help.main.dir.arg=<dir>
  help.main.allowSetSecurityManager.desc=Allow agentVM tests to set a 
security manager. \

3. README. It's not OK to change the baseline JDK used to build jtreg.  
If there is a WSL-reason why JDK 8 is required to run jtreg in a WSL 
environment, that should be done as a runtime check.

Beyond that, the changes to ShellAction feel a bit clunky, changing the 
non-WSL flow of the code more than might be thought necessary. After 
all, there's really only two changes here: inserting wsl.exe at the 
begtinning of the command, and computing/inserting WSLENV into the 
environment. Given that WSLENV only contains the names of the env 
variables, with suffixes as needed, I would have thought that a simpler 
impl would be to scan the keys in env to build WSLENV, using 
strings-in-switch to identify the suffixes, and do that all in a 
relatively simple helper method, without having to affect the mainline 
code to put values into the env.

I've created CODETOOLS-7902357 to track this work. 
https://bugs.openjdk.java.net/browse/CODETOOLS-7902357
I've put a webrev containing your patch at 
http://cr.openjdk.java.net/~jjg/7902357/webrev.00/
I've put a followup containing your patch + fixes for 1-2-3 above at 
http://cr.openjdk.java.net/~jjg/7902357/webrev.01/

-- Jon

On 01/07/2019 12:36 PM, Andrew Luo wrote:
> Hi Jon,
>
> For a general intro to WSL, see here:
>
> https://blogs.msdn.microsoft.com/wsl/2016/04/22/windows-subsystem-for-linux-overview/
> https://docs.microsoft.com/en-us/windows/wsl/faq
>
> These links might also be useful for reviewing these changes specifically:
>
> https://docs.microsoft.com/en-us/windows/wsl/interop
> https://blogs.msdn.microsoft.com/commandline/2017/12/22/share-environment-vars-between-wsl-and-windows/
>
> Thanks,
>
> -Andrew
>
> -----Original Message-----
> From: Jonathan Gibbons <jonathan.gibbons at oracle.com>
> Sent: Monday, January 7, 2019 7:54 AM
> To: Andrew Luo <andrewluotechnologies at outlook.com>; code-tools-dev at openjdk.java.net
> Subject: Re: [PATCH] Enable jtreg tests to run on WSL (Windows Subsystem for Linux)
>
> Andrew,
>
> I've been generally following the progress for supporting WSL, but now that I need to get more involved, (i.e. reviewing jtreg updates) can you point me at any reasonable general introductory material for WSL?
>
> -- Jon
>
> On 1/6/19 11:29 AM, Andrew Luo wrote:
>> We recently added support for building OpenJDK on WSL (targeting
>> Windows; targeting Linux always worked).  However, running tests on
>> WSL did not work, and we determined that some changes would be needed
>> to jtreg to support this use case.  In particular, jtreg needs to know
>> if it is running on WSL targeting Windows or WSL targeting Linux (when
>> targeting Linux, no changes are needed as it is the same as any other
>> Linux system, but when targeting Windows jtreg needs to run shell
>> scripts in a special manner - in particular, calling "wsl.exe sh"
>> instead of just "sh" since it's a Windows boot JDK, as well as
>> translating environment variables using WSLENV...)
>>
>> I've attached my patch with my proposed changes.  I welcome any thoughts/feedback.
>>
>> (See https://bugs.openjdk.java.net/browse/JDK-8215445 for more details
>> about supporting WSL for building OpenJDK)
>>
>> Thanks,
>>
>> -Andrew