[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