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

Jonathan Gibbons jonathan.gibbons at oracle.com
Tue Jan 8 00:25:33 UTC 2019


Andrew,

I've put out an updated webrev, that is somewhat simpler than your 
version, although it should be functionally equivalent. In particular, 
it has a much smaller impact on the non-WSL code.

Some questions:

  * Why is it important to distinguish "path" and "path list"? Isn't a
    "path" just a "path list" with a single entry? In other words, can
    we always use suffix `/l` for a list of one-or-more paths?
  * Is it important to ignore case when checking for "PATH"? Would the
    Windows environment ever contain an env variable "path"?
  * The env should never have entries in it with "blank" names; was it
    just defensive programming on your part to check for blank names and
    ignore them, or did you see them in practice?
  * I'm not set up here right now to be able to test with WSL; can you
    try building and testing this version of jtreg?

-- Jon

Webrev: http://cr.openjdk.java.net/~jjg/7902357/webrev.02/



On 01/07/2019 03:23 PM, Andrew Luo wrote:
>
> 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:
>
> ifcase${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
>
> exit1
>
> 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 
> <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/ 
> <http://cr.openjdk.java.net/%7Ejjg/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/ 
> <http://cr.openjdk.java.net/%7Ejjg/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.
>
>         (Seehttps://bugs.openjdk.java.net/browse/JDK-8215445  for more details
>
>         about supporting WSL for building OpenJDK)
>
>         Thanks,
>
>         -Andrew
>



More information about the code-tools-dev mailing list