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

[Andrew Luo]

Hi Jon,

Thanks for reviewing this.  Yes, there is a bug there, it should be !result.isEmpty() at the end of joinWSLENV.  Is it still the case that we support JDK 7?  I got the impression from build-all.sh:48 that JDK 7 is no longer supported:

if case ${JAVA_VERSION} in 1.8*) false ;; *) true; esac; then
    echo "Error: Expected a path to JDK with version 1.8, got version ${JAVA_VERSION}" >&2
    exit 1
fi

Or perhaps is this only the build-all.sh script that doesn’t support JDK 7, build jtreg itself does?

As for the changes in ShellAction, the reason I did it that way instead of scanning through env, is because some environment variables have to be translated raw, others are paths, and others are path lists (multiple paths separated by ; in Windows).  These all have to be added into WSLENV differently (in particular with the /p or /l suffix).  Scanning through would be a bit error prone, as you’d have to decide if the variable is a raw string, path, or path list just using the value.  Although it’s possible, it seems a bit error prone/clunky.  If you have some preferred solution to this though, I’d be happy to make those changes.

Thanks,

-Andrew

From: Jonathan Gibbons <jonathan.gibbons at oracle.com>
Sent: Monday, January 7, 2019 2:20 PM
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,

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><mailto:jonathan.gibbons at oracle.com>

Sent: Monday, January 7, 2019 7:54 AM

To: Andrew Luo <andrewluotechnologies at outlook.com><mailto:andrewluotechnologies at outlook.com>; code-tools-dev at openjdk.java.net<mailto: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